| Message ID | 20181112212532.13126-4-simon.k.r.goldschmidt@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Fix CVE-2018-18440 | expand |
Hi Simon, On Mon, Nov 12, 2018 at 7:25 PM Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> wrote: > diff --git a/fs/fs.c b/fs/fs.c > index adae98d021..4baf6b1c39 100644 > --- a/fs/fs.c > +++ b/fs/fs.c > @@ -428,13 +428,57 @@ int fs_size(const char *filename, loff_t *size) > return ret; > } > > -int fs_read(const char *filename, ulong addr, loff_t offset, loff_t len, > - loff_t *actread) > +#ifdef CONFIG_LMB Unrelated to your series, but I was wondering if we could get rid of the CONFIG_LMB option. As far as I can see all the architectures define it, the only exception being arch/sh. If you agree I can send a patch after your series gets applied that removes CONFIG_LMB.
On Tue, Nov 13, 2018 at 3:23 AM Fabio Estevam <festevam@gmail.com> wrote: > > Hi Simon, > > On Mon, Nov 12, 2018 at 7:25 PM Simon Goldschmidt > <simon.k.r.goldschmidt@gmail.com> wrote: > > > diff --git a/fs/fs.c b/fs/fs.c > > index adae98d021..4baf6b1c39 100644 > > --- a/fs/fs.c > > +++ b/fs/fs.c > > @@ -428,13 +428,57 @@ int fs_size(const char *filename, loff_t *size) > > return ret; > > } > > > > -int fs_read(const char *filename, ulong addr, loff_t offset, loff_t len, > > - loff_t *actread) > > +#ifdef CONFIG_LMB > > Unrelated to your series, but I was wondering if we could get rid of > the CONFIG_LMB option. > > As far as I can see all the architectures define it, the only > exception being arch/sh. > > If you agree I can send a patch after your series gets applied that > removes CONFIG_LMB. Sure, that would clean things up. Simon
On 11/13/18 6:47 AM, Simon Goldschmidt wrote: > On Tue, Nov 13, 2018 at 3:23 AM Fabio Estevam <festevam@gmail.com> wrote: >> >> Hi Simon, >> >> On Mon, Nov 12, 2018 at 7:25 PM Simon Goldschmidt >> <simon.k.r.goldschmidt@gmail.com> wrote: >> >>> diff --git a/fs/fs.c b/fs/fs.c >>> index adae98d021..4baf6b1c39 100644 >>> --- a/fs/fs.c >>> +++ b/fs/fs.c >>> @@ -428,13 +428,57 @@ int fs_size(const char *filename, loff_t *size) >>> return ret; >>> } >>> >>> -int fs_read(const char *filename, ulong addr, loff_t offset, loff_t len, >>> - loff_t *actread) >>> +#ifdef CONFIG_LMB >> >> Unrelated to your series, but I was wondering if we could get rid of >> the CONFIG_LMB option. >> >> As far as I can see all the architectures define it, the only >> exception being arch/sh. >> >> If you agree I can send a patch after your series gets applied that >> removes CONFIG_LMB. > > Sure, that would clean things up. > > Simon > NAK This patch-series does not provide what is needed. With odroid-c2_defconfig I get fdt list /reserved-memory/secmon@10000000 reserved-memory { secmon@10000000 { reg = <0x00000000 0x10000000 0x00000000 0x00200000>; no-map; }; }; => load mmc 0:1 0x10000000 dtb 22925 bytes read in 8 ms (2.7 MiB/s) So now I have successfully overwritten the secure monitor. Urrgh. As you have observed load is still writing into a memory area that is reserved by the device-tree. Please, iterate over the device tree to ensure that nothing is loaded into a reserved memory area. Do not expect board files to do anything but create the reserve-memory entry in the device tree. Best regards Heinrich
On 13.11.2018 20:42, Heinrich Schuchardt wrote: > On 11/13/18 6:47 AM, Simon Goldschmidt wrote: >> On Tue, Nov 13, 2018 at 3:23 AM Fabio Estevam <festevam@gmail.com> wrote: >>> Hi Simon, >>> >>> On Mon, Nov 12, 2018 at 7:25 PM Simon Goldschmidt >>> <simon.k.r.goldschmidt@gmail.com> wrote: >>> >>>> diff --git a/fs/fs.c b/fs/fs.c >>>> index adae98d021..4baf6b1c39 100644 >>>> --- a/fs/fs.c >>>> +++ b/fs/fs.c >>>> @@ -428,13 +428,57 @@ int fs_size(const char *filename, loff_t *size) >>>> return ret; >>>> } >>>> >>>> -int fs_read(const char *filename, ulong addr, loff_t offset, loff_t len, >>>> - loff_t *actread) >>>> +#ifdef CONFIG_LMB >>> Unrelated to your series, but I was wondering if we could get rid of >>> the CONFIG_LMB option. >>> >>> As far as I can see all the architectures define it, the only >>> exception being arch/sh. >>> >>> If you agree I can send a patch after your series gets applied that >>> removes CONFIG_LMB. >> Sure, that would clean things up. >> >> Simon >> > NAK > > This patch-series does not provide what is needed. With > odroid-c2_defconfig I get > > fdt list /reserved-memory/secmon@10000000 > reserved-memory { > secmon@10000000 { > reg = <0x00000000 0x10000000 0x00000000 0x00200000>; > no-map; > }; > }; > > => load mmc 0:1 0x10000000 dtb > 22925 bytes read in 8 ms (2.7 MiB/s) > > So now I have successfully overwritten the secure monitor. Urrgh. > > As you have observed load is still writing into a memory area that is > reserved by the device-tree. > > Please, iterate over the device tree to ensure that nothing is loaded > into a reserved memory area. Do not expect board files to do anything > but create the reserve-memory entry in the device tree. First of all, thanks for testing. I must admit I haven't tested this case, I just had the impression the existing function 'boot_fdt_add_mem_rsv_regions()' (in image-fdt.c) would do that. I'll have to dig into why it doesn't. Or do you have CONFIG_OF_LIBFDT disabled? In any case, it *was* my intention to include the dts memory reservation! I'll make sure I test that for a v2. Simon
On 11/13/18 9:01 PM, Simon Goldschmidt wrote: > On 13.11.2018 20:42, Heinrich Schuchardt wrote: >> On 11/13/18 6:47 AM, Simon Goldschmidt wrote: >>> On Tue, Nov 13, 2018 at 3:23 AM Fabio Estevam <festevam@gmail.com> >>> wrote: >>>> Hi Simon, >>>> >>>> On Mon, Nov 12, 2018 at 7:25 PM Simon Goldschmidt >>>> <simon.k.r.goldschmidt@gmail.com> wrote: >>>> >>>>> diff --git a/fs/fs.c b/fs/fs.c >>>>> index adae98d021..4baf6b1c39 100644 >>>>> --- a/fs/fs.c >>>>> +++ b/fs/fs.c >>>>> @@ -428,13 +428,57 @@ int fs_size(const char *filename, loff_t *size) >>>>> return ret; >>>>> } >>>>> >>>>> -int fs_read(const char *filename, ulong addr, loff_t offset, >>>>> loff_t len, >>>>> - loff_t *actread) >>>>> +#ifdef CONFIG_LMB >>>> Unrelated to your series, but I was wondering if we could get rid of >>>> the CONFIG_LMB option. >>>> >>>> As far as I can see all the architectures define it, the only >>>> exception being arch/sh. >>>> >>>> If you agree I can send a patch after your series gets applied that >>>> removes CONFIG_LMB. >>> Sure, that would clean things up. >>> >>> Simon >>> >> NAK >> >> This patch-series does not provide what is needed. With >> odroid-c2_defconfig I get >> >> fdt list /reserved-memory/secmon@10000000 >> reserved-memory { >> secmon@10000000 { >> reg = <0x00000000 0x10000000 0x00000000 0x00200000>; >> no-map; >> }; >> }; >> >> => load mmc 0:1 0x10000000 dtb >> 22925 bytes read in 8 ms (2.7 MiB/s) >> >> So now I have successfully overwritten the secure monitor. Urrgh. >> >> As you have observed load is still writing into a memory area that is >> reserved by the device-tree. >> >> Please, iterate over the device tree to ensure that nothing is loaded >> into a reserved memory area. Do not expect board files to do anything >> but create the reserve-memory entry in the device tree. > > First of all, thanks for testing. I must admit I haven't tested this > case, I just had the impression the existing function > 'boot_fdt_add_mem_rsv_regions()' (in image-fdt.c) would do that. I'll > have to dig into why it doesn't. > > Or do you have CONFIG_OF_LIBFDT disabled? `make odroid-c2_defconfig` sets CONFIG_OF_LIBFDT=y CONFIG_CMD_FDT depends on it. So without I would not have had the fdt command used above. The device-tree I was looking at was the one provided by U-Boot at ${fdtcontroladdr}. We should also think about making this testable. I would be happy if we had a test on a QEMU board. Best regards Heinrich > > In any case, it *was* my intention to include the dts memory > reservation! I'll make sure I test that for a v2. > > Simon >
Am Di., 13. Nov. 2018, 22:37 hat Heinrich Schuchardt <xypron.glpk@gmx.de> geschrieben: > On 11/13/18 9:01 PM, Simon Goldschmidt wrote: > > On 13.11.2018 20:42, Heinrich Schuchardt wrote: > >> On 11/13/18 6:47 AM, Simon Goldschmidt wrote: > >>> On Tue, Nov 13, 2018 at 3:23 AM Fabio Estevam <festevam@gmail.com> > >>> wrote: > >>>> Hi Simon, > >>>> > >>>> On Mon, Nov 12, 2018 at 7:25 PM Simon Goldschmidt > >>>> <simon.k.r.goldschmidt@gmail.com> wrote: > >>>> > >>>>> diff --git a/fs/fs.c b/fs/fs.c > >>>>> index adae98d021..4baf6b1c39 100644 > >>>>> --- a/fs/fs.c > >>>>> +++ b/fs/fs.c > >>>>> @@ -428,13 +428,57 @@ int fs_size(const char *filename, loff_t *size) > >>>>> return ret; > >>>>> } > >>>>> > >>>>> -int fs_read(const char *filename, ulong addr, loff_t offset, > >>>>> loff_t len, > >>>>> - loff_t *actread) > >>>>> +#ifdef CONFIG_LMB > >>>> Unrelated to your series, but I was wondering if we could get rid of > >>>> the CONFIG_LMB option. > >>>> > >>>> As far as I can see all the architectures define it, the only > >>>> exception being arch/sh. > >>>> > >>>> If you agree I can send a patch after your series gets applied that > >>>> removes CONFIG_LMB. > >>> Sure, that would clean things up. > >>> > >>> Simon > >>> > >> NAK > >> > >> This patch-series does not provide what is needed. With > >> odroid-c2_defconfig I get > >> > >> fdt list /reserved-memory/secmon@10000000 > >> reserved-memory { > >> secmon@10000000 { > >> reg = <0x00000000 0x10000000 0x00000000 0x00200000>; > >> no-map; > >> }; > >> }; > >> > >> => load mmc 0:1 0x10000000 dtb > >> 22925 bytes read in 8 ms (2.7 MiB/s) > >> > >> So now I have successfully overwritten the secure monitor. Urrgh. > >> > >> As you have observed load is still writing into a memory area that is > >> reserved by the device-tree. > >> > >> Please, iterate over the device tree to ensure that nothing is loaded > >> into a reserved memory area. Do not expect board files to do anything > >> but create the reserve-memory entry in the device tree. > > > > First of all, thanks for testing. I must admit I haven't tested this > > case, I just had the impression the existing function > > 'boot_fdt_add_mem_rsv_regions()' (in image-fdt.c) would do that. I'll > > have to dig into why it doesn't. > > > > Or do you have CONFIG_OF_LIBFDT disabled? > > `make odroid-c2_defconfig` sets > CONFIG_OF_LIBFDT=y > > CONFIG_CMD_FDT depends on it. So without I would not have had the fdt > command used above. > > The device-tree I was looking at was the one provided by U-Boot at > ${fdtcontroladdr}. > That might be an explanation. I used 'gd->fdt_blob' only in my patch. Are there any more device tree locations to care about? We should also think about making this testable. I would be happy if we > had a test on a QEMU board. > I tried to build the tests but I only got build errors. Is there any documentation about what I could be missing? I think my Ubuntu should be up to date, so maybe I am missing some dependencies...? Simon > Best regards > > Heinrich > > > > > In any case, it *was* my intention to include the dts memory > > reservation! I'll make sure I test that for a v2. > > > > Simon > > > >
On 11/13/18 10:47 PM, Simon Goldschmidt wrote: > > > Am Di., 13. Nov. 2018, 22:37 hat Heinrich Schuchardt <xypron.glpk@gmx.de > <mailto:xypron.glpk@gmx.de>> geschrieben: > > On 11/13/18 9:01 PM, Simon Goldschmidt wrote: > > On 13.11.2018 20:42, Heinrich Schuchardt wrote: > >> On 11/13/18 6:47 AM, Simon Goldschmidt wrote: > >>> On Tue, Nov 13, 2018 at 3:23 AM Fabio Estevam > <festevam@gmail.com <mailto:festevam@gmail.com>> > >>> wrote: > >>>> Hi Simon, > >>>> > >>>> On Mon, Nov 12, 2018 at 7:25 PM Simon Goldschmidt > >>>> <simon.k.r.goldschmidt@gmail.com > <mailto:simon.k.r.goldschmidt@gmail.com>> wrote: > >>>> > >>>>> diff --git a/fs/fs.c b/fs/fs.c > >>>>> index adae98d021..4baf6b1c39 100644 > >>>>> --- a/fs/fs.c > >>>>> +++ b/fs/fs.c > >>>>> @@ -428,13 +428,57 @@ int fs_size(const char *filename, loff_t > *size) > >>>>> return ret; > >>>>> } > >>>>> > >>>>> -int fs_read(const char *filename, ulong addr, loff_t offset, > >>>>> loff_t len, > >>>>> - loff_t *actread) > >>>>> +#ifdef CONFIG_LMB > >>>> Unrelated to your series, but I was wondering if we could get > rid of > >>>> the CONFIG_LMB option. > >>>> > >>>> As far as I can see all the architectures define it, the only > >>>> exception being arch/sh. > >>>> > >>>> If you agree I can send a patch after your series gets applied that > >>>> removes CONFIG_LMB. > >>> Sure, that would clean things up. > >>> > >>> Simon > >>> > >> NAK > >> > >> This patch-series does not provide what is needed. With > >> odroid-c2_defconfig I get > >> > >> fdt list /reserved-memory/secmon@10000000 > >> reserved-memory { > >> secmon@10000000 { > >> reg = <0x00000000 0x10000000 0x00000000 0x00200000>; > >> no-map; > >> }; > >> }; > >> > >> => load mmc 0:1 0x10000000 dtb > >> 22925 bytes read in 8 ms (2.7 MiB/s) > >> > >> So now I have successfully overwritten the secure monitor. Urrgh. > >> > >> As you have observed load is still writing into a memory area that is > >> reserved by the device-tree. > >> > >> Please, iterate over the device tree to ensure that nothing is loaded > >> into a reserved memory area. Do not expect board files to do anything > >> but create the reserve-memory entry in the device tree. > > > > First of all, thanks for testing. I must admit I haven't tested this > > case, I just had the impression the existing function > > 'boot_fdt_add_mem_rsv_regions()' (in image-fdt.c) would do that. I'll > > have to dig into why it doesn't. > > > > Or do you have CONFIG_OF_LIBFDT disabled? > > `make odroid-c2_defconfig` sets > CONFIG_OF_LIBFDT=y > > CONFIG_CMD_FDT depends on it. So without I would not have had the fdt > command used above. > > The device-tree I was looking at was the one provided by U-Boot at > ${fdtcontroladdr}. > > > That might be an explanation. I used 'gd->fdt_blob' only in my patch. For the Odroid C2 the relevant memory reservations are created in arch/arm/mach-meson/board.c:61: void meson_gx_init_reserved_memory(void *fdt) According to /README ${fdtcontroladdr} and gd->fdt_blob should be the same: lib/fdtdec.c:1255: gd->fdt_blob = (void *)env_get_ulong("fdtcontroladdr", 16, (uintptr_t)gd->fdt_blob); The boards with CONFIG_OF_PRIOR_STAGE=y set fdtcontroladdr in the board file (board/broadcom/bcmstb/bcmstb.c). You should use gd->fdt_blob. Just make sure it is already set. Best regards Heinrich > Are there any more device tree locations to care about? > > We should also think about making this testable. I would be happy if we > had a test on a QEMU board. > > > I tried to build the tests but I only got build errors. Is there any > documentation about what I could be missing? I think my Ubuntu should be > up to date, so maybe I am missing some dependencies...? > > Simon > > > Best regards > > Heinrich > > > > > In any case, it *was* my intention to include the dts memory > > reservation! I'll make sure I test that for a v2. > > > > Simon > > >
On 14.11.2018 00:03, Heinrich Schuchardt wrote: > On 11/13/18 10:47 PM, Simon Goldschmidt wrote: >> >> Am Di., 13. Nov. 2018, 22:37 hat Heinrich Schuchardt <xypron.glpk@gmx.de >> <mailto:xypron.glpk@gmx.de>> geschrieben: >> >> On 11/13/18 9:01 PM, Simon Goldschmidt wrote: >> > On 13.11.2018 20:42, Heinrich Schuchardt wrote: >> >> On 11/13/18 6:47 AM, Simon Goldschmidt wrote: >> >>> On Tue, Nov 13, 2018 at 3:23 AM Fabio Estevam >> <festevam@gmail.com <mailto:festevam@gmail.com>> >> >>> wrote: >> >>>> Hi Simon, >> >>>> >> >>>> On Mon, Nov 12, 2018 at 7:25 PM Simon Goldschmidt >> >>>> <simon.k.r.goldschmidt@gmail.com >> <mailto:simon.k.r.goldschmidt@gmail.com>> wrote: >> >>>> >> >>>>> diff --git a/fs/fs.c b/fs/fs.c >> >>>>> index adae98d021..4baf6b1c39 100644 >> >>>>> --- a/fs/fs.c >> >>>>> +++ b/fs/fs.c >> >>>>> @@ -428,13 +428,57 @@ int fs_size(const char *filename, loff_t >> *size) >> >>>>> return ret; >> >>>>> } >> >>>>> >> >>>>> -int fs_read(const char *filename, ulong addr, loff_t offset, >> >>>>> loff_t len, >> >>>>> - loff_t *actread) >> >>>>> +#ifdef CONFIG_LMB >> >>>> Unrelated to your series, but I was wondering if we could get >> rid of >> >>>> the CONFIG_LMB option. >> >>>> >> >>>> As far as I can see all the architectures define it, the only >> >>>> exception being arch/sh. >> >>>> >> >>>> If you agree I can send a patch after your series gets applied that >> >>>> removes CONFIG_LMB. >> >>> Sure, that would clean things up. >> >>> >> >>> Simon >> >>> >> >> NAK >> >> >> >> This patch-series does not provide what is needed. With >> >> odroid-c2_defconfig I get >> >> >> >> fdt list /reserved-memory/secmon@10000000 >> >> reserved-memory { >> >> secmon@10000000 { >> >> reg = <0x00000000 0x10000000 0x00000000 0x00200000>; >> >> no-map; >> >> }; >> >> }; >> >> >> >> => load mmc 0:1 0x10000000 dtb >> >> 22925 bytes read in 8 ms (2.7 MiB/s) >> >> >> >> So now I have successfully overwritten the secure monitor. Urrgh. >> >> >> >> As you have observed load is still writing into a memory area that is >> >> reserved by the device-tree. >> >> >> >> Please, iterate over the device tree to ensure that nothing is loaded >> >> into a reserved memory area. Do not expect board files to do anything >> >> but create the reserve-memory entry in the device tree. >> > >> > First of all, thanks for testing. I must admit I haven't tested this >> > case, I just had the impression the existing function >> > 'boot_fdt_add_mem_rsv_regions()' (in image-fdt.c) would do that. I'll >> > have to dig into why it doesn't. >> > >> > Or do you have CONFIG_OF_LIBFDT disabled? >> >> `make odroid-c2_defconfig` sets >> CONFIG_OF_LIBFDT=y >> >> CONFIG_CMD_FDT depends on it. So without I would not have had the fdt >> command used above. >> >> The device-tree I was looking at was the one provided by U-Boot at >> ${fdtcontroladdr}. >> >> >> That might be an explanation. I used 'gd->fdt_blob' only in my patch. > For the Odroid C2 the relevant memory reservations are created in > arch/arm/mach-meson/board.c:61: > void meson_gx_init_reserved_memory(void *fdt) > > According to /README ${fdtcontroladdr} and gd->fdt_blob should be the same: > > lib/fdtdec.c:1255: > gd->fdt_blob = (void *)env_get_ulong("fdtcontroladdr", 16, > (uintptr_t)gd->fdt_blob); > > The boards with CONFIG_OF_PRIOR_STAGE=y set fdtcontroladdr in the board > file (board/broadcom/bcmstb/bcmstb.c). > > You should use gd->fdt_blob. Just make sure it is already set. OK, so it seems U-Boot just cannot handle the "reserved-memory" node with its subnodes but only the "memreserve" thing on top level? I have the rest of the patches in a state I would submit, but I'll need some more time to dig into the dts reserved memory reservation... Simon > > Best regards > > Heinrich > >> Are there any more device tree locations to care about? >> >> We should also think about making this testable. I would be happy if we >> had a test on a QEMU board. >> >> >> I tried to build the tests but I only got build errors. Is there any >> documentation about what I could be missing? I think my Ubuntu should be >> up to date, so maybe I am missing some dependencies...? >> >> Simon >> >> >> Best regards >> >> Heinrich >> >> > >> > In any case, it *was* my intention to include the dts memory >> > reservation! I'll make sure I test that for a v2. >> > >> > Simon >> > >>
On 11/16/18 7:48 AM, Simon Goldschmidt wrote: > On 14.11.2018 00:03, Heinrich Schuchardt wrote: >> On 11/13/18 10:47 PM, Simon Goldschmidt wrote: >>> >>> Am Di., 13. Nov. 2018, 22:37 hat Heinrich Schuchardt <xypron.glpk@gmx.de >>> <mailto:xypron.glpk@gmx.de>> geschrieben: >>> >>> On 11/13/18 9:01 PM, Simon Goldschmidt wrote: >>> > On 13.11.2018 20:42, Heinrich Schuchardt wrote: >>> >> On 11/13/18 6:47 AM, Simon Goldschmidt wrote: >>> >>> On Tue, Nov 13, 2018 at 3:23 AM Fabio Estevam >>> <festevam@gmail.com <mailto:festevam@gmail.com>> >>> >>> wrote: >>> >>>> Hi Simon, >>> >>>> >>> >>>> On Mon, Nov 12, 2018 at 7:25 PM Simon Goldschmidt >>> >>>> <simon.k.r.goldschmidt@gmail.com >>> <mailto:simon.k.r.goldschmidt@gmail.com>> wrote: >>> >>>> >>> >>>>> diff --git a/fs/fs.c b/fs/fs.c >>> >>>>> index adae98d021..4baf6b1c39 100644 >>> >>>>> --- a/fs/fs.c >>> >>>>> +++ b/fs/fs.c >>> >>>>> @@ -428,13 +428,57 @@ int fs_size(const char *filename, >>> loff_t >>> *size) >>> >>>>> return ret; >>> >>>>> } >>> >>>>> >>> >>>>> -int fs_read(const char *filename, ulong addr, loff_t offset, >>> >>>>> loff_t len, >>> >>>>> - loff_t *actread) >>> >>>>> +#ifdef CONFIG_LMB >>> >>>> Unrelated to your series, but I was wondering if we could get >>> rid of >>> >>>> the CONFIG_LMB option. >>> >>>> >>> >>>> As far as I can see all the architectures define it, the only >>> >>>> exception being arch/sh. >>> >>>> >>> >>>> If you agree I can send a patch after your series gets >>> applied that >>> >>>> removes CONFIG_LMB. >>> >>> Sure, that would clean things up. >>> >>> >>> >>> Simon >>> >>> >>> >> NAK >>> >> >>> >> This patch-series does not provide what is needed. With >>> >> odroid-c2_defconfig I get >>> >> >>> >> fdt list /reserved-memory/secmon@10000000 >>> >> reserved-memory { >>> >> secmon@10000000 { >>> >> reg = <0x00000000 0x10000000 0x00000000 >>> 0x00200000>; >>> >> no-map; >>> >> }; >>> >> }; >>> >> >>> >> => load mmc 0:1 0x10000000 dtb >>> >> 22925 bytes read in 8 ms (2.7 MiB/s) >>> >> >>> >> So now I have successfully overwritten the secure monitor. >>> Urrgh. >>> >> >>> >> As you have observed load is still writing into a memory area >>> that is >>> >> reserved by the device-tree. >>> >> >>> >> Please, iterate over the device tree to ensure that nothing >>> is loaded >>> >> into a reserved memory area. Do not expect board files to do >>> anything >>> >> but create the reserve-memory entry in the device tree. >>> > >>> > First of all, thanks for testing. I must admit I haven't >>> tested this >>> > case, I just had the impression the existing function >>> > 'boot_fdt_add_mem_rsv_regions()' (in image-fdt.c) would do >>> that. I'll >>> > have to dig into why it doesn't. >>> > >>> > Or do you have CONFIG_OF_LIBFDT disabled? >>> >>> `make odroid-c2_defconfig` sets >>> CONFIG_OF_LIBFDT=y >>> >>> CONFIG_CMD_FDT depends on it. So without I would not have had >>> the fdt >>> command used above. >>> >>> The device-tree I was looking at was the one provided by U-Boot at >>> ${fdtcontroladdr}. >>> >>> >>> That might be an explanation. I used 'gd->fdt_blob' only in my patch. >> For the Odroid C2 the relevant memory reservations are created in >> arch/arm/mach-meson/board.c:61: >> void meson_gx_init_reserved_memory(void *fdt) >> >> According to /README ${fdtcontroladdr} and gd->fdt_blob should be the >> same: >> >> lib/fdtdec.c:1255: >> gd->fdt_blob = (void *)env_get_ulong("fdtcontroladdr", 16, >> (uintptr_t)gd->fdt_blob); >> >> The boards with CONFIG_OF_PRIOR_STAGE=y set fdtcontroladdr in the board >> file (board/broadcom/bcmstb/bcmstb.c). >> >> You should use gd->fdt_blob. Just make sure it is already set. > > OK, so it seems U-Boot just cannot handle the "reserved-memory" node > with its subnodes but only the "memreserve" thing on top level? > I have the rest of the patches in a state I would submit, but I'll need > some more time to dig into the dts reserved memory reservation... > > Simon > Linux Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt defines /reserved-memory. Documentation/arm64/booting.txt defines /memreserve The device tree specification v0.2 fails to provide the label used for memory reservations. https://patchwork.kernel.org/patch/5143721/ sheds some light: "Device tree regions described by /memreserve/ entries are not available in /proc/device-tree, and hence are not available to user space utilities that use /proc/device-tree. In order for these regions to be available, convert any arm64 DTS files using /memreserve/ entries to use reserved-memory nodes." So /memreserve and /reserved-memory have different semantics but neither region should be used by the load command. Best regards Heinrich >> >> Best regards >> >> Heinrich >> >>> Are there any more device tree locations to care about? >>> >>> We should also think about making this testable. I would be >>> happy if we >>> had a test on a QEMU board. >>> >>> >>> I tried to build the tests but I only got build errors. Is there any >>> documentation about what I could be missing? I think my Ubuntu should be >>> up to date, so maybe I am missing some dependencies...? >>> >>> Simon >>> >>> >>> Best regards >>> >>> Heinrich >>> >>> > >>> > In any case, it *was* my intention to include the dts memory >>> > reservation! I'll make sure I test that for a v2. >>> > >>> > Simon >>> > >>> > >
diff --git a/fs/fs.c b/fs/fs.c index adae98d021..4baf6b1c39 100644 --- a/fs/fs.c +++ b/fs/fs.c @@ -428,13 +428,57 @@ int fs_size(const char *filename, loff_t *size) return ret; } -int fs_read(const char *filename, ulong addr, loff_t offset, loff_t len, - loff_t *actread) +#ifdef CONFIG_LMB +/* Check if a file may be read to the given address */ +static int fs_read_lmb_check(const char *filename, ulong addr, loff_t offset, + loff_t len, struct fstype_info *info) +{ + struct lmb lmb; + int ret; + loff_t size; + loff_t read_len; + + /* get the actual size of the file */ + ret = info->size(filename, &size); + if (ret) + return ret; + if (offset >= size) { + /* offset >= EOF, no bytes will be written */ + return 0; + } + read_len = size - offset; + + /* limit to 'len' if it is smaller */ + if (len && len < read_len) + read_len = len; + + lmb_init_and_reserve(&lmb, gd->bd->bi_dram[0].start, + gd->bd->bi_dram[0].size, (void *)gd->fdt_blob); + lmb_dump_all(&lmb); + + if (lmb_alloc_addr(&lmb, addr, read_len) == addr) + return 0; + + printf("** Reading file would overwrite reserved memory **\n"); + return -1; +} +#endif + +static int _fs_read(const char *filename, ulong addr, loff_t offset, loff_t len, + int do_lmb_check, loff_t *actread) { struct fstype_info *info = fs_get_info(fs_type); void *buf; int ret; +#ifdef CONFIG_LMB + if (do_lmb_check) { + ret = fs_read_lmb_check(filename, addr, offset, len, info); + if (ret) + return ret; + } +#endif + /* * We don't actually know how many bytes are being read, since len==0 * means read the whole file. @@ -451,6 +495,12 @@ int fs_read(const char *filename, ulong addr, loff_t offset, loff_t len, return ret; } +int fs_read(const char *filename, ulong addr, loff_t offset, loff_t len, + loff_t *actread) +{ + return _fs_read(filename, addr, offset, len, 0, actread); +} + int fs_write(const char *filename, ulong addr, loff_t offset, loff_t len, loff_t *actwrite) { @@ -621,7 +671,7 @@ int do_load(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[], pos = 0; time = get_timer(0); - ret = fs_read(filename, addr, pos, bytes, &len_read); + ret = _fs_read(filename, addr, pos, bytes, 1, &len_read); time = get_timer(time); if (ret < 0) return 1; diff --git a/include/lmb.h b/include/lmb.h index bc06f175e1..810a37d5a5 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -31,6 +31,8 @@ struct lmb { extern struct lmb lmb; extern void lmb_init(struct lmb *lmb); +extern void lmb_init_and_reserve(struct lmb *lmb, phys_addr_t base, + phys_size_t size, void *fdt_blob); extern long lmb_add(struct lmb *lmb, phys_addr_t base, phys_size_t size); extern long lmb_reserve(struct lmb *lmb, phys_addr_t base, phys_size_t size); extern phys_addr_t lmb_alloc(struct lmb *lmb, phys_size_t size, ulong align); diff --git a/lib/lmb.c b/lib/lmb.c index c3af2fd5fc..99a4aaa09e 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -104,6 +104,19 @@ void lmb_init(struct lmb *lmb) lmb->reserved.size = 0; } +/* Initialize the struct, add memory and call arch/board reserve functions */ +void lmb_init_and_reserve(struct lmb *lmb, phys_addr_t base, phys_size_t size, + void *fdt_blob) +{ + lmb_init(lmb); + lmb_add(lmb, base, size); + arch_lmb_reserve(lmb); + board_lmb_reserve(lmb); + + if (IMAGE_ENABLE_OF_LIBFDT) + boot_fdt_add_mem_rsv_regions(lmb, fdt_blob); +} + /* This routine called with relocation disabled. */ static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t size) {
This fixes CVE-2018-18440 ("insufficient boundary checks in filesystem image load") by using lmb to check the load size of a file against reserved memory addresses. Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> --- fs/fs.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++--- include/lmb.h | 2 ++ lib/lmb.c | 13 ++++++++++++ 3 files changed, 68 insertions(+), 3 deletions(-)