mcuboot icon indicating copy to clipboard operation
mcuboot copied to clipboard

Nuttx flash_area_get_sectors contains possible buffer overflow

Open willdean opened this issue 10 months ago • 0 comments

The nuttx implementation of flash_area_get_sectors runs a loop from 0 to fa->fa_size, writing sector entries into sectors[] and incrementing a local variable total_count for the array index.

It does not, however, check that total_count does not exceed the provided *count (which has been set by the caller to BOOT_MAX_IMG_SECTORS.)

I think this means that if BOOT_MAX_IMG_SECTORS is configured too small, flash_area_get_sectors will write past the end of the sectors[] array. There may be other checks which might catch this, but they seem to be run AFTER flash_area_get_sectors, which means that the memory corruption will have already occurred.

IME it's actually pretty easy to have BOOT_MAX_IMG_SECTORS configured too low, and one expects the ""Failed reading sectors; BOOT_MAX_IMG_SECTORS=%d - too small?" error from much higher up the stack, however if a buffer's been blown before then it might not be reliable.

Because the Nuttx code is using uniform sector sizes, this error situation could be pre-calculated, but if one wanted to protect the loop structure in case of future generalisation (partly because the Nuttx code is a great general porting reference), then the loop could have something like this added:

  for (off = 0; off < fa->fa_size; off += sector_size)
    {
      /* Note: Offset here is relative to flash area, not device */

      sectors[total_count].fs_off = off;
      sectors[total_count].fs_size = sector_size;
      total_count++;
      /* New code below */
      if(total_count >= *count) 
         {
           return ERROR;
         }
    }

Happy to file a PR if that's useful (and I'm not completely off-track here).

willdean avatar Jan 16 '25 09:01 willdean