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...@gmail.com>
This seems like a good idea to me. Applied to u-boot/next, thanks! -- Tom
signature.asc
Description: PGP signature
