+Tom Rini too
On Wed, 21 Oct 2020 at 17:33, Alexandru Gagniuc <mr.nuke...@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...@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
>
