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@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@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