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.goldschm...@gmail.com>
Acked-by: Joe Hershberger <joe.hershber...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
