[U-Boot] [PATCH] fs: ext4: Prevent erasing buffer past file size

[U-Boot] [PATCH] fs: ext4: Prevent erasing buffer past file size

[Marek Vasut]

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 */
-- 
2.16.2

[U-Boot] [PATCH] fs: ext4: Prevent erasing buffer past file size

[Stefano Babic]

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);
>  		}
>  		buf += blocksize - skipfirst;
> +		read_total += blocksize - skipfirst;
>  	}
>  	if (previous_block_number != -1) {
>  		/* spill */
> 

Acked-by: Stefano Babic <sbabic@denx.de>
Tested-by: Stefano Babic <sbabic@denx.de>

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[U-Boot] [PATCH] fs: ext4: Prevent erasing buffer past file size

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180723/3ac24264/attachment.sig>

[U-Boot] [PATCH] fs: ext4: Prevent erasing buffer past file size

[Marek Vasut]

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.

-- 
Best regards,
Marek Vasut