[PATCH][isapnp] Fix a potential NULL pointer dereference in isapnp_read_tag()

[PATCH][isapnp] Fix a potential NULL pointer dereference in isapnp_read_tag()

[Jesper Juhl]

The Coverity checker spotted (as bug #809) that we dereference 'type' 
long before we actually test it against NULL in 
drivers/pnp/isapnp/core.c::isapnp_read_tag() - both branches of the 
'if (tag & 0x80)' dereference type, and since this 'if' is before the test 
against NULL and the return of -1, this will blow up is ever type is NULL.
This is easy to fix by simply moving the NULL test to the beginning of 
the function.


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

 drivers/pnp/isapnp/core.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/00-INDEX b/Documentation/00-INDEX
diff --git a/drivers/pnp/isapnp/core.c b/drivers/pnp/isapnp/core.c
index a0b1587..5696924 100644
--- a/drivers/pnp/isapnp/core.c
+++ b/drivers/pnp/isapnp/core.c
@@ -356,6 +356,8 @@ static int __init isapnp_read_tag(unsigned char *type, unsigned short *size)
 {
 	unsigned char tag, tmp[2];
 
+	if (!type)	/* wrong type */
+		return -1;
 	isapnp_peek(&tag, 1);
 	if (tag == 0)				/* invalid tag */
 		return -1;
@@ -370,8 +372,6 @@ static int __init isapnp_read_tag(unsigned char *type, unsigned short *size)
 #if 0
 	printk(KERN_DEBUG "tag = 0x%x, type = 0x%x, size = %i\n", tag, *type, *size);
 #endif
-	if (type == 0)				/* wrong type */
-		return -1;
 	if (*type == 0xff && *size == 0xffff)	/* probably invalid data */
 		return -1;
 	return 0;

Re: [PATCH][isapnp] Fix a potential NULL pointer dereference in isapnp_read_tag()

[Andrew Morton]

On Sun, 1 Jul 2007 01:38:31 +0200
Jesper Juhl <jesper.juhl@gmail.com> wrote:

> The Coverity checker spotted (as bug #809) that we dereference 'type' 
> long before we actually test it against NULL in 
> drivers/pnp/isapnp/core.c::isapnp_read_tag() - both branches of the 
> 'if (tag & 0x80)' dereference type, and since this 'if' is before the test 
> against NULL and the return of -1, this will blow up is ever type is NULL.
> This is easy to fix by simply moving the NULL test to the beginning of 
> the function.
> 
> 
> Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
> ---
> 
>  drivers/pnp/isapnp/core.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/00-INDEX b/Documentation/00-INDEX
> diff --git a/drivers/pnp/isapnp/core.c b/drivers/pnp/isapnp/core.c
> index a0b1587..5696924 100644
> --- a/drivers/pnp/isapnp/core.c
> +++ b/drivers/pnp/isapnp/core.c
> @@ -356,6 +356,8 @@ static int __init isapnp_read_tag(unsigned char *type, unsigned short *size)
>  {
>  	unsigned char tag, tmp[2];
>  
> +	if (!type)	/* wrong type */
> +		return -1;
>  	isapnp_peek(&tag, 1);
>  	if (tag == 0)				/* invalid tag */
>  		return -1;
> @@ -370,8 +372,6 @@ static int __init isapnp_read_tag(unsigned char *type, unsigned short *size)
>  #if 0
>  	printk(KERN_DEBUG "tag = 0x%x, type = 0x%x, size = %i\n", tag, *type, *size);
>  #endif
> -	if (type == 0)				/* wrong type */
> -		return -1;
>  	if (*type == 0xff && *size == 0xffff)	/* probably invalid data */
>  		return -1;
>  	return 0;

dood, look at the callers.  NULL is not possible here.

Re: [PATCH][isapnp] Fix a potential NULL pointer dereference in isapnp_read_tag()

[Jesper Juhl]

On 03/07/07, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Sun, 1 Jul 2007 01:38:31 +0200
> Jesper Juhl <jesper.juhl@gmail.com> wrote:
>
> > The Coverity checker spotted (as bug #809) that we dereference 'type'
> > long before we actually test it against NULL in
> > drivers/pnp/isapnp/core.c::isapnp_read_tag() - both branches of the
> > 'if (tag & 0x80)' dereference type, and since this 'if' is before the test
> > against NULL and the return of -1, this will blow up is ever type is NULL.
> > This is easy to fix by simply moving the NULL test to the beginning of
> > the function.
> >
[snip]
>
> dood, look at the callers.  NULL is not possible here.
>
You are right, there's absolutely no way that we could get a NULL
pointer there - that was sloppy of me :-(

I guess we should just get rid of the check completely.  I'll cook up
a patch for that tomorrow.

-- 
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

[PATCH][isapnp] Remove pointless check of 'type' against 0 in isapnp_read_tag()

[Jesper Juhl]

On Wednesday 04 July 2007 03:04:13 Jesper Juhl wrote:
> On 03/07/07, Andrew Morton <akpm@linux-foundation.org> wrote:
> > On Sun, 1 Jul 2007 01:38:31 +0200
> >
> > Jesper Juhl <jesper.juhl@gmail.com> wrote:
> > > The Coverity checker spotted (as bug #809) that we dereference 'type'
> > > long before we actually test it against NULL in
> > > drivers/pnp/isapnp/core.c::isapnp_read_tag() - both branches of the
> > > 'if (tag & 0x80)' dereference type, and since this 'if' is before the
> > > test against NULL and the return of -1, this will blow up is ever type
> > > is NULL. This is easy to fix by simply moving the NULL test to the
> > > beginning of the function.
>
> [snip]
>
> > dood, look at the callers.  NULL is not possible here.
>
> You are right, there's absolutely no way that we could get a NULL
> pointer there - that was sloppy of me :-(
>
> I guess we should just get rid of the check completely.  I'll cook up
> a patch for that tomorrow.

Ok, here's a patch to just remove the check.



In drivers/pnp/isapnp/core.c::isapnp_read_tag() there is a test of 
'type' being == 0 a bit down in the function. That test doesn't make 
any sense. If 'type' could indeed be NULL, then the test happens way 
too late as we'd already have tried to dereference the pointer 
earlier and looking at the callers it also turns out that there is 
no way type can ever actually be NULL.
So the test is completely pointless and should just be removed.


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

 drivers/pnp/isapnp/core.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/pnp/isapnp/core.c b/drivers/pnp/isapnp/core.c
index a0b1587..914d00c 100644
--- a/drivers/pnp/isapnp/core.c
+++ b/drivers/pnp/isapnp/core.c
@@ -370,8 +370,6 @@ static int __init isapnp_read_tag(unsigned char *type, unsigned short *size)
 #if 0
 	printk(KERN_DEBUG "tag = 0x%x, type = 0x%x, size = %i\n", tag, *type, *size);
 #endif
-	if (type == 0)				/* wrong type */
-		return -1;
 	if (*type == 0xff && *size == 0xffff)	/* probably invalid data */
 		return -1;
 	return 0;