From: Alexandru Gagniuc Date: Wed, 21 Oct 2020 23:32:58 +0000 (-0500) Subject: spl: fit: Prefer a malloc()'d buffer for loading images X-Git-Url: http://git.dujemihanovic.xyz/?a=commitdiff_plain;h=03f1f78a9b44b5fd6fc09faf81639879d2d0f85f;p=u-boot.git spl: fit: Prefer a malloc()'d buffer for loading images 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 --- diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index 2fbee4f19f..795e2922ce 100644 --- a/common/spl/spl_fit.c +++ b/common/spl/spl_fit.c @@ -494,6 +494,23 @@ static int spl_fit_image_get_os(const void *fit, int noffset, uint8_t *os) #endif } +/* + * The purpose of the FIT load buffer is to provide a memory location that is + * independent of the load address of any FIT component. + */ +static void *spl_get_fit_load_buffer(size_t size) +{ + void *buf; + + buf = malloc(size); + if (!buf) { + pr_err("Could not get FIT buffer of %lu bytes\n", (ulong)size); + pr_err("\tcheck CONFIG_SYS_SPL_MALLOC_SIZE\n"); + buf = spl_get_load_buffer(0, size); + } + return buf; +} + /* * Weak default function to allow customizing SPL fit loading for load-only * use cases by allowing to skip the parsing/processing of the FIT contents @@ -508,12 +525,12 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, struct spl_load_info *info, ulong sector, void *fit) { int sectors; - ulong size; + ulong size, hsize; unsigned long count; struct spl_image_info image_info; int node = -1; int images, ret; - int base_offset, hsize, align_len = ARCH_DMA_MINALIGN - 1; + int base_offset; int index = 0; int firmware_node; @@ -529,24 +546,14 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, /* * So far we only have one block of data from the FIT. Read the entire - * thing, including that first block, placing it so it finishes before - * where we will load the image. - * - * Note that we will load the image such that its first byte will be - * at the load address. Since that byte may be part-way through a - * block, we may load the image up to one block before the load - * address. So take account of that here by subtracting an addition - * block length from the FIT start position. - * - * In fact the FIT has its own load address, but we assume it cannot - * be before CONFIG_SYS_TEXT_BASE. + * thing, including that first block. * * For FIT with data embedded, data is loaded as part of FIT image. * For FIT with external data, data is not loaded in this step. */ - hsize = (size + info->bl_len + align_len) & ~align_len; - fit = spl_get_load_buffer(-hsize, hsize); sectors = get_aligned_image_size(info, size, 0); + hsize = sectors * info->bl_len; + fit = spl_get_fit_load_buffer(hsize); count = info->read(info, sector, sectors, fit); debug("fit read sector %lx, sectors=%d, dst=%p, count=%lu, size=0x%lx\n", sector, sectors, fit, count, size);