[PATCH] ocfs2: dereference before NULL check in ocfs2_direct_IO_get_blocks()

[PATCH] ocfs2: dereference before NULL check in ocfs2_direct_IO_get_blocks()

[Florin Malita]

'bh_result' & 'inode' are explicitly checked against NULL so they
shouldn't be dereferenced prior to that.

Coverity ID: 1273, 1274.

Signed-off-by: Florin Malita <fmalita@gmail.com>
---

 fs/ocfs2/aops.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
index 47152bf..16d6478 100644
--- a/fs/ocfs2/aops.c
+++ b/fs/ocfs2/aops.c
@@ -559,13 +559,14 @@ static int ocfs2_direct_IO_get_blocks(st
 	u64 p_blkno;
 	int contig_blocks;
 	unsigned char blocksize_bits;
-	unsigned long max_blocks = bh_result->b_size >> inode->i_blkbits;
+	unsigned long max_blocks;
 
 	if (!inode || !bh_result) {
 		mlog(ML_ERROR, "inode or bh_result is null\n");
 		return -EIO;
 	}
 
+	max_blocks = bh_result->b_size >> inode->i_blkbits;
 	blocksize_bits = inode->i_sb->s_blocksize_bits;
 
 	/* This function won't even be called if the request isn't all

Re: [PATCH] ocfs2: dereference before NULL check in ocfs2_direct_IO_get_blocks()

[Alexey Dobriyan]

On Sat, Jun 03, 2006 at 11:34:39AM -0400, Florin Malita wrote:
> 'bh_result' & 'inode' are explicitly checked against NULL so they
> shouldn't be dereferenced prior to that.
>
> Coverity ID: 1273, 1274.

> --- a/fs/ocfs2/aops.c
> +++ b/fs/ocfs2/aops.c
> @@ -559,13 +559,14 @@ static int ocfs2_direct_IO_get_blocks(st
>  	u64 p_blkno;
>  	int contig_blocks;
>  	unsigned char blocksize_bits;
> -	unsigned long max_blocks = bh_result->b_size >> inode->i_blkbits;
> +	unsigned long max_blocks;
>  
>  	if (!inode || !bh_result) {
>  		mlog(ML_ERROR, "inode or bh_result is null\n");
>  		return -EIO;
>  	}
>  
> +	max_blocks = bh_result->b_size >> inode->i_blkbits;
>  	blocksize_bits = inode->i_sb->s_blocksize_bits;

AFAICS, the patch is BS, as usual with this type of patches.

Can "inode" and "bh_result" be NULL here? I bet they can't.

1. The sole purpose of ocfs2_direct_IO_get_blocks() is to be given to
   blockdev_direct_IO_no_locking() at fs/ocfs2/aops.c:662

	Can blockdev_direct_IO_no_locking() call it with first or third
	arg being NULL?

2. blockdev_direct_IO_no_locking() is static inline wrapper
   include/linux/fs.h:1687

   	Can __blockdev_direct_IO() call it's "get_block" arg with first
	or third arg being NULL?

3. Aiee, __blockdev_direct_IO() is normal function (fs/direct-io.c:1179)
   and it gives damn "get_block" arg to direct_io_worker() at
   fs/direct-io.c:1276

4. direct_io_worker() initializes ->get_block method of "dio" structure
   and then does something my tiny mind fails to understand. All I can
   see it doesn't call nor change method later.

   Where is ->get_block called?

5. That what grep(1) is for. One place

	fs/direct-io.c:553:             ret = (*dio->get_block)(dio->inode, fs_startblk,
	fs/direct-io.c-554-                                             map_bh, create);

   Now time to press PageUp. We're seeking for dereferences of
   "dio->inode" and "map_bh" above.

6. Both are immediately out of question:

   536			map_bh->b_size = fs_count << dio->inode->i_blkbits;
			^^^^^^			     ^^^^^^^^^^
			so it can't be NULL		ditto
   537
   538			create = dio->rw == WRITE;
   539			if (dio->lock_type == DIO_LOCKING) {
   540				if (dio->block_in_file < (i_size_read(dio->inode) >>
   541								dio->blkbits))
   542					create = 0;
   543			} else if (dio->lock_type == DIO_NO_LOCKING) {
   544				create = 0;
   545			}
   546
   547			/*
   548			 * For writes inside i_size we forbid block creations: only
   549			 * overwrites are permitted.  We fall back to buffered writes
   550			 * at a higher level for inside-i_size block-instantiating
   551			 * writes.
   552			 */
   553			ret = (*dio->get_block)(dio->inode, fs_startblk,
				^^^^^^^^^^^^^^	^^^^^^^^^^
				offending call	offending arg
   554							map_bh, create);
							^^^^^^
							offending arg

Re: [PATCH] ocfs2: dereference before NULL check in ocfs2_direct_IO_get_blocks()

[Joel Becker]

On Sat, Jun 03, 2006 at 11:15:58PM +0400, Alexey Dobriyan wrote:
> On Sat, Jun 03, 2006 at 11:34:39AM -0400, Florin Malita wrote:
> > 'bh_result' & 'inode' are explicitly checked against NULL so they
> > shouldn't be dereferenced prior to that.
> >
> > Coverity ID: 1273, 1274.
> 
> AFAICS, the patch is BS, as usual with this type of patches.
> 
> Can "inode" and "bh_result" be NULL here? I bet they can't.

	This is a common result of this sort of scan.  The scan merely
provides good information, not a perfect patch.  There are two
possibilities:

	1) The scan is right, and the dereference is dangerous.  The
	   patch is correct.
	2) The dereference is not dangerous ("can't happen"), and the
	   later check for NULL is spurious.  A correct patch would
	   merely remove the check.

	This is clearly a case of (2), but I bet that (1) is seen just
as often.

Joel

-- 

Life's Little Instruction Book #444

	"Never underestimate the power of a kind word or deed."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [PATCH] ocfs2: dereference before NULL check in ocfs2_direct_IO_get_blocks()

[Florin Malita]

Alexey Dobriyan wrote:
> AFAICS, the patch is BS, as usual with this type of patches.
>   
You missed the full purpose: the patch makes a function consistent about
its own assumptions (but leaves them in place since they're on the
better-safe-than-sorry side). It addresses assumptions inconsistency
(confusing for both human readers and static analysis tools) just as
much as it addresses a possible bug. Regardless of whether "inode" &
"bh_result" can be NULL, I don't think the following is OK (coding-wise):

	unsigned long max_blocks = bh_result->b_size >> inode->i_blkbits;

	if (!inode || !bh_result) {
		mlog(ML_ERROR, "inode or bh_result is null\n");
		return -EIO;
	}


> Can "inode" and "bh_result" be NULL here? I bet they can't
Great, then the NULL branch is dead code and we can fix consistency
differently:

Signed-off-by: Florin Malita <fmalita@gmail.com>
---

 fs/ocfs2/aops.c |    9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
index 47152bf..370c241 100644
--- a/fs/ocfs2/aops.c
+++ b/fs/ocfs2/aops.c
@@ -558,16 +558,9 @@ static int ocfs2_direct_IO_get_blocks(st
 	u64 vbo_max; /* file offset, max_blocks from iblock */
 	u64 p_blkno;
 	int contig_blocks;
-	unsigned char blocksize_bits;
+	unsigned char blocksize_bits = inode->i_sb->s_blocksize_bits;
 	unsigned long max_blocks = bh_result->b_size >> inode->i_blkbits;
 
-	if (!inode || !bh_result) {
-		mlog(ML_ERROR, "inode or bh_result is null\n");
-		return -EIO;
-	}
-
-	blocksize_bits = inode->i_sb->s_blocksize_bits;
-
 	/* This function won't even be called if the request isn't all
 	 * nicely aligned and of the right size, so there's no need
 	 * for us to check any of that. */

Re: [PATCH] ocfs2: dereference before NULL check in ocfs2_direct_IO_get_blocks()

[Arjan van de Ven]

On Sat, 2006-06-03 at 14:16 -0700, Joel Becker wrote:
> On Sat, Jun 03, 2006 at 11:15:58PM +0400, Alexey Dobriyan wrote:
> > On Sat, Jun 03, 2006 at 11:34:39AM -0400, Florin Malita wrote:
> > > 'bh_result' & 'inode' are explicitly checked against NULL so they
> > > shouldn't be dereferenced prior to that.
> > >
> > > Coverity ID: 1273, 1274.
> > 
> > AFAICS, the patch is BS, as usual with this type of patches.
> > 
> > Can "inode" and "bh_result" be NULL here? I bet they can't.
> 
> 	This is a common result of this sort of scan.  The scan merely
> provides good information, not a perfect patch.  There are two
> possibilities:
> 
> 	1) The scan is right, and the dereference is dangerous.  The
> 	   patch is correct.
> 	2) The dereference is not dangerous ("can't happen"), and the
> 	   later check for NULL is spurious.  A correct patch would
> 	   merely remove the check.
> 
> 	This is clearly a case of (2), but I bet that (1) is seen just
> as often.

and in case of (2); newer gcc is often smart enough to do that
automatically :)

Re: [PATCH] ocfs2: dereference before NULL check in ocfs2_direct_IO_get_blocks()

[Mark Fasheh]

On Sat, Jun 03, 2006 at 07:30:10PM -0400, Florin Malita wrote:
> Great, then the NULL branch is dead code and we can fix consistency
> differently:
Ok, this one seems much more reasonable. Thanks for the patch Florin.
	--Mark

--
Mark Fasheh
Senior Software Developer, Oracle
mark.fasheh@oracle.com