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

[David Miller <davem@davemloft.net>]

From: Jesper Juhl <jesper.juhl@gmail.com>
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@gmail.com>

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

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. :-/