Messages in this thread | | | Date | Tue, 21 Nov 2006 23:21:32 +0100 | From | "Jesper Juhl" <> | Subject | Re: [PATCH] ISDN: Avoid a potential NULL ptr deref in ippp |
| |
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |