[PATCH] fix potential null pointer deref in quota

[PATCH] fix potential null pointer deref in quota

[Jesper Juhl]

The coverity checker noticed that we may pass a NULL super_block to
do_quotactl() that dereferences it.
Dereferencing NULL pointers is bad medicine, better check and fail 
gracefully.

Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---

 fs/quota.c |    3 +++
 1 files changed, 3 insertions(+)

--- linux-2.6.16-rc6-orig/fs/quota.c	2006-03-12 14:19:02.000000000 +0100
+++ linux-2.6.16-rc6/fs/quota.c	2006-03-18 23:03:32.000000000 +0100
@@ -231,6 +231,9 @@ static int do_quotactl(struct super_bloc
 {
 	int ret;
 
+	if (!sb)
+		return -ENODEV;
+
 	switch (cmd) {
 		case Q_QUOTAON: {
 			char *pathname;

Re: [PATCH] fix potential null pointer deref in quota

[Andrew Morton]

Jesper Juhl <jesper.juhl@gmail.com> wrote:
>
> The coverity checker noticed that we may pass a NULL super_block to
>  do_quotactl() that dereferences it.
>  Dereferencing NULL pointers is bad medicine, better check and fail 
>  gracefully.
> 
>  Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
>  ---
> 
>   fs/quota.c |    3 +++
>   1 files changed, 3 insertions(+)
> 
>  --- linux-2.6.16-rc6-orig/fs/quota.c	2006-03-12 14:19:02.000000000 +0100
>  +++ linux-2.6.16-rc6/fs/quota.c	2006-03-18 23:03:32.000000000 +0100
>  @@ -231,6 +231,9 @@ static int do_quotactl(struct super_bloc
>   {
>   	int ret;
>   
>  +	if (!sb)
>  +		return -ENODEV;
>  +
>   	switch (cmd) {
>   		case Q_QUOTAON: {
>   			char *pathname;

I'd have thought that check_quotactl_valid() would be the appropriate place
for this check.  Jan, can you please sort out what we need to do here?

Re: [PATCH] fix potential null pointer deref in quota

[Jesper Juhl]

On 3/20/06, Andrew Morton <akpm@osdl.org> wrote:
> Jesper Juhl <jesper.juhl@gmail.com> wrote:
> >
> > The coverity checker noticed that we may pass a NULL super_block to
> >  do_quotactl() that dereferences it.
> >  Dereferencing NULL pointers is bad medicine, better check and fail
> >  gracefully.
> >
[snip]
>
> I'd have thought that check_quotactl_valid() would be the appropriate place
> for this check.  Jan, can you please sort out what we need to do here?
>

You may well be right. I openly admit that this is the first time I've
ever stuck my head in the quota code. I picked the location I did
simply because I thought making the function resistant to being passed
invalid data was the sane thing to do, but there may well be a more
logical place to fix it.

--
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [PATCH] fix potential null pointer deref in quota

[Jan Kara]

  Hello,

> The coverity checker noticed that we may pass a NULL super_block to
> do_quotactl() that dereferences it.
> Dereferencing NULL pointers is bad medicine, better check and fail 
> gracefully.
  Umm, when do you think we can dereference NULL pointer to a super_block?
check_quotactl_valid() allows sb==NULL only in the case of Q_SYNC command.
And that is handled in do_quotactl() correctly even if sb==NULL...

								Honza

-- 
Jan Kara <jack@suse.cz>
SuSE CR Labs

Re: [PATCH] fix potential null pointer deref in quota

[Jesper Juhl]

On 3/20/06, Jan Kara <jack@suse.cz> wrote:
>   Hello,
>
> > The coverity checker noticed that we may pass a NULL super_block to
> > do_quotactl() that dereferences it.
> > Dereferencing NULL pointers is bad medicine, better check and fail
> > gracefully.
>   Umm, when do you think we can dereference NULL pointer to a super_block?
> check_quotactl_valid() allows sb==NULL only in the case of Q_SYNC command.
> And that is handled in do_quotactl() correctly even if sb==NULL...
>

Reading the code again it looks like you are right and that this is a
false positive by the coverity checker.

This is what the checker had to say :


346  	asmlinkage long sys_quotactl(unsigned int cmd, const char __user
*special, qid_t id, void __user *addr)
347  	{
348  		uint cmds, type;

Event assign_zero: Variable "sb" assigned value 0.
Also see events:
[var_deref_model][var_deref_model][var_deref_model][var_deref_model][var_deref_model][var_deref_model][var_deref_model][var_deref_model]

349  		struct super_block *sb = NULL;
350  		struct block_device *bdev;
351  		char *tmp;
352  		int ret;
353  	
354  		cmds = cmd >> SUBCMDSHIFT;
355  		type = cmd & SUBCMDMASK;
356  	

At conditional (1): "cmds != 8388609" taking false path
At conditional (2): "special != 0" taking false path

357  		if (cmds != Q_SYNC || special) {
358  			tmp = getname(special);
359  			if (IS_ERR(tmp))
360  				return PTR_ERR(tmp);
361  			bdev = lookup_bdev(tmp);
362  			putname(tmp);
363  			if (IS_ERR(bdev))
364  				return PTR_ERR(bdev);
365  			sb = get_super(bdev);
366  			bdput(bdev);
367  			if (!sb)
368  				return -ENODEV;
369  		}
370  	
371  		ret = check_quotactl_valid(sb, type, cmds, id);

At conditional (3): "ret >= 0" taking true path

372  		if (ret >= 0)

Event var_deref_model: Variable "sb" tracked as NULL was passed to a
function that dereferences it. [model]
Event var_deref_model: Variable "sb" tracked as NULL was passed to a
function that dereferences it. [model]
Event var_deref_model: Variable "sb" tracked as NULL was passed to a
function that dereferences it. [model]
Event var_deref_model: Variable "sb" tracked as NULL was passed to a
function that dereferences it. [model]
Event var_deref_model: Variable "sb" tracked as NULL was passed to a
function that dereferences it. [model]
Event var_deref_model: Variable "sb" tracked as NULL was passed to a
function that dereferences it. [model]
Event var_deref_model: Variable "sb" tracked as NULL was passed to a
function that dereferences it. [model]
Event var_deref_model: Variable "sb" tracked as NULL was passed to a
function that dereferences it. [model]
Also see events:
[assign_zero][var_deref_model][var_deref_model][var_deref_model][var_deref_model][var_deref_model][var_deref_model][var_deref_model]

373  			ret = do_quotactl(sb, type, cmds, id, addr);
374  		if (sb)
375  			drop_super(sb);
376  	
377  		return ret;
378  	}



--
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html