Fix buffer overrun in lfs_cache_zero #11614
Conversation
|
@kyle-cypress, thank you for your changes. |
lfs_cache_zero assumes that the buffer which it is passed is prog_size in length. There are two cases where this is false: 1. In lfs_file_opencfg, when the file is opened readonly file->cache.buffer is malloc'd as read_size 2. In lfs_init, lfs->rcache is always malloc'd as read_size, passed to lfs_cache_zero. In both cases, the buffer in question is passed to lfs_cache_zero later in the same function. To address this, add a cache_size argument to lfs_cache_zero, which is set to either read_size or prog_size as appropriate based on the caller's knowledge of the buffer that it is passing in. The "pcache" argument to lfs_cache_zero is renamed to simply "cache" to make it clear that this pointer is not necessarily to a program cache.
kyle-cypress
force-pushed
the
pr/lfs-cache-zero
branch
from
84f54b6 to
7f24bfc
Compare
|
Ah! Thanks for creating a PR. This was actually fixed in littlefs v1.6.1 here: littlefs-project/littlefs@3419284 (Mbed OS is currently at littlefs v1.6.0). I created a PR to bring Mbed OS up to date: #11624 The solution I went with was slightly different, the purpose of Sidenote: v2 does away with separate read/prog cache sizes and has a single I did look into an explicit size field on each cache, which provide a more generalized solution. However it did add a lot of complication to cache handling and increased the code-size by a noticeable amount. Though it may be worth revisiting, at least one user has raised the issue that a single cache size limits the flexibility of having different files open with different cache sizes to better utilize the available RAM. |
|
Looks like #11624 needs some greentea test updates so will be a bit longer |
|
#11624 was approved so I would just wait for it |
|
Closing this in favour of #11624 |
Description
lfs_cache_zero assumes that the buffer which it is passed is prog_size in length. There are two cases where this is false:
In both cases, the buffer in question is passed to lfs_cache_zero later in the same function.
To address this, add a cache_size argument to lfs_cache_zero, which is set to either read_size or prog_size as appropriate based on the caller's knowledge of the buffer that it is passing in.
The "pcache" argument to lfs_cache_zero is renamed to simply "cache" to make it clear that this pointer is not necessarily to a program cache.
Note: Ideally, I think the lfs_cache_t struct should be extended to include a buffer_size member; right now there is also no length checking possible for user-provided buffers. However, such a change would be much more invasive and possibly breaking for user applications. This PR therefore attempts to simply fix the cases that are clearly wrong in a manner which should be transparent to applications.
Pull request type
Reviewers
Release Notes