[U-Boot] [PATCH] fs: ext4: Prevent erasing buffer past file size
The variable 'n' represents the number of bytes to be read from a certain offset in a file, to a certain offset in buffer 'buf'. The variable 'len' represents the length of the entire file, clamped correctly to avoid any overflows.
Therefore, comparing 'n' and 'len' to determine whether clearing 'n' bytes of the buffer 'buf' at a certain offset would clear data past buffer 'buf' cannot lead to a correct result, since the 'n' does not contain the offset from the beginning of the file.
This patch keeps track of the amount of data read and checks for the buffer overflow by comparing the 'n' to the remaining amount of data to be read instead.
Signed-off-by: Marek Vasut marex@denx.de Cc: Ian Ray ian.ray@ge.com Cc: Martyn Welch martyn.welch@collabora.co.uk Cc: Stefano Babic sbabic@denx.de Cc: Tom Rini trini@konsulko.com Fixes: ecdfb4195b20 ("ext4: recover from filesystem corruption when reading") --- fs/ext4/ext4fs.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/fs/ext4/ext4fs.c b/fs/ext4/ext4fs.c index 2a28031d14..537aa05eff 100644 --- a/fs/ext4/ext4fs.c +++ b/fs/ext4/ext4fs.c @@ -60,6 +60,7 @@ int ext4fs_read_file(struct ext2fs_node *node, loff_t pos, lbaint_t delayed_extent = 0; lbaint_t delayed_skipfirst = 0; lbaint_t delayed_next = 0; + lbaint_t read_total = 0; char *delayed_buf = NULL; short status;
@@ -140,13 +141,14 @@ int ext4fs_read_file(struct ext2fs_node *node, loff_t pos, return -1; previous_block_number = -1; } - /* Zero no more than `len' bytes. */ + /* Zero no more than 'filesize' bytes. */ n = blocksize - skipfirst; - if (n > len) - n = len; + if (n > (len - read_total)) + n = (len - read_total); memset(buf, 0, n); } buf += blocksize - skipfirst; + read_total += blocksize - skipfirst; } if (previous_block_number != -1) { /* spill */
Hi Marek,
On 23/07/2018 11:42, Marek Vasut wrote:
The variable 'n' represents the number of bytes to be read from a certain offset in a file, to a certain offset in buffer 'buf'. The variable 'len' represents the length of the entire file, clamped correctly to avoid any overflows.
Therefore, comparing 'n' and 'len' to determine whether clearing 'n' bytes of the buffer 'buf' at a certain offset would clear data past buffer 'buf' cannot lead to a correct result, since the 'n' does not contain the offset from the beginning of the file.
This patch keeps track of the amount of data read and checks for the buffer overflow by comparing the 'n' to the remaining amount of data to be read instead.
Signed-off-by: Marek Vasut marex@denx.de
Cc: Ian Ray ian.ray@ge.com Cc: Martyn Welch martyn.welch@collabora.co.uk Cc: Stefano Babic sbabic@denx.de Cc: Tom Rini trini@konsulko.com Fixes: ecdfb4195b20 ("ext4: recover from filesystem corruption when reading")
fs/ext4/ext4fs.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/fs/ext4/ext4fs.c b/fs/ext4/ext4fs.c index 2a28031d14..537aa05eff 100644 --- a/fs/ext4/ext4fs.c +++ b/fs/ext4/ext4fs.c @@ -60,6 +60,7 @@ int ext4fs_read_file(struct ext2fs_node *node, loff_t pos, lbaint_t delayed_extent = 0; lbaint_t delayed_skipfirst = 0; lbaint_t delayed_next = 0;
- lbaint_t read_total = 0; char *delayed_buf = NULL; short status;
@@ -140,13 +141,14 @@ int ext4fs_read_file(struct ext2fs_node *node, loff_t pos, return -1; previous_block_number = -1; }
/* Zero no more than `len' bytes. */
/* Zero no more than 'filesize' bytes. */ n = blocksize - skipfirst;
if (n > len)n = len;
if (n > (len - read_total))n = (len - read_total); memset(buf, 0, n);read_total += blocksize - skipfirst;
Acked-by: Stefano Babic sbabic@denx.de Tested-by: Stefano Babic sbabic@denx.de
Best regards, Stefano Babic
On Mon, Jul 23, 2018 at 11:42:12AM +0200, Marek Vasut wrote:
The variable 'n' represents the number of bytes to be read from a certain offset in a file, to a certain offset in buffer 'buf'. The variable 'len' represents the length of the entire file, clamped correctly to avoid any overflows.
Therefore, comparing 'n' and 'len' to determine whether clearing 'n' bytes of the buffer 'buf' at a certain offset would clear data past buffer 'buf' cannot lead to a correct result, since the 'n' does not contain the offset from the beginning of the file.
This patch keeps track of the amount of data read and checks for the buffer overflow by comparing the 'n' to the remaining amount of data to be read instead.
Signed-off-by: Marek Vasut marex@denx.de Cc: Ian Ray ian.ray@ge.com Cc: Martyn Welch martyn.welch@collabora.co.uk Cc: Stefano Babic sbabic@denx.de Cc: Tom Rini trini@konsulko.com Fixes: ecdfb4195b20 ("ext4: recover from filesystem corruption when reading")
Good catch. Can this problem also be recreated/tested with test/fs/fs-test.sh? Thanks!
On 07/23/2018 04:09 PM, Tom Rini wrote:
On Mon, Jul 23, 2018 at 11:42:12AM +0200, Marek Vasut wrote:
The variable 'n' represents the number of bytes to be read from a certain offset in a file, to a certain offset in buffer 'buf'. The variable 'len' represents the length of the entire file, clamped correctly to avoid any overflows.
Therefore, comparing 'n' and 'len' to determine whether clearing 'n' bytes of the buffer 'buf' at a certain offset would clear data past buffer 'buf' cannot lead to a correct result, since the 'n' does not contain the offset from the beginning of the file.
This patch keeps track of the amount of data read and checks for the buffer overflow by comparing the 'n' to the remaining amount of data to be read instead.
Signed-off-by: Marek Vasut marex@denx.de Cc: Ian Ray ian.ray@ge.com Cc: Martyn Welch martyn.welch@collabora.co.uk Cc: Stefano Babic sbabic@denx.de Cc: Tom Rini trini@konsulko.com Fixes: ecdfb4195b20 ("ext4: recover from filesystem corruption when reading")
Good catch. Can this problem also be recreated/tested with test/fs/fs-test.sh? Thanks!
I think so. I'd memalign() a buffer with some safe space around it, ie. a 4k page on each side and poison it with a pattern. I'd then read a file which is not ext4 FS block size aligned into 1-page offset from the beginning of that buffer . Finally, I'd check if exactly the size of the file was changed in that buffer and the poisoned area of the buffer still contains the poison or not.
|................poison................| | v |...poison...|file...|.DZ.|...poison...|
If DZ is poison, everything is OK. If DZ is 0x0, the ext4 corruption happened.
participants (3)
-
Marek Vasut -
Stefano Babic -
Tom Rini