[PATCH master] fs: ext4: fix bogus behavior on failure to read ext4 block

[PATCH master] fs: ext4: fix bogus behavior on failure to read ext4 block

[Ahmad Fatoum]

The conversion of blknr from a signed 32-bit to an unsigned 64-type resulted
in the check for error to never return true. Fix this.

Affected configuration would behave incorrectly when served with invalid
blocks. Instead of aborting and having the filesystem bubble up an error
code, it would return invalid data. As there is no ext4 write support,
this wouldn't lead to ext4 data corruption however.

Reported-by: Bastian Krause <bst@pengutronix.de>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 fs/ext4/ext4fs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/ext4fs.c b/fs/ext4/ext4fs.c
index 54349aad3f3f..344d423fd9c4 100644
--- a/fs/ext4/ext4fs.c
+++ b/fs/ext4/ext4fs.c
@@ -74,11 +74,11 @@ loff_t ext4fs_read_file(struct ext2fs_node *node, loff_t pos,
 		loff_t blockend = blocksize;
 		loff_t skipfirst = 0;
 
-		blknr = read_allocated_block(node, i);
-		if (blknr < 0)
-			return blknr;
+		ret = read_allocated_block(node, i);
+		if (ret < 0)
+			return ret;
 
-		blknr = blknr << log2blocksize;
+		blknr = ret << log2blocksize;
 
 		/* Last block.  */
 		if (i == blockcnt - 1) {
-- 
2.29.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH master] fs: ext4: fix bogus behavior on failure to read ext4 block

[Sascha Hauer]

On Wed, Mar 24, 2021 at 01:19:28PM +0100, Ahmad Fatoum wrote:
> The conversion of blknr from a signed 32-bit to an unsigned 64-type resulted
> in the check for error to never return true. Fix this.
> 
> Affected configuration would behave incorrectly when served with invalid
> blocks. Instead of aborting and having the filesystem bubble up an error
> code, it would return invalid data. As there is no ext4 write support,
> this wouldn't lead to ext4 data corruption however.
> 
> Reported-by: Bastian Krause <bst@pengutronix.de>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  fs/ext4/ext4fs.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext4/ext4fs.c b/fs/ext4/ext4fs.c
> index 54349aad3f3f..344d423fd9c4 100644
> --- a/fs/ext4/ext4fs.c
> +++ b/fs/ext4/ext4fs.c
> @@ -74,11 +74,11 @@ loff_t ext4fs_read_file(struct ext2fs_node *node, loff_t pos,
>  		loff_t blockend = blocksize;
>  		loff_t skipfirst = 0;
>  
> -		blknr = read_allocated_block(node, i);
> -		if (blknr < 0)
> -			return blknr;
> +		ret = read_allocated_block(node, i);
> +		if (ret < 0)
> +			return ret;

ret is a 32bit type. What about drives > 2T?

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH master] fs: ext4: fix bogus behavior on failure to read ext4 block

[Ahmad Fatoum]

On 25.03.21 14:04, Sascha Hauer wrote:
> On Wed, Mar 24, 2021 at 01:19:28PM +0100, Ahmad Fatoum wrote:
>> The conversion of blknr from a signed 32-bit to an unsigned 64-type resulted
>> in the check for error to never return true. Fix this.
>>
>> Affected configuration would behave incorrectly when served with invalid
>> blocks. Instead of aborting and having the filesystem bubble up an error
>> code, it would return invalid data. As there is no ext4 write support,
>> this wouldn't lead to ext4 data corruption however.
>>
>> Reported-by: Bastian Krause <bst@pengutronix.de>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>>  fs/ext4/ext4fs.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/ext4/ext4fs.c b/fs/ext4/ext4fs.c
>> index 54349aad3f3f..344d423fd9c4 100644
>> --- a/fs/ext4/ext4fs.c
>> +++ b/fs/ext4/ext4fs.c
>> @@ -74,11 +74,11 @@ loff_t ext4fs_read_file(struct ext2fs_node *node, loff_t pos,
>>  		loff_t blockend = blocksize;
>>  		loff_t skipfirst = 0;
>>  
>> -		blknr = read_allocated_block(node, i);
>> -		if (blknr < 0)
>> -			return blknr;
>> +		ret = read_allocated_block(node, i);
>> +		if (ret < 0)
>> +			return ret;
> 
> ret is a 32bit type. What about drives > 2T?

the first if-clause has a 64-bit type coerced to signed long, but the
rest will only return up to 32-bit, so this needs a bit more investigation.

The fix is correct regardless IMO, so it should go into master.
Expanding the type can go into next then.

Cheers,
Ahmad

> 
> Sascha
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH master] fs: ext4: fix bogus behavior on failure to read ext4 block

[Sascha Hauer]

On Wed, Mar 24, 2021 at 01:19:28PM +0100, Ahmad Fatoum wrote:
> The conversion of blknr from a signed 32-bit to an unsigned 64-type resulted
> in the check for error to never return true. Fix this.
> 
> Affected configuration would behave incorrectly when served with invalid
> blocks. Instead of aborting and having the filesystem bubble up an error
> code, it would return invalid data. As there is no ext4 write support,
> this wouldn't lead to ext4 data corruption however.
> 
> Reported-by: Bastian Krause <bst@pengutronix.de>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  fs/ext4/ext4fs.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Applied, thanks

Sascha

> 
> diff --git a/fs/ext4/ext4fs.c b/fs/ext4/ext4fs.c
> index 54349aad3f3f..344d423fd9c4 100644
> --- a/fs/ext4/ext4fs.c
> +++ b/fs/ext4/ext4fs.c
> @@ -74,11 +74,11 @@ loff_t ext4fs_read_file(struct ext2fs_node *node, loff_t pos,
>  		loff_t blockend = blocksize;
>  		loff_t skipfirst = 0;
>  
> -		blknr = read_allocated_block(node, i);
> -		if (blknr < 0)
> -			return blknr;
> +		ret = read_allocated_block(node, i);
> +		if (ret < 0)
> +			return ret;
>  
> -		blknr = blknr << log2blocksize;
> +		blknr = ret << log2blocksize;
>  
>  		/* Last block.  */
>  		if (i == blockcnt - 1) {
> -- 
> 2.29.2
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox