On 12.11.2018 19:03, Heinrich Schuchardt wrote:
On 11/12/18 7:56 AM, Simon Goldschmidt wrote:
On Mon, Nov 12, 2018 at 12:22 AM Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 11/11/18 3:22 PM, Wolfgang Denk wrote:
Dear Andrea,
In message 20181109094615.GC9586@lambda.inversepath.com you wrote:
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 pretty easy. On all architectures I'm aware of the stack has the lowest location in memory, and is growing downward.
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.
I think this is not that complicated. At least in standard U-Boot (not speaking for SPL) it should be sufficient to check the current stack pointer (which is easy to read) and take this a upper limit of available/allowed memory. If we add some reasonable safety margin (say, 1 MB or so) we should be really safe.
Unfortunately this does not hold true. E.g. the Odroid-C2 has the secure monitor in the middle of the RAM. You would not want to overwrite those addresses.
For a board with a device tree all reserved memory areas should be secured against overwriting.
That's why I proposed to use the already existing memory reservation scheme 'lmb' (used in loading boot images).
In your case, 'board_lmb_reserve' should make sure the secure monitor does not get overwritten. The 'arch_lmb_reserve' function for arm already ensures U-Boot text, heap and stack don't get overwritten. It could be improved to reserve +1M to the current stack pointer where it does reserve +4K now.
If board_lmb_reserve() should be the solution I would prefer that not each individual board calls board_lmb_reserve() but that some common code is used to iterate over the memory reservations in the device tree.
I know that the efi loader has its own scheme of memory reservation. I just thought it cleaner to stay with lmb for fs_load and tftp as lmb is already used in image loading/booting.
But using the memory reservations from U-Boot device tree definitively makes sense, thanks for the hint. This should probably be done for the bootm code, too...
Simon
Cheers
Heinrich
I am working on a patch for the 'load' issue (which could be reused for the tftp issue). There are some problems with the existing lmb code though, which delayed me a bit. However, given that this doesn't make it into the 2018.11 release, anyway, I figured some more days to get it cleaner won't hurt...
Simon
Best regards
Heinrich
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'.
The approach is also not appliccable to networ boot; with TFTP we don't know the image size in advance.
Eventyally the boundary checking should be done where the image content actually gets copied to memory.
Best regards,
Wolfgang Denk
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot