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 <[email protected]>
---
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) {
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
Checks for NULL dev_alloc_skb() and returns on true to avoid subsequent
dereference.
Cc: Jeff Garzik <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Signed-off-by: David Rientjes <[email protected]>
---
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);
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 <[email protected]>
Signed-off-by: David Rientjes <[email protected]>
---
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]);
David Rientjes wrote:
> Checks for NULL dev_alloc_skb() and returns on true to avoid subsequent
> dereference.
>
> Cc: Jeff Garzik <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Signed-off-by: David Rientjes <[email protected]>
applied
Any reason why we can't apply the patch below?
On 30/10/06, Jesper Juhl <[email protected]> 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 <[email protected]>
> ---
>
> 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 <[email protected]>
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
On Tue, 21 Nov 2006, Jesper Juhl wrote:
> Any reason why we can't apply the patch below?
>
> On 30/10/06, Jesper Juhl <[email protected]> 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 <[email protected]>
> > ---
> >
> > 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
On 21/11/06, David Rientjes <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > ---
> > >
> > > 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 <[email protected]>
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
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
On 21/11/06, David Rientjes <[email protected]> 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 <[email protected]>
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