[U-Boot] [U-Boot, v10, 09/10] tftp: prevent overwriting reserved memory

[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 at gmail.com>
>>> Acked-by: Joe Hershberger <joe.hershberger at 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