On 09.11.2018 11:24, Simon Goldschmidt wrote:
On Fri, Nov 9, 2018 at 10:46 AM Andrea Barisani
<[email protected]> wrote:
On Fri, Nov 09, 2018 at 07:11:36AM +0100, Simon Goldschmidt wrote:
On Fri, Nov 9, 2018 at 1:37 AM Fabio Estevam <[email protected]> wrote:
Hi Andrea,
On Tue, Nov 6, 2018 at 12:57 PM Andrea Barisani
<[email protected]> wrote:
# load large file
=> ext2load mmc 0 0x60000000 fitimage.itb
Does this change work for you?
http://dark-code.bulix.org/u6gw3b-499924
My understanding was U-Boot text or stack could get overwritten which
leads to the loaded bytes being executed as code.
So you would have to check that the loaded range is within ram but not
within that reserved range of code or stack (or heap).
Exactly, merely checking RAM size is not sufficient. The specific memory
layout would need to be accounted for which means understanding where the
stack and heap are located, their direction of growth and to ensure that the
loaded payload can never overwrite them along with all other U-Boot data
segments.
This is not easy given that the stack and heap size I think can only be
guessed and not precisely limited, additionally board configurations have the
ability to set arbitrary stack, relocation and load addresses which
complicates things even further in understanding exactly how the memory
layout is set.
It's not easy, but in my opinion, it should already be solved by the
code in 'boot_start_lmb' mentioned in my last mail.
This function includes arch and board callbacks that should be able to
return a safe memory range.
I have a patched version running here with the above mentioned changes
that should fix this issue. A few cleanups are missing before sending a
patch though (changes are in fs.c and lmb.c only).
Simon
The only thing that cannot be controlled here is stack size, that's
true. The ARM port tries to solve this by getting the current stack
pointer and subtracting "4K to be safe". As far as I know, there are
no methods in U-Boot currently to ensure this is safe, though. And
depending on the RAM size, we could just subtract more. Personally, I
wouldn't mind subtracting some MBytes on my board. Actually using such
a stack would definively be another bug that needs fixing.
But it seems a good start to use these functions to limit loading from fs, too.
Simon
Getting this reserved range is what 'boot_start_lmb' does (in
bootm.c). Maybe this code can be refactored and reused in fs.c to get
a valid range for loading?
Additionally, your patch checks the loaded file's size without taking
the load address into account. So unless I read that wrong, your check
is only valid for 'addr == 0'.
Plus, the 'bytes' parameter should probably be a restriction to the
file's size when checking for a valid load range.
Simon
--
Andrea Barisani Head of Hardware Security | F-Secure
Founder | Inverse Path
https://www.f-secure.com https://inversepath.com
0x864C9B9E 0A76 074A 02CD E989 CE7F AC3F DA47 578E 864C 9B9E
"Pluralitas non est ponenda sine necessitate"
_______________________________________________
U-Boot mailing list
[email protected]
https://lists.denx.de/listinfo/u-boot
