[PATCH] befs: redundant test on unsigned in befs_get_block()?

[PATCH] befs: redundant test on unsigned in befs_get_block()?

[Roel Kluin]

block is unsigned, this test appears redundant.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
Or is a different test required?

diff --git a/fs/befs/linuxvfs.c b/fs/befs/linuxvfs.c
index 33baf27..3ab1336 100644
--- a/fs/befs/linuxvfs.c
+++ b/fs/befs/linuxvfs.c
@@ -128,13 +128,6 @@ befs_get_block(struct inode *inode, sector_t block,
 	befs_debug(sb, "---> befs_get_block() for inode %lu, block %ld",
 		   inode->i_ino, block);
 
-	if (block < 0) {
-		befs_error(sb, "befs_get_block() was asked for a block "
-			   "number less than zero: block %ld in inode %lu",
-			   block, inode->i_ino);
-		return -EIO;
-	}
-
 	if (create) {
 		befs_error(sb, "befs_get_block() was asked to write to "
 			   "block %ld in inode %lu", block, inode->i_ino);

Re: [PATCH] befs: redundant test on unsigned in befs_get_block()?

[Andrew Morton]

On Fri, 16 Oct 2009 17:14:41 +0200
Roel Kluin <roel.kluin@gmail.com> wrote:

> block is unsigned, this test appears redundant.
> 
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> Or is a different test required?
> 
> diff --git a/fs/befs/linuxvfs.c b/fs/befs/linuxvfs.c
> index 33baf27..3ab1336 100644
> --- a/fs/befs/linuxvfs.c
> +++ b/fs/befs/linuxvfs.c
> @@ -128,13 +128,6 @@ befs_get_block(struct inode *inode, sector_t block,
>  	befs_debug(sb, "---> befs_get_block() for inode %lu, block %ld",
>  		   inode->i_ino, block);
>  
> -	if (block < 0) {
> -		befs_error(sb, "befs_get_block() was asked for a block "
> -			   "number less than zero: block %ld in inode %lu",
> -			   block, inode->i_ino);
> -		return -EIO;
> -	}
> -
>  	if (create) {
>  		befs_error(sb, "befs_get_block() was asked to write to "
>  			   "block %ld in inode %lu", block, inode->i_ino);

As far as the VFS is concerned, `block' is indeed unsigned and may well
be in the range 2G-4G with a 32-bit sector_t.  Perhaps not possible on
befs but still legal to the VFS.

So the test is wrong from that POV.

However it is possible that befs is defending itself here.  Perhaps code
internal to befs will explode if passed a "negative" block number.  Due
to coding errors within the fs implementation.

So really, we'd need to check all code paths called by
befs_get_block() and check that they are signednessly clean.

Re: [PATCH] befs: redundant test on unsigned in befs_get_block()?

[Roel Kluin]

block is unsigned, check whether it is not too large.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
> As far as the VFS is concerned, `block' is indeed unsigned and may well
> be in the range 2G-4G with a 32-bit sector_t.  Perhaps not possible on
> befs but still legal to the VFS.
> 
> So the test is wrong from that POV.
> 
> However it is possible that befs is defending itself here.  Perhaps code
> internal to befs will explode if passed a "negative" block number.  Due
> to coding errors within the fs implementation.
> 
> So really, we'd need to check all code paths called by
> befs_get_block() and check that they are signednessly clean.

This appears to be already noted by Jesper Juhl in 2004, however it
was never fixed: http://search.luky.org/linux-kernel.2004/msg01392.html

It's getting late here, but what do you think about this:
befs_get_block() calls befs_fblock2brun() and there occurs a 


pos = fblock << BEFS_SB(sb)->block_shift;

and in effect:
if (pos >= data->max_double_indirect_range)
	error out.

So if I'm not mistaken, this should provide protection:

diff --git a/fs/befs/linuxvfs.c b/fs/befs/linuxvfs.c
index 33baf27..eeb4625 100644
--- a/fs/befs/linuxvfs.c
+++ b/fs/befs/linuxvfs.c
@@ -128,9 +128,9 @@ befs_get_block(struct inode *inode, sector_t block,
 	befs_debug(sb, "---> befs_get_block() for inode %lu, block %ld",
 		   inode->i_ino, block);
 
-	if (block < 0) {
-		befs_error(sb, "befs_get_block() was asked for a block "
-			   "number less than zero: block %ld in inode %lu",
+	if (block >= ds->max_double_indirect_range >>
+			BEFS_SB(sb)->block_shift) {
+		befs_error(sb, "befs_get_block() was asked for a too large block: block %ld in inode %lu",
 			   block, inode->i_ino);
 		return -EIO;
 	}