[PATCH] ISDN: Avoid a potential NULL ptr deref in ippp

[PATCH] ISDN: Avoid a potential NULL ptr deref in ippp

[Jesper Juhl]

There's a potential problem in isdn_ppp.c::isdn_ppp_decompress().
dev_alloc_skb() may fail and return NULL. If it does we will be passing a
NULL skb_out to ipc->decompress() and may also end up
dereferencing a NULL pointer at 
    *proto = isdn_ppp_strip_proto(skb_out);
Correct this by testing 'skb_out' against NULL early and bail out.


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

 drivers/isdn/i4l/isdn_ppp.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/isdn/i4l/isdn_ppp.c b/drivers/isdn/i4l/isdn_ppp.c
index 119412d..5a97ce6 100644
--- a/drivers/isdn/i4l/isdn_ppp.c
+++ b/drivers/isdn/i4l/isdn_ppp.c
@@ -2536,6 +2536,11 @@ static struct sk_buff *isdn_ppp_decompre
   		rsparm.maxdlen = IPPP_RESET_MAXDATABYTES;
   
   		skb_out = dev_alloc_skb(is->mru + PPP_HDRLEN);
+  		if (!skb_out) {
+  			kfree_skb(skb);
+  			printk(KERN_ERR "ippp: decomp memory allocation failure\n");
+			return NULL;
+  		}  		
 		len = ipc->decompress(stat, skb, skb_out, &rsparm);
 		kfree_skb(skb);
 		if (len <= 0) {

[PATCH] net s2io: return on NULL dev_alloc_skb()

[David Rientjes]

Checks for NULL dev_alloc_skb() and returns on true to avoid subsequent
dereference.

Cc: Jeff Garzik <jgarzik@pobox.com>
Cc: Christoph Hellwig <hch@infrared.org>
Signed-off-by: David Rientjes <rientjes@cs.washington.edu>
---
 drivers/net/s2io.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/drivers/net/s2io.c b/drivers/net/s2io.c
index a231ab7..33569ec 100644
--- a/drivers/net/s2io.c
+++ b/drivers/net/s2io.c
@@ -5985,6 +5985,11 @@ static int set_rxd_buffer_pointer(nic_t 
 			((RxD3_t*)rxdp)->Buffer1_ptr = *temp1;
 		} else {
 			*skb = dev_alloc_skb(size);
+			if (!(*skb)) {
+				DBG_PRINT(ERR_DBG, "%s: dev_alloc_skb failed\n",
+					  dev->name);
+				return -ENOMEM;
+			}
 			((RxD3_t*)rxdp)->Buffer2_ptr = *temp2 =
 				pci_map_single(sp->pdev, (*skb)->data,
 					       dev->mtu + 4,
@@ -6007,7 +6012,11 @@ static int set_rxd_buffer_pointer(nic_t 
 			((RxD3_t*)rxdp)->Buffer2_ptr = *temp2;
 		} else {
 			*skb = dev_alloc_skb(size);
-
+			if (!(*skb)) {
+				DBG_PRINT(ERR_DBG, "%s: dev_alloc_skb failed\n",
+					  dev->name);
+				return -ENOMEM;
+			}
 			((RxD3_t*)rxdp)->Buffer0_ptr = *temp0 =
 				pci_map_single(sp->pdev, ba->ba_0, BUF0_LEN,
 					       PCI_DMA_FROMDEVICE);

Re: [PATCH] ISDN: Avoid a potential NULL ptr deref in ippp

[David Rientjes]

On Mon, 30 Oct 2006, Jesper Juhl wrote:

> 
> There's a potential problem in isdn_ppp.c::isdn_ppp_decompress().
> dev_alloc_skb() may fail and return NULL. If it does we will be passing a
> NULL skb_out to ipc->decompress() and may also end up
> dereferencing a NULL pointer at 
>     *proto = isdn_ppp_strip_proto(skb_out);
> Correct this by testing 'skb_out' against NULL early and bail out.
> 

Good catch.  There's also been a potential NULL pointer on 
etrax_ethernet_init in drivers/net/cris/eth_v10.c.  RxDescList[i].skb 
calls dev_alloc_skb and does not check its return value before 
dereferencing it for the RxDescList[i].descr.buf virt_to_phys conversion.

(Mikael Starvik Cc'd)

		David

[PATCH] drivers cris: return on NULL dev_alloc_skb()

[David Rientjes]

If the next descriptor array entry cannot be allocated by dev_alloc_skb(),
return immediately so it is not dereferenced later.  We cannot register 
the device with a partial descriptor list.

Cc: Mikael Starvik <starvik@axis.com>
Signed-off-by: David Rientjes <rientjes@cs.washington.edu>
---
 drivers/net/cris/eth_v10.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/net/cris/eth_v10.c b/drivers/net/cris/eth_v10.c
index 966b563..a03d781 100644
--- a/drivers/net/cris/eth_v10.c
+++ b/drivers/net/cris/eth_v10.c
@@ -509,6 +509,8 @@ etrax_ethernet_init(void)
 		 * does not share cacheline with any other data (to avoid cache bug)
 		 */
 		RxDescList[i].skb = dev_alloc_skb(MAX_MEDIA_DATA_SIZE + 2 * L1_CACHE_BYTES);
+		if (!RxDescList[i].skb)
+			return -ENOMEM;
 		RxDescList[i].descr.ctrl   = 0;
 		RxDescList[i].descr.sw_len = MAX_MEDIA_DATA_SIZE;
 		RxDescList[i].descr.next   = virt_to_phys(&RxDescList[i + 1]);

Re: [PATCH] net s2io: return on NULL dev_alloc_skb()

[Jeff Garzik]

David Rientjes wrote:
> Checks for NULL dev_alloc_skb() and returns on true to avoid subsequent
> dereference.
> 
> Cc: Jeff Garzik <jgarzik@pobox.com>
> Cc: Christoph Hellwig <hch@infrared.org>
> Signed-off-by: David Rientjes <rientjes@cs.washington.edu>

applied

Re: [PATCH] ISDN: Avoid a potential NULL ptr deref in ippp

[Jesper Juhl]

Any reason why we can't apply the patch below?

On 30/10/06, Jesper Juhl <jesper.juhl@gmail.com> wrote:
>
> There's a potential problem in isdn_ppp.c::isdn_ppp_decompress().
> dev_alloc_skb() may fail and return NULL. If it does we will be passing a
> NULL skb_out to ipc->decompress() and may also end up
> dereferencing a NULL pointer at
>     *proto = isdn_ppp_strip_proto(skb_out);
> Correct this by testing 'skb_out' against NULL early and bail out.
>
>
> Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
> ---
>
>  drivers/isdn/i4l/isdn_ppp.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/isdn/i4l/isdn_ppp.c b/drivers/isdn/i4l/isdn_ppp.c
> index 119412d..5a97ce6 100644
> --- a/drivers/isdn/i4l/isdn_ppp.c
> +++ b/drivers/isdn/i4l/isdn_ppp.c
> @@ -2536,6 +2536,11 @@ static struct sk_buff *isdn_ppp_decompre
>                 rsparm.maxdlen = IPPP_RESET_MAXDATABYTES;
>
>                 skb_out = dev_alloc_skb(is->mru + PPP_HDRLEN);
> +               if (!skb_out) {
> +                       kfree_skb(skb);
> +                       printk(KERN_ERR "ippp: decomp memory allocation failure\n");
> +                       return NULL;
> +               }
>                 len = ipc->decompress(stat, skb, skb_out, &rsparm);
>                 kfree_skb(skb);
>                 if (len <= 0) {
>


-- 
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] ISDN: Avoid a potential NULL ptr deref in ippp

[David Rientjes]

On Tue, 21 Nov 2006, Jesper Juhl wrote:

> Any reason why we can't apply the patch below?
> 
> On 30/10/06, Jesper Juhl <jesper.juhl@gmail.com> wrote:
> >
> > There's a potential problem in isdn_ppp.c::isdn_ppp_decompress().
> > dev_alloc_skb() may fail and return NULL. If it does we will be passing a
> > NULL skb_out to ipc->decompress() and may also end up
> > dereferencing a NULL pointer at
> >     *proto = isdn_ppp_strip_proto(skb_out);
> > Correct this by testing 'skb_out' against NULL early and bail out.
> >
> >
> > Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
> > ---
> >
> >  drivers/isdn/i4l/isdn_ppp.c |    5 +++++
> >  1 files changed, 5 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/isdn/i4l/isdn_ppp.c b/drivers/isdn/i4l/isdn_ppp.c
> > index 119412d..5a97ce6 100644
> > --- a/drivers/isdn/i4l/isdn_ppp.c
> > +++ b/drivers/isdn/i4l/isdn_ppp.c
> > @@ -2536,6 +2536,11 @@ static struct sk_buff *isdn_ppp_decompre
> >                 rsparm.maxdlen = IPPP_RESET_MAXDATABYTES;
> >
> >                 skb_out = dev_alloc_skb(is->mru + PPP_HDRLEN);
> > +               if (!skb_out) {
> > +                       kfree_skb(skb);
> > +                       printk(KERN_ERR "ippp: decomp memory allocation
> > failure\n");
> > +                       return NULL;
> > +               }
> >                 len = ipc->decompress(stat, skb, skb_out, &rsparm);
> >                 kfree_skb(skb);
> >                 if (len <= 0) {
> >
> 

I'm not sure that there's a problem with the original code.  If skb_out 
cannot be allocated, the ipc->decompress function should return NULL 
because struct ippp_struct *master would have been passed as NULL.  So len 
would be 0 upon return, skb would be freed, and the following switch 
statement would catch the error.  Notice it's not a bug to pass NULL to 
kfree_skb() later.

		David

Re: [PATCH] ISDN: Avoid a potential NULL ptr deref in ippp

[Jesper Juhl]

On 21/11/06, David Rientjes <rientjes@cs.washington.edu> wrote:
> On Tue, 21 Nov 2006, Jesper Juhl wrote:
>
> > Any reason why we can't apply the patch below?
> >
> > On 30/10/06, Jesper Juhl <jesper.juhl@gmail.com> wrote:
> > >
> > > There's a potential problem in isdn_ppp.c::isdn_ppp_decompress().
> > > dev_alloc_skb() may fail and return NULL. If it does we will be passing a
> > > NULL skb_out to ipc->decompress() and may also end up
> > > dereferencing a NULL pointer at
> > >     *proto = isdn_ppp_strip_proto(skb_out);
> > > Correct this by testing 'skb_out' against NULL early and bail out.
> > >
> > >
> > > Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
> > > ---
> > >
> > >  drivers/isdn/i4l/isdn_ppp.c |    5 +++++
> > >  1 files changed, 5 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/drivers/isdn/i4l/isdn_ppp.c b/drivers/isdn/i4l/isdn_ppp.c
> > > index 119412d..5a97ce6 100644
> > > --- a/drivers/isdn/i4l/isdn_ppp.c
> > > +++ b/drivers/isdn/i4l/isdn_ppp.c
> > > @@ -2536,6 +2536,11 @@ static struct sk_buff *isdn_ppp_decompre
> > >                 rsparm.maxdlen = IPPP_RESET_MAXDATABYTES;
> > >
> > >                 skb_out = dev_alloc_skb(is->mru + PPP_HDRLEN);
> > > +               if (!skb_out) {
> > > +                       kfree_skb(skb);
> > > +                       printk(KERN_ERR "ippp: decomp memory allocation
> > > failure\n");
> > > +                       return NULL;
> > > +               }
> > >                 len = ipc->decompress(stat, skb, skb_out, &rsparm);
> > >                 kfree_skb(skb);
> > >                 if (len <= 0) {
> > >
> >
>
> I'm not sure that there's a problem with the original code.  If skb_out
> cannot be allocated, the ipc->decompress function should return NULL
> because struct ippp_struct *master would have been passed as NULL.  So len
> would be 0 upon return, skb would be freed, and the following switch
> statement would catch the error.  Notice it's not a bug to pass NULL to
> kfree_skb() later.
>
Ok, I see your point. There may not be an actual bug here, but
couldn't it still be considered an improvement, given that with my
patch we'll  a) print a warning that we ran into a memory shortage
problem, and  b) we save a call to ipc->decompress() and some switch
logic in the failing case.   ???

I still think the patch makes sense. Perhaps not for the reasons
initially stated, but it adds an error message for a condition that
people may want to see and it errors out a bit earlier in the error
case.

-- 
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] ISDN: Avoid a potential NULL ptr deref in ippp

[David Rientjes]

On Tue, 21 Nov 2006, Jesper Juhl wrote:

> Ok, I see your point. There may not be an actual bug here, but
> couldn't it still be considered an improvement, given that with my
> patch we'll  a) print a warning that we ran into a memory shortage
> problem, and  b) we save a call to ipc->decompress() and some switch
> logic in the failing case.   ???
> 

No, because there is duplication of code.  This error condition is already 
addressed:

	skb_out = dev_alloc_skb(is->mru + PPP_HDRLEN);
	len = ipc->decompress(stat, skb, skb_out, &rsparm);
	kfree_skb(skb);
	if (len <= 0) {
		switch (len) {
		case DECOMP_ERROR:
			...
			break;
		case DECOMP_FATALERROR:
			...
			break;
		}
		kfree_skb(skb_out);
		return NULL;
	}

Since neither DECOMP_ERROR or DECOMP_FATALERROR represent a NULL return 
from ipc->decompress(), the switch clause is a no-op and skb_out is freed 
and NULL is returned.

The only thing your patch addresses is moving this before the 
ipc->decompress() call and _duplicating_ both the skb free and the return 
code, as well as adding a warning.  The warning is unnecessary because OOM 
killer will be called soon anyway if this condition is ever reached so the 
fact that it was this allocation that could not be satisfied doesn't 
matter.  Likewise, we need the return code from ipc->decompress() to do 
the other error checking involved.

		David

Re: [PATCH] ISDN: Avoid a potential NULL ptr deref in ippp

[Jesper Juhl]

On 21/11/06, David Rientjes <rientjes@cs.washington.edu> wrote:
> On Tue, 21 Nov 2006, Jesper Juhl wrote:
>
> > Ok, I see your point. There may not be an actual bug here, but
> > couldn't it still be considered an improvement, given that with my
> > patch we'll  a) print a warning that we ran into a memory shortage
> > problem, and  b) we save a call to ipc->decompress() and some switch
> > logic in the failing case.   ???
> >
>
> No, because there is duplication of code.  This error condition is already
> addressed:
>
>         skb_out = dev_alloc_skb(is->mru + PPP_HDRLEN);
>         len = ipc->decompress(stat, skb, skb_out, &rsparm);
>         kfree_skb(skb);
>         if (len <= 0) {
>                 switch (len) {
>                 case DECOMP_ERROR:
>                         ...
>                         break;
>                 case DECOMP_FATALERROR:
>                         ...
>                         break;
>                 }
>                 kfree_skb(skb_out);
>                 return NULL;
>         }
>
> Since neither DECOMP_ERROR or DECOMP_FATALERROR represent a NULL return
> from ipc->decompress(), the switch clause is a no-op and skb_out is freed
> and NULL is returned.
>
> The only thing your patch addresses is moving this before the
> ipc->decompress() call and _duplicating_ both the skb free and the return
> code, as well as adding a warning.  The warning is unnecessary because OOM
> killer will be called soon anyway if this condition is ever reached so the
> fact that it was this allocation that could not be satisfied doesn't
> matter.  Likewise, we need the return code from ipc->decompress() to do
> the other error checking involved.
>

You are right. I concede your point.

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