[PATCH] spl: fit: Prefer a malloc()'d buffer for loading images

[Tom Rini]

On Wed, Oct 21, 2020 at 06:32:58PM -0500, Alexandru Gagniuc wrote:

> Fit images were loaded to a buffer provided by spl_get_load_buffer().
> This may work when the FIT image is small and fits between the start
> of DRAM and SYS_TEXT_BASE.
> 
> One problem with this approach is that the location of the buffer may
> be manipulated by changing the 'size' field of the FIT. A maliciously
> crafted FIT image could place the buffer over executable code and be
> able to take control of SPL. This is unacceptable for secure boot of
> signed FIT images.
> 
> Another problem is with larger FIT images, usually containing one or
> more linux kernels. In such cases the buffer be be large enough so as
> to start before DRAM (Figure I). Trying to load an image in this case
> has undefined behavior.
> For example, on stm32mp1, the MMC controller hits a RX overrun error,
> and aborts loading.
>     _________________
>    |    FIT Image    |
>    |                 |
>   /===================\                        /=====================\
>   ||      DRAM       ||                        |        DRAM         |
>   ||                 ||                        |                     |
>   ||_________________||  SYS_TEXT_BASE         | ___________________ |
>   |                   |                        ||     FIT Image     ||
>   |                   |                        ||                   ||
>   | _________________ |  SYS_SPL_MALLOC_START  || _________________ ||
>   ||  malloc() data  ||                        |||  malloc() data  |||
>   ||_________________||                        |||_________________|||
>   |                   |                        ||___________________||
>   |                   |                        |                     |
> 
>         Figure I                                       Figure II
> 
> One possibility that was analyzed was to remove the negative offset,
> such that the buffer starts at SYS_TEXT_BASE. This is not a proper
> solution because on a number of platforms, the malloc buffer() is
> placed at a fixed address, usually after SYS_TEXT_BASE. A large
> enough FIT image could cause the malloc()'d data to be overwritten
> (Figure II) when loading.
> 
>           /======================\
>           |        DRAM          |
>           |                      |
>           |                      |   CONFIG_SYS_TEXT_BASE
>           |                      |
>           |                      |
>           | ____________________ |   CONFIG_SYS_SPL_MALLOC_START
>           ||   malloc() data    ||
>           ||                    ||
>           || __________________ ||
>           |||    FIT Image     |||
>           |||                  |||
>           |||                  |||
> 
>                  Figure III
> 
> The solution proposed here is to replace the ad-hoc heuristics of
> spl_get_load_buffer() with malloc(). This provides two advantages:
>     * Bounds checking of the buffer region
>     * Guarantees the buffer does not conflict with other memory
> 
> The first problem is solved by constraining the buffer such that it
> will not overlap currently executing code. This eliminates the chance
> of a malicious FIT being able to replace the executing SPL code prior
> to signature checking.
> 
> The second problem is solved in conjunction with increasing
> CONFIG_SYS_SPL_MALLOC_SIZE. Since the SPL malloc() region is
> carefully crafted on a per-platform basis, the chances of memory
> conflicts are virtually eliminated.
> 
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me at gmail.com>

This seems like a good idea to me.

Applied to u-boot/next, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201208/178afd0e/attachment.sig>