[PATCH] fs: Don't pass stack garbage to filesystem's get_block() in map_bh->b_size

[PATCH] fs: Don't pass stack garbage to filesystem's get_block() in map_bh->b_size

[Theodore Ts'o]

The nobh_truncate_page() function is used by ext2, exofs, and jfs.  Of
these three, only ext2 and jfs's get_block() function pays attention
to bh->b_size --- which is normally always the filesystem blocksize
except when the get_block() function is called by either
mpage_readpage(), mpage_readpages(), or the direct I/O routines in
fs/direct_io.c.

Unfortunately, nobh_truncate_page() does not initialize map_bh before
calling the filesystem-supplied get_block() function.  So ext2 and jfs
will try to calculate the number of blocks to map by taking stack
garbage and shifting it left by inode->i_blkbits.  This should be
*mostly* harmless (except the filesystem will do some unnneeded work)
unless the stack garbage is less than filesystem's blocksize, in which
case maxblocks will be zero, and the attempt to find out whether or
not the filesystem has a hole at a given logical block will fail, and
the page cache entry might not get zero'ed out.

Also if the stack garbage in in map_bh->state happens to have the
BH_Mapped bit set, there could be an attempt to call readpage() on a
non-existent page, which could cause nobh_truncate_page() to return an
error when it should not.

Fix this by initializing map_bh->state and map_bh->size.

Fortunately, it's probably fairly unlikely that ext2 and jfs users
mount with nobh these days.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
Cc: Dave Kleikamp <shaggy@linux.vnet.ibm.com>
Cc: linux-fsdevel@vger.kernel.org
---
 fs/buffer.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index ad01129..1864d0b 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -2736,6 +2736,8 @@ has_buffers:
 		pos += blocksize;
 	}
 
+	map_bh.b_size = blocksize;
+	map_bh.b_state = 0;
 	err = get_block(inode, iblock, &map_bh, 0);
 	if (err)
 		goto unlock;
-- 
1.6.3.rc4.1.g3e14.dirty

Re: [PATCH] fs: Don't pass stack garbage to filesystem's get_block() in map_bh->b_size

[Theodore Tso]

Hmm, I realized that I didn't put the name of the function that was
misbehaving in the summary line.  So how about changing the patch
summary to be:

fs: Fix nobh_truncate_page() to not pass stack garbage to get_block()

    			     	    	       - Ted

Re: [PATCH] fs: Don't pass stack garbage to filesystem's get_block() in map_bh->b_size

[Al Viro]

On Tue, May 12, 2009 at 08:19:59AM -0400, Theodore Tso wrote:
> Hmm, I realized that I didn't put the name of the function that was
> misbehaving in the summary line.  So how about changing the patch
> summary to be:
> 
> fs: Fix nobh_truncate_page() to not pass stack garbage to get_block()

OK...  Merged, will push to Linus in the next pile.

Re: [PATCH] fs: Don't pass stack garbage to filesystem's get_block() in map_bh->b_size

[Dave Kleikamp]

On Tue, 2009-05-12 at 07:37 -0400, Theodore Ts'o wrote:
> The nobh_truncate_page() function is used by ext2, exofs, and jfs.  Of
> these three, only ext2 and jfs's get_block() function pays attention
> to bh->b_size --- which is normally always the filesystem blocksize
> except when the get_block() function is called by either
> mpage_readpage(), mpage_readpages(), or the direct I/O routines in
> fs/direct_io.c.
> 
> Unfortunately, nobh_truncate_page() does not initialize map_bh before
> calling the filesystem-supplied get_block() function.  So ext2 and jfs
> will try to calculate the number of blocks to map by taking stack
> garbage and shifting it left by inode->i_blkbits.  This should be
> *mostly* harmless (except the filesystem will do some unnneeded work)
> unless the stack garbage is less than filesystem's blocksize, in which
> case maxblocks will be zero, and the attempt to find out whether or
> not the filesystem has a hole at a given logical block will fail, and
> the page cache entry might not get zero'ed out.
> 
> Also if the stack garbage in in map_bh->state happens to have the
> BH_Mapped bit set, there could be an attempt to call readpage() on a
> non-existent page, which could cause nobh_truncate_page() to return an
> error when it should not.
> 
> Fix this by initializing map_bh->state and map_bh->size.

This appears obviously correct.

> Fortunately, it's probably fairly unlikely that ext2 and jfs users
> mount with nobh these days.

Well, jfs doesn't have a nobh mount option.  It always calls the *_nobh
routines.  I don't really have a good excuse why.
> 
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

Acked-by: Dave Kleikamp <shaggy@linux.vnet.ibm.com>

> Cc: linux-fsdevel@vger.kernel.org
> ---
>  fs/buffer.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/buffer.c b/fs/buffer.c
> index ad01129..1864d0b 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -2736,6 +2736,8 @@ has_buffers:
>  		pos += blocksize;
>  	}
> 
> +	map_bh.b_size = blocksize;
> +	map_bh.b_state = 0;
>  	err = get_block(inode, iblock, &map_bh, 0);
>  	if (err)
>  		goto unlock;
-- 
David Kleikamp
IBM Linux Technology Center