From d02cd5c0ad0a48e03d50d8119f629b8453e779b5 Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Wed, 31 Jan 2024 18:18:14 +1100 Subject: [PATCH 1/2] extmod/vfs_blockdev: Check block device function positive results. 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 #13046 This work was funded through GitHub Sponsors. Signed-off-by: Angus Gratton --- extmod/vfs_blockdev.c | 16 ++- tests/extmod/vfs_blockdev_invalid.py | 87 ++++++++++++++++ tests/extmod/vfs_blockdev_invalid.py.exp | 126 +++++++++++++++++++++++ 3 files changed, 227 insertions(+), 2 deletions(-) create mode 100644 tests/extmod/vfs_blockdev_invalid.py create mode 100644 tests/extmod/vfs_blockdev_invalid.py.exp diff --git a/extmod/vfs_blockdev.c b/extmod/vfs_blockdev.c index 57c83b4289..e98ed5abeb 100644 --- a/extmod/vfs_blockdev.c +++ b/extmod/vfs_blockdev.c @@ -32,6 +32,18 @@ #if MICROPY_VFS +// Block device functions are expected to return 0 on success +// and negative integer on errors. Check for positive integer +// results as some callers (i.e. littlefs) will produce corrupt +// results from these. +static int mp_vfs_check_result(mp_obj_t ret) { + int i = MP_OBJ_SMALL_INT_VALUE(ret); + if (i > 0) { + mp_raise_OSError(MP_EINVAL); + } + return i; +} + void mp_vfs_blockdev_init(mp_vfs_blockdev_t *self, mp_obj_t bdev) { mp_load_method(bdev, MP_QSTR_readblocks, self->readblocks); mp_load_method_maybe(bdev, MP_QSTR_writeblocks, self->writeblocks); @@ -69,7 +81,7 @@ int mp_vfs_blockdev_read_ext(mp_vfs_blockdev_t *self, size_t block_num, size_t b if (ret == mp_const_none) { return 0; } else { - return MP_OBJ_SMALL_INT_VALUE(ret); + return mp_vfs_check_result(ret); } } @@ -106,7 +118,7 @@ int mp_vfs_blockdev_write_ext(mp_vfs_blockdev_t *self, size_t block_num, size_t if (ret == mp_const_none) { return 0; } else { - return MP_OBJ_SMALL_INT_VALUE(ret); + return mp_vfs_check_result(ret); } } diff --git a/tests/extmod/vfs_blockdev_invalid.py b/tests/extmod/vfs_blockdev_invalid.py new file mode 100644 index 0000000000..80afc6414d --- /dev/null +++ b/tests/extmod/vfs_blockdev_invalid.py @@ -0,0 +1,87 @@ +# Tests where the block device returns invalid values + +try: + import vfs + + vfs.VfsFat + vfs.VfsLfs2 +except (ImportError, AttributeError): + print("SKIP") + raise SystemExit + + +class RAMBlockDevice: + ERASE_BLOCK_SIZE = 512 + + def __init__(self, blocks): + self.data = bytearray(blocks * self.ERASE_BLOCK_SIZE) + self.read_res = 0 + self.write_res = 0 + + def readblocks(self, block, buf, off=0): + print("readblocks") + addr = block * self.ERASE_BLOCK_SIZE + off + for i in range(len(buf)): + buf[i] = self.data[addr + i] + return self.read_res + + def writeblocks(self, block, buf, off=None): + if off is None: + # erase, then write + off = 0 + addr = block * self.ERASE_BLOCK_SIZE + off + for i in range(len(buf)): + self.data[addr + i] = buf[i] + return self.write_res + + def ioctl(self, op, arg): + if op == 4: # block count + return len(self.data) // self.ERASE_BLOCK_SIZE + if op == 5: # block size + return self.ERASE_BLOCK_SIZE + if op == 6: # erase block + return 0 + + +try: + bdev = RAMBlockDevice(50) +except MemoryError: + print("SKIP") + raise SystemExit + + +def test(vfs_class): + print(vfs_class) + + vfs_class.mkfs(bdev) + fs = vfs_class(bdev) + + with fs.open("test", "w") as f: + f.write("a" * 64) + + for res in (0, -5, 5, 33, "invalid"): + # -5 is a legitimate negative failure (EIO), positive integer + # is not + + # This variant will fail on open + bdev.read_res = res + try: + with fs.open("test", "r") as f: + print("opened") + except OSError as e: + print("OSError", e) + + # This variant should succeed on open, may fail on read + # unless the filesystem cached the contents already + bdev.read_res = 0 + try: + with fs.open("test", "r") as f: + bdev.read_res = res + print("read 1", f.read(1)) + print("read rest", f.read()) + except OSError as e: + print("OSError", e) + + +test(vfs.VfsLfs2) +test(vfs.VfsFat) # Looks like most failures of underlying device are ignored by VFAT currently diff --git a/tests/extmod/vfs_blockdev_invalid.py.exp b/tests/extmod/vfs_blockdev_invalid.py.exp new file mode 100644 index 0000000000..992111719e --- /dev/null +++ b/tests/extmod/vfs_blockdev_invalid.py.exp @@ -0,0 +1,126 @@ + +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +opened +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +read 1 a +read rest aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +readblocks +OSError [Errno 5] EIO +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +read 1 a +read rest aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +readblocks +OSError [Errno 22] EINVAL +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +read 1 a +read rest aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +readblocks +OSError [Errno 22] EINVAL +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +read 1 a +read rest aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +readblocks +OSError [Errno 22] EINVAL +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +readblocks +read 1 a +read rest aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa + +readblocks +readblocks +readblocks +readblocks +readblocks +opened +readblocks +read 1 a +read rest aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +readblocks +opened +readblocks +read 1 a +read rest aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +readblocks +opened +readblocks +read 1 a +read rest aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +readblocks +opened +readblocks +read 1 a +read rest aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa +readblocks +opened +readblocks +read 1 a +read rest aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa From d278b460f3167c791e8d419580d30f04a296f30b Mon Sep 17 00:00:00 2001 From: Angus Gratton Date: Wed, 31 Jan 2024 18:01:30 +1100 Subject: [PATCH 2/2] py/stream: Check for stream read function returning too many bytes. 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 #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 --- py/stream.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/py/stream.c b/py/stream.c index aa905ca8cc..b0fd712c26 100644 --- a/py/stream.c +++ b/py/stream.c @@ -64,6 +64,11 @@ mp_uint_t mp_stream_rw(mp_obj_t stream, void *buf_, mp_uint_t size, int *errcode if (out_sz == 0) { return done; } + if (out_sz != MP_STREAM_ERROR && out_sz > size) { + // This can only happen if the filesystem implementation returned invalid out_sz + *errcode = MP_EINVAL; + out_sz = MP_STREAM_ERROR; + } if (out_sz == MP_STREAM_ERROR) { // If we read something before getting EAGAIN, don't leak it if (mp_is_nonblocking_error(*errcode) && done != 0) {