Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nuttx flash_area_get_sectors contains possible buffer overflow #2180

Open
willdean opened this issue Jan 16, 2025 · 0 comments
Open

Nuttx flash_area_get_sectors contains possible buffer overflow #2180

willdean opened this issue Jan 16, 2025 · 0 comments

Comments

@willdean
Copy link

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant