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

[Simon Goldschmidt]

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 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

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