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

[Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>]

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