Re: [PATCH] ISDN: fix double free bug in isdn_net

[Jesper Juhl]

On 15/08/06, David Miller <davem@xxxxxxxxxxxxx> wrote:
> From: Jesper Juhl <jesper.juhl@xxxxxxxxx>
> Date: Sat, 12 Aug 2006 22:48:22 +0200
>
> > There's double-free bug in
> > drivers/isdn/i4l/isdn_net.c::isdn_net_writebuf_skb().
> >
> > If isdn_writebuf_skb_stub() doesn't handle the entire skb, then it will
> > have freed the skb that was passed to it and when the code then jumps
> > to the error label it'll result in a double free of the skb.
> >
> > The easy way to fix this is to insert an assignment of skb = NULL in the
> > 'if' following the call to isdn_writebuf_skb_stub() so that when the code
> > at the error label calls dev_kfree_skb(skb); the skb will be NULL and
> > nothing will happen since dev_kfree_skb() just does a return if passed a
> > NULL.
> >
> > Signed-off-by: Jesper Juhl <jesper.juhl@xxxxxxxxx>
>
> I can't ascertain that this is exactly true.
>
> For example, in isdn_writebuf_skb_stub() we have this:
>
>         if (ret > 0) {
>  ...
>                 if (dev->v110[idx]) {
>  ...
>                         if (ret == skb->len)
>                                 dev_kfree_skb(skb);

Hmm, perhaps I made a mistake and missed a path. Maybe it would be
better to fix if by making isdn_writebuf_skb_stub() always set the skb
to NULL when it does free it. That would add a few more assignments
but should ensure the right result always.
What do you say?

> So if ret > 0 and we're doing V.110, we only free the skb
> if isdn_v110_encode put a value at *((int *)nskb->data)
> equal to skb->len.
>
> So in the V.110 case it's not purely the ->writebuf_skb() return
> value that gets returned, rather we return that integer that
> isdn_v110_encode() put there.
>
> Therefore I don't think this SKB leak is %100 correct.  What did
> I miss?
>
> BTW, this ISDN code is rotting a bit and could use some cleanups if
> not a rewrite. :-/
A rewrite would be beyond me, but I try to clean up bits and pieces...

--
Jesper Juhl <jesper.juhl@xxxxxxxxx>
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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/