From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Goldschmidt Date: Sat, 26 Jan 2019 22:20:16 +0100 Subject: [U-Boot] [U-Boot, v10, 09/10] tftp: prevent overwriting reserved memory In-Reply-To: <7f7f0658-2a41-8113-9fdd-c5128b21f6aa@gmx.de> References: <20190114213823.32486-10-simon.k.r.goldschmidt@gmail.com> <166124ca-e92c-56fc-8b97-6661901be8d9@gmail.com> <7f7f0658-2a41-8113-9fdd-c5128b21f6aa@gmx.de> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de 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 >>>> Acked-by: Joe Hershberger >>>> --- >>> >>> 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