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

[Simon Glass]

+Tom Rini too


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
>