| Message ID | 20190114213823.32486-10-simon.k.r.goldschmidt@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Fix CVE-2018-18440 and CVE-2018-18439 | expand |
On Mon, Jan 14, 2019 at 10:38:22PM +0100, Simon Goldschmidt wrote: > This fixes CVE-2018-18439 ("insufficient boundary checks in network > image boot") by using lmb to check for a valid range to store > received blocks. > > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> > Acked-by: Joe Hershberger <joe.hershberger@ni.com> With some lib/Makefile tweaks for the odd SPL+network use cases: Applied to u-boot/master, thanks!
TheOn 1/14/19 10:38 PM, Simon Goldschmidt wrote: > This fixes CVE-2018-18439 ("insufficient boundary checks in network > image boot") by using lmb to check for a valid range to store > received blocks. > > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> > Acked-by: Joe Hershberger <joe.hershberger@ni.com> > --- Hello Simon, due to this patch merged as a156c47e39ad7d00 on vexpress_ca15_tc2_defconfig the command 'dhcp filename' always fails. It was working in v2019.01 Same is true for other platforms, e.g. vexpress_ca9x4_defconfig. I put in an extra printf() and got: TFTP error: trying to overwrite reserved memory... storeaddr 0, tftp_load_addr 0, tftp_load_size 0 It is not even possible to disable the checks by undefining CONFIG_LMB because a compile error arises without CONFIG_LMB: cmd/bootz.c:48:21: error: ‘bootm_headers_t’ {aka ‘struct bootm_headers’} has no member named ‘lmb’ I think the code should compile if CONFIG_LMB is undefined. Further for all boards 'dhcp filename' should be working after your patch series if it was working before the patch series. Why is CONFIG_LMB hard coded? Shouldn't we try to avoid any new hard coded CONFIG symbols? Consider moving it to Kconfig. The logic you use in tftp_init_load_addr() is problematic: Essentially it allows loading via tftp only in a single region within the first DRAM bank. Why shouldn't I load to the second DRAM bank? Even in a single DRAM bank we will have several reserved regions and in between them several allowable regions for loading. The LMB tests do not even find all reserved regions. E.g. on x86_64 it allows loading to 0x1000000 though this address is used as a reserved region for PCI, loading to which leads to a crash. @Tom This LMB patch series stops us from straightening out the Python tests for tftp to make efi-next build without Travis CI error. Please, advise how to proceed. Best regards Heinrich
Am 26.01.2019 um 04:20 schrieb Heinrich Schuchardt: > TheOn 1/14/19 10:38 PM, Simon Goldschmidt wrote: >> This fixes CVE-2018-18439 ("insufficient boundary checks in network >> image boot") by using lmb to check for a valid range to store >> received blocks. >> >> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> >> Acked-by: Joe Hershberger <joe.hershberger@ni.com> >> --- > > Hello Simon, > > due to this patch merged as a156c47e39ad7d00 on > vexpress_ca15_tc2_defconfig the command 'dhcp filename' always fails. It > was working in v2019.01 > > Same is true for other platforms, e.g. vexpress_ca9x4_defconfig. OK, that's probably not expected ;-) I'd appreciate it if you could continue to track this down to get it fixed. > > I put in an extra printf() and got: > TFTP error: trying to overwrite reserved memory... > storeaddr 0, tftp_load_addr 0, tftp_load_size 0 I don't know the first. The latter 2 are not initialized yet in this error path and so are expected to be zero here. Could you run that test again if I sent you a patch enabling required output for me to debug this? > > It is not even possible to disable the checks by undefining CONFIG_LMB > because a compile error arises without CONFIG_LMB: > > cmd/bootz.c:48:21: error: ‘bootm_headers_t’ {aka ‘struct bootm_headers’} > has no member named ‘lmb’ > > I think the code should compile if CONFIG_LMB is undefined. You're right, it should compile without CONFIG_LMB. It did initially, so I guess that got lost somewhere during all the versions until v10, sorry. I'll work on that. > > Further for all boards 'dhcp filename' should be working after your > patch series if it was working before the patch series. Well, I wouldn't say it like that. This new code is required from a security point of view. There might be boards violating these requirements, I can't tell. But it's true that until your ${loadaddr} is not completely bogus, 'dhcp filename' should continue to work, yes. If not, let's work on this. > > Why is CONFIG_LMB hard coded? Shouldn't we try to avoid any new hard > coded CONFIG symbols? Consider moving it to Kconfig. Ehrm, sorry, I can't follow you. Which new config symbols are you talking about? CONFIG_LMB in ARM's config.h is more than 8 years old! > > The logic you use in tftp_init_load_addr() is problematic: > > Essentially it allows loading via tftp only in a single region within > the first DRAM bank. Why shouldn't I load to the second DRAM bank? > > Even in a single DRAM bank we will have several reserved regions and in > between them several allowable regions for loading. What leads you to think it's only a single region? Multiple reserved regions should work and the 'holes' in between should be valid tftp targets. This is tested in the unit tests. You're right that currently only the first DRAM bank works. Let me work on that... > > The LMB tests do not even find all reserved regions. E.g. on x86_64 it > allows loading to 0x1000000 though this address is used as a reserved > region for PCI, loading to which leads to a crash. LMB is a long established concept for U-Boot loading boot files. I added using it to the 'load' and 'tftp' commands. If x86_64 fails to reserve memory for LMB, I think x86_64 should be fixed to do so (e.g. via 'arch_lmb_reserve'). > > @Tom > This LMB patch series stops us from straightening out the Python tests > for tftp to make efi-next build without Travis CI error. Please, advise > how to proceed. My idea of how to proceed would be: let's just sort out these issues as fast as possible. I'll send you a patch to debug why tftp thinks it would overwrite reserved memory. Also, I'll fix the compile error with CONFIG_LMB disabled and I'll try to add DRAM banks other than the first. Regards, Simon
On 1/26/19 9:46 AM, Simon Goldschmidt wrote: > Am 26.01.2019 um 04:20 schrieb Heinrich Schuchardt: >> TheOn 1/14/19 10:38 PM, Simon Goldschmidt wrote: >>> This fixes CVE-2018-18439 ("insufficient boundary checks in network >>> image boot") by using lmb to check for a valid range to store >>> received blocks. >>> >>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> >>> Acked-by: Joe Hershberger <joe.hershberger@ni.com> >>> --- >> >> Hello Simon, >> >> due to this patch merged as a156c47e39ad7d00 on >> vexpress_ca15_tc2_defconfig the command 'dhcp filename' always fails. It >> was working in v2019.01 >> >> Same is true for other platforms, e.g. vexpress_ca9x4_defconfig. > > OK, that's probably not expected ;-) > > I'd appreciate it if you could continue to track this down to get it fixed. Let's see how far I get. You can easily test yourself with QEMU. I was using: QEMU_AUDIO_DRV=none qemu-system-arm \ -M vexpress-a15 -cpu cortex-a15 -kernel u-boot \ -netdev \ user,id=net0,tftp=tftp,net=192.168.76.0/24,dhcpstart=192.168.76.9 \ -net nic,model=lan9118,netdev=net0 \ -m 1024M --nographic \ -drive if=sd,file=img.vexpress,media=disk,format=raw > >> >> I put in an extra printf() and got: >> TFTP error: trying to overwrite reserved memory... >> storeaddr 0, tftp_load_addr 0, tftp_load_size 0 > > I don't know the first. The latter 2 are not initialized yet in this > error path and so are expected to be zero here. > > Could you run that test again if I sent you a patch enabling required > output for me to debug this? Sure. > >> >> It is not even possible to disable the checks by undefining CONFIG_LMB >> because a compile error arises without CONFIG_LMB: >> >> cmd/bootz.c:48:21: error: ‘bootm_headers_t’ {aka ‘struct bootm_headers’} >> has no member named ‘lmb’ >> >> I think the code should compile if CONFIG_LMB is undefined. > > You're right, it should compile without CONFIG_LMB. It did initially, so > I guess that got lost somewhere during all the versions until v10, > sorry. I'll work on that. > >> >> Further for all boards 'dhcp filename' should be working after your >> patch series if it was working before the patch series. > > Well, I wouldn't say it like that. This new code is required from a > security point of view. There might be boards violating these > requirements, I can't tell. But it's true that until your ${loadaddr} is > not completely bogus, 'dhcp filename' should continue to work, yes. If > not, let's work on this. I think we are on the same line. > >> >> Why is CONFIG_LMB hard coded? Shouldn't we try to avoid any new hard >> coded CONFIG symbols? Consider moving it to Kconfig. > > Ehrm, sorry, I can't follow you. Which new config symbols are you > talking about? CONFIG_LMB in ARM's config.h is more than 8 years old! Sorry, I did not check this. So you didn't put in a new switch. > >> >> The logic you use in tftp_init_load_addr() is problematic: >> >> Essentially it allows loading via tftp only in a single region within >> the first DRAM bank. Why shouldn't I load to the second DRAM bank? >> >> Even in a single DRAM bank we will have several reserved regions and in >> between them several allowable regions for loading. > > What leads you to think it's only a single region? Multiple reserved > regions should work and the 'holes' in between should be valid tftp > targets. This is tested in the unit tests. I did not see that load_addr is a global set in cmd/net.c based on the parameter passed to the tftp command. > > You're right that currently only the first DRAM bank works. Let me work > on that... > >> >> The LMB tests do not even find all reserved regions. E.g. on x86_64 it >> allows loading to 0x1000000 though this address is used as a reserved >> region for PCI, loading to which leads to a crash. > > LMB is a long established concept for U-Boot loading boot files. I added > using it to the 'load' and 'tftp' commands. If x86_64 fails to reserve > memory for LMB, I think x86_64 should be fixed to do so (e.g. via > 'arch_lmb_reserve'). > >> >> @Tom >> This LMB patch series stops us from straightening out the Python tests >> for tftp to make efi-next build without Travis CI error. Please, advise >> how to proceed. > > My idea of how to proceed would be: let's just sort out these issues as > fast as possible. I'll send you a patch to debug why tftp thinks it > would overwrite reserved memory. Also, I'll fix the compile error with > CONFIG_LMB disabled and I'll try to add DRAM banks other than the first. > > Regards, > Simon > Best regards Heinrich
On Sat, Jan 26, 2019 at 09:46:35AM +0100, Simon Goldschmidt wrote: > Am 26.01.2019 um 04:20 schrieb Heinrich Schuchardt: > >TheOn 1/14/19 10:38 PM, Simon Goldschmidt wrote: > >>This fixes CVE-2018-18439 ("insufficient boundary checks in network > >>image boot") by using lmb to check for a valid range to store > >>received blocks. > >> > >>Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> > >>Acked-by: Joe Hershberger <joe.hershberger@ni.com> > >>--- > > > >Hello Simon, > > > >due to this patch merged as a156c47e39ad7d00 on > >vexpress_ca15_tc2_defconfig the command 'dhcp filename' always fails. It > >was working in v2019.01 > > > >Same is true for other platforms, e.g. vexpress_ca9x4_defconfig. > > OK, that's probably not expected ;-) > > I'd appreciate it if you could continue to track this down to get it fixed. > > > > >I put in an extra printf() and got: > >TFTP error: trying to overwrite reserved memory... > >storeaddr 0, tftp_load_addr 0, tftp_load_size 0 > > I don't know the first. The latter 2 are not initialized yet in this error > path and so are expected to be zero here. > > Could you run that test again if I sent you a patch enabling required output > for me to debug this? > > > > >It is not even possible to disable the checks by undefining CONFIG_LMB > >because a compile error arises without CONFIG_LMB: > > > >cmd/bootz.c:48:21: error: ‘bootm_headers_t’ {aka ‘struct bootm_headers’} > >has no member named ‘lmb’ > > > >I think the code should compile if CONFIG_LMB is undefined. > > You're right, it should compile without CONFIG_LMB. It did initially, so I > guess that got lost somewhere during all the versions until v10, sorry. I'll > work on that. That might be on me. There were a few cases in the networking code where the patch broke building the existing world.
On 1/26/19 10:56 AM, Heinrich Schuchardt wrote: > On 1/26/19 9:46 AM, Simon Goldschmidt wrote: >> Am 26.01.2019 um 04:20 schrieb Heinrich Schuchardt: >>> TheOn 1/14/19 10:38 PM, Simon Goldschmidt wrote: >>>> This fixes CVE-2018-18439 ("insufficient boundary checks in network >>>> image boot") by using lmb to check for a valid range to store >>>> received blocks. >>>> >>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> >>>> Acked-by: Joe Hershberger <joe.hershberger@ni.com> >>>> --- >>> >>> Hello Simon, >>> >>> due to this patch merged as a156c47e39ad7d00 on >>> vexpress_ca15_tc2_defconfig the command 'dhcp filename' always fails. It >>> was working in v2019.01 >>> >>> Same is true for other platforms, e.g. vexpress_ca9x4_defconfig. >> >> OK, that's probably not expected ;-) >> >> I'd appreciate it if you could continue to track this down to get it fixed. > > Let's see how far I get. bdinfo shows: DRAM bank = 0x00000000 -> start = 0x80000000 -> size = 0x20000000 DRAM bank = 0x00000001 -> start = 0xa0000000 -> size = 0x20000000 printenv: loadaddr=0xa0008000 So the load address is in the second DRAM bank. I guess we need changes in the following places: t/tftp.c:609: lmb_init_and_reserve(&lmb, gd->bd->bi_dram[0].start, fs/fs.c:456: lmb_init_and_reserve(&lmb, gd->bd->bi_dram[0].start, common/bootm.c:62: lmb_init_and_reserve(&images->lmb, (phys_addr_t)mem_start, mem_size, I wonder why bootm.c is different and why isn't the fdt considered? I would suggest the following: Remove parameter lmb from lmb_get_unreserved_size(). Instead let lmb_get_unreserved_size() check if a static struct lmb in lib/lmb.c is initialized. If not use the different DRAM banks and the fdt for initialization. Best regards Heinrich
Am 26.01.2019 um 14:17 schrieb Tom Rini: > On Sat, Jan 26, 2019 at 09:46:35AM +0100, Simon Goldschmidt wrote: >> Am 26.01.2019 um 04:20 schrieb Heinrich Schuchardt: >>> TheOn 1/14/19 10:38 PM, Simon Goldschmidt wrote: >>>> This fixes CVE-2018-18439 ("insufficient boundary checks in network >>>> image boot") by using lmb to check for a valid range to store >>>> received blocks. >>>> >>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> >>>> Acked-by: Joe Hershberger <joe.hershberger@ni.com> >>>> --- >>> >>> Hello Simon, >>> >>> due to this patch merged as a156c47e39ad7d00 on >>> vexpress_ca15_tc2_defconfig the command 'dhcp filename' always fails. It >>> was working in v2019.01 >>> >>> Same is true for other platforms, e.g. vexpress_ca9x4_defconfig. >> >> OK, that's probably not expected ;-) >> >> I'd appreciate it if you could continue to track this down to get it fixed. >> >>> >>> I put in an extra printf() and got: >>> TFTP error: trying to overwrite reserved memory... >>> storeaddr 0, tftp_load_addr 0, tftp_load_size 0 >> >> I don't know the first. The latter 2 are not initialized yet in this error >> path and so are expected to be zero here. >> >> Could you run that test again if I sent you a patch enabling required output >> for me to debug this? >> >>> >>> It is not even possible to disable the checks by undefining CONFIG_LMB >>> because a compile error arises without CONFIG_LMB: >>> >>> cmd/bootz.c:48:21: error: ‘bootm_headers_t’ {aka ‘struct bootm_headers’} >>> has no member named ‘lmb’ >>> >>> I think the code should compile if CONFIG_LMB is undefined. >> >> You're right, it should compile without CONFIG_LMB. It did initially, so I >> guess that got lost somewhere during all the versions until v10, sorry. I'll >> work on that. > > That might be on me. There were a few cases in the networking code > where the patch broke building the existing world. Trying again to compile with CONFIG_LMB disabled, it didn't work at all. It failed in places none of us touched for about 8 years, so I don't think it was you. OTOH, I don't know what I had been testing to think it works with CONFIG_LMB disabled. I had to disable quite a few commands and features to keep it compiling. In the end, I think we'll have to decide if we want to make it work with CONFIG_LMB disabled or if we make this mandatory. What I did see is that some of the architectures don't overwrite 'arch_lmb_reserve' and are thus probably still affected by these CVEs... Regards, Simon
Am 26.01.2019 um 10:56 schrieb Heinrich Schuchardt: > On 1/26/19 9:46 AM, Simon Goldschmidt wrote: >> Am 26.01.2019 um 04:20 schrieb Heinrich Schuchardt: >>> TheOn 1/14/19 10:38 PM, Simon Goldschmidt wrote: >>>> This fixes CVE-2018-18439 ("insufficient boundary checks in network >>>> image boot") by using lmb to check for a valid range to store >>>> received blocks. >>>> >>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> >>>> Acked-by: Joe Hershberger <joe.hershberger@ni.com> >>>> --- >>> >>> Hello Simon, >>> >>> due to this patch merged as a156c47e39ad7d00 on >>> vexpress_ca15_tc2_defconfig the command 'dhcp filename' always fails. It >>> was working in v2019.01 >>> >>> Same is true for other platforms, e.g. vexpress_ca9x4_defconfig. >> >> OK, that's probably not expected ;-) >> >> I'd appreciate it if you could continue to track this down to get it fixed. > > Let's see how far I get. > > You can easily test yourself with QEMU. I was using: > > QEMU_AUDIO_DRV=none qemu-system-arm \ > -M vexpress-a15 -cpu cortex-a15 -kernel u-boot \ > -netdev \ > user,id=net0,tftp=tftp,net=192.168.76.0/24,dhcpstart=192.168.76.9 \ > -net nic,model=lan9118,netdev=net0 \ > -m 1024M --nographic \ > -drive if=sd,file=img.vexpress,media=disk,format=raw Yes, this worked quite well (after creating the 'img.vexpress' file, that is), and 'dhcp somefile' now works for me in that configuration. Thanks for the hint. > >> >>> >>> I put in an extra printf() and got: >>> TFTP error: trying to overwrite reserved memory... >>> storeaddr 0, tftp_load_addr 0, tftp_load_size 0 >> >> I don't know the first. The latter 2 are not initialized yet in this >> error path and so are expected to be zero here. >> >> Could you run that test again if I sent you a patch enabling required >> output for me to debug this? > > Sure. > >> >>> >>> It is not even possible to disable the checks by undefining CONFIG_LMB >>> because a compile error arises without CONFIG_LMB: >>> >>> cmd/bootz.c:48:21: error: ‘bootm_headers_t’ {aka ‘struct bootm_headers’} >>> has no member named ‘lmb’ >>> >>> I think the code should compile if CONFIG_LMB is undefined. >> >> You're right, it should compile without CONFIG_LMB. It did initially, so >> I guess that got lost somewhere during all the versions until v10, >> sorry. I'll work on that. >> >>> >>> Further for all boards 'dhcp filename' should be working after your >>> patch series if it was working before the patch series. >> >> Well, I wouldn't say it like that. This new code is required from a >> security point of view. There might be boards violating these >> requirements, I can't tell. But it's true that until your ${loadaddr} is >> not completely bogus, 'dhcp filename' should continue to work, yes. If >> not, let's work on this. > > I think we are on the same line. > >> >>> >>> Why is CONFIG_LMB hard coded? Shouldn't we try to avoid any new hard >>> coded CONFIG symbols? Consider moving it to Kconfig. >> >> Ehrm, sorry, I can't follow you. Which new config symbols are you >> talking about? CONFIG_LMB in ARM's config.h is more than 8 years old! > > Sorry, I did not check this. So you didn't put in a new switch. > >> >>> >>> The logic you use in tftp_init_load_addr() is problematic: >>> >>> Essentially it allows loading via tftp only in a single region within >>> the first DRAM bank. Why shouldn't I load to the second DRAM bank? >>> >>> Even in a single DRAM bank we will have several reserved regions and in >>> between them several allowable regions for loading. >> >> What leads you to think it's only a single region? Multiple reserved >> regions should work and the 'holes' in between should be valid tftp >> targets. This is tested in the unit tests. > > I did not see that load_addr is a global set in cmd/net.c based on the > parameter passed to the tftp command. > >> >> You're right that currently only the first DRAM bank works. Let me work >> on that... >> >>> >>> The LMB tests do not even find all reserved regions. E.g. on x86_64 it >>> allows loading to 0x1000000 though this address is used as a reserved >>> region for PCI, loading to which leads to a crash. >> >> LMB is a long established concept for U-Boot loading boot files. I added >> using it to the 'load' and 'tftp' commands. If x86_64 fails to reserve >> memory for LMB, I think x86_64 should be fixed to do so (e.g. via >> 'arch_lmb_reserve'). >> >>> >>> @Tom >>> This LMB patch series stops us from straightening out the Python tests >>> for tftp to make efi-next build without Travis CI error. Please, advise >>> how to proceed. >> >> My idea of how to proceed would be: let's just sort out these issues as >> fast as possible. I'll send you a patch to debug why tftp thinks it >> would overwrite reserved memory. Also, I'll fix the compile error with >> CONFIG_LMB disabled and I'll try to add DRAM banks other than the first. So I just sent a patch that should fix the "multiple DRAM banks" issue. I gave up compiling without CONFIG_LMB for now, I guess we need a more global decision on if we want that or not, since those compiler errors seem to be a reuslt of changes much more in the past than I thought... I hope this new patch fixes things for you. Thanks for working on this with me! Regards, Simon
diff --git a/net/tftp.c b/net/tftp.c index 68ffd81414..a9335b1b7e 100644 --- a/net/tftp.c +++ b/net/tftp.c @@ -17,6 +17,8 @@ #include <flash.h> #endif +DECLARE_GLOBAL_DATA_PTR; + /* Well known TFTP port # */ #define WELL_KNOWN_PORT 69 /* Millisecs to timeout for lost pkt */ @@ -81,6 +83,10 @@ static ulong tftp_block_wrap; /* memory offset due to wrapping */ static ulong tftp_block_wrap_offset; static int tftp_state; +static ulong tftp_load_addr; +#ifdef CONFIG_LMB +static ulong tftp_load_size; +#endif #ifdef CONFIG_TFTP_TSIZE /* The file size reported by the server */ static int tftp_tsize; @@ -164,10 +170,11 @@ static void mcast_cleanup(void) #endif /* CONFIG_MCAST_TFTP */ -static inline void store_block(int block, uchar *src, unsigned len) +static inline int store_block(int block, uchar *src, unsigned int len) { ulong offset = block * tftp_block_size + tftp_block_wrap_offset; ulong newsize = offset + len; + ulong store_addr = tftp_load_addr + offset; #ifdef CONFIG_SYS_DIRECT_FLASH_TFTP int i, rc = 0; @@ -175,24 +182,32 @@ static inline void store_block(int block, uchar *src, unsigned len) /* start address in flash? */ if (flash_info[i].flash_id == FLASH_UNKNOWN) continue; - if (load_addr + offset >= flash_info[i].start[0]) { + if (store_addr >= flash_info[i].start[0]) { rc = 1; break; } } if (rc) { /* Flash is destination for this packet */ - rc = flash_write((char *)src, (ulong)(load_addr+offset), len); + rc = flash_write((char *)src, store_addr, len); if (rc) { flash_perror(rc); - net_set_state(NETLOOP_FAIL); - return; + return rc; } } else #endif /* CONFIG_SYS_DIRECT_FLASH_TFTP */ { - void *ptr = map_sysmem(load_addr + offset, len); - + void *ptr; + +#ifdef CONFIG_LMB + if (store_addr < tftp_load_addr || + store_addr + len > tftp_load_addr + tftp_load_size) { + puts("\nTFTP error: "); + puts("trying to overwrite reserved memory...\n"); + return -1; + } +#endif + ptr = map_sysmem(store_addr, len); memcpy(ptr, src, len); unmap_sysmem(ptr); } @@ -203,6 +218,8 @@ static inline void store_block(int block, uchar *src, unsigned len) if (net_boot_file_size < newsize) net_boot_file_size = newsize; + + return 0; } /* Clear our state ready for a new transfer */ @@ -606,7 +623,11 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip, timeout_count_max = tftp_timeout_count_max; net_set_timeout_handler(timeout_ms, tftp_timeout_handler); - store_block(tftp_cur_block - 1, pkt + 2, len); + if (store_block(tftp_cur_block - 1, pkt + 2, len)) { + eth_halt(); + net_set_state(NETLOOP_FAIL); + break; + } /* * Acknowledge the block just received, which will prompt @@ -695,6 +716,25 @@ static void tftp_timeout_handler(void) } } +/* Initialize tftp_load_addr and tftp_load_size from load_addr and lmb */ +static int tftp_init_load_addr(void) +{ +#ifdef CONFIG_LMB + struct lmb lmb; + phys_size_t max_size; + + lmb_init_and_reserve(&lmb, gd->bd->bi_dram[0].start, + gd->bd->bi_dram[0].size, (void *)gd->fdt_blob); + + max_size = lmb_get_unreserved_size(&lmb, load_addr); + if (!max_size) + return -1; + + tftp_load_size = max_size; +#endif + tftp_load_addr = load_addr; + return 0; +} void tftp_start(enum proto_t protocol) { @@ -791,7 +831,14 @@ void tftp_start(enum proto_t protocol) } else #endif { - printf("Load address: 0x%lx\n", load_addr); + if (tftp_init_load_addr()) { + eth_halt(); + net_set_state(NETLOOP_FAIL); + puts("\nTFTP error: "); + puts("trying to overwrite reserved memory...\n"); + return; + } + printf("Load address: 0x%lx\n", tftp_load_addr); puts("Loading: *\b"); tftp_state = STATE_SEND_RRQ; #ifdef CONFIG_CMD_BOOTEFI @@ -842,9 +889,15 @@ void tftp_start_server(void) { tftp_filename[0] = 0; + if (tftp_init_load_addr()) { + eth_halt(); + net_set_state(NETLOOP_FAIL); + puts("\nTFTP error: trying to overwrite reserved memory...\n"); + return; + } printf("Using %s device\n", eth_get_name()); printf("Listening for TFTP transfer on %pI4\n", &net_ip); - printf("Load address: 0x%lx\n", load_addr); + printf("Load address: 0x%lx\n", tftp_load_addr); puts("Loading: *\b");
This fixes CVE-2018-18439 ("insufficient boundary checks in network image boot") by using lmb to check for a valid range to store received blocks. Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> Acked-by: Joe Hershberger <joe.hershberger@ni.com> --- Changes in v10: - add acked-by Changes in v9: None Changes in v8: None Changes in v7: - fix compiling without CONFIG_LMB Changes in v6: None Changes in v5: None Changes in v4: - this was patch 8, is now patch 7 - lines changed because v3 patch 7 got removed and MCAST_TFTP still exists Changes in v2: - this patch is new in v2 net/tftp.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 63 insertions(+), 10 deletions(-)