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

py/stream: Check for stream read function returning too many bytes. #13572

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

projectgus
Copy link
Contributor

Closes #13046. This is two fixes:

  1. Force results of mp_vfs_blockdev_read_ext/mp_vfs_blockdev_write_ext to be 0 or a negative integer. Returning positive integers here confuses littlefs (see lfs1.c:66 for example) and led to the linked bug. In general it seems like it can lead to unexpected behaviour, in libraries not part of MicroPython.
  2. Check the result of a stream read for too many bytes read. Again, this only happens if the underlying function is malformed and returns an invalid value.

Either one of these is sufficient to fix the linked issue, but checking both seem potentially worthwhile as it's possible for other similar cases to arise (either different stream implementations that return the wrong length, or other errors in block device consumers that are returned invalid results.)

This work was funded through GitHub Sponsors.

@projectgus projectgus added the bug label Jan 31, 2024
Copy link

github-actions bot commented Jan 31, 2024

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:   +48 +0.006% standard
      stm32:   +32 +0.008% PYBV10
     mimxrt:   +40 +0.011% TEENSY40
        rp2:   +24 +0.007% RPI_PICO
       samd:   +32 +0.012% ADAFRUIT_ITSYBITSY_M4_EXPRESS

Copy link

codecov bot commented Jan 31, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 98.38%. Comparing base (d712feb) to head (d278b46).

Files Patch % Lines
py/stream.c 33.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #13572      +/-   ##
==========================================
- Coverage   98.39%   98.38%   -0.01%     
==========================================
  Files         161      161              
  Lines       21078    21085       +7     
==========================================
+ Hits        20739    20744       +5     
- Misses        339      341       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@projectgus

This comment was marked as resolved.

A positive result here can result in eventual memory corruption
as littlefs expects the result of a cache read/write function to be
0 or a negative integer for an error.

Closes micropython#13046

This work was funded through GitHub Sponsors.

Signed-off-by: Angus Gratton <angus@redyak.com.au>
This only happens if the underlying stream implementation is malformed, but
results in unsigned integer overflow and out of bounds read otherwise.

Second fix for micropython#13046 - allows for possibility an invalid result comes back
from a different stream implementation.

This work was funded through GitHub Sponsors.

Signed-off-by: Angus Gratton <angus@redyak.com.au>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

heap-buffer-overflow: from integer overflow at mp_stream_rw
1 participant