[PATCH] spl: fit: Prefer a malloc()'d buffer for loading images
Alex G.
mr.nuke.me at gmail.com
Mon Dec 7 16:17:30 CET 2020
On 11/3/20 9:11 AM, Simon Glass wrote:
> +Tom Rini too
ping?
Alex
> On Wed, 21 Oct 2020 at 17:33, Alexandru Gagniuc <mr.nuke.me at gmail.com> 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>
>> ---
>>
>> Contingency plan:
>>
>> If the malloc() fails, the backup plan is to use spl_get_load_buffer()
>> with the negative offset eliminated. This puts the buffer at
>> CONFIG_SYS_TEXT_BASE as a final measure. We can't use the negative
>> offset because that would re-introduce the first prooblem.
>>
>> Expected fallout:
>>
>> This only affects the FIT image loading path, so no breakage is
>> expected for default configurations. Most configs have a sufficiently
>> large SYS_SPL_MALLOC_SIZE, with the most common value at 1 MB. This
>> adequate for the majority of u-boot FIT images.
>>
>> No significant breakage is expected.
>>
>>
>> common/spl/spl_fit.c | 37 ++++++++++++++++++++++---------------
>> 1 file changed, 22 insertions(+), 15 deletions(-)
>>
>> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
>> index a90d821c82..98a7531f72 100644
>> --- a/common/spl/spl_fit.c
>> +++ b/common/spl/spl_fit.c
>> @@ -472,6 +472,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
>> @@ -486,12 +503,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;
>>
>> @@ -507,24 +524,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);
>> --
>> 2.26.2
>>
More information about the U-Boot
mailing list