[PATCH] ISDN: fix double free bug in isdn_net

[PATCH] ISDN: fix double free bug in isdn_net

[Jesper Juhl]

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

 drivers/isdn/i4l/isdn_net.c |    1 +
 1 files changed, 1 insertion(+)

--- linux-2.6.18-rc4-orig/drivers/isdn/i4l/isdn_net.c	2006-08-11 00:10:55.000000000 +0200
+++ linux-2.6.18-rc4/drivers/isdn/i4l/isdn_net.c	2006-08-12 22:39:56.000000000 +0200
@@ -1023,6 +1023,7 @@ void isdn_net_writebuf_skb(isdn_net_loca
 	if (ret != len) {
 		/* we should never get here */
 		printk(KERN_WARNING "%s: HL driver queue full\n", lp->name);
+		skb = NULL;
 		goto error;
 	}
 	

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

[David Miller]

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

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

[Jesper Juhl]

On 15/08/06, David Miller <davem@davemloft.net> wrote:
> 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);
>

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@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: fix double free bug in isdn_net

[David Miller]

From: "Jesper Juhl" <jesper.juhl@gmail.com>
Date: Tue, 15 Aug 2006 11:08:35 +0200

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

Do we know if the ->writebuf_skb() method ever frees the skb?  If it
never does, then yes your suggestion would be one way to handle this.

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

[Jesper Juhl]

On 15/08/06, David Miller <davem@davemloft.net> wrote:
> From: "Jesper Juhl" <jesper.juhl@gmail.com>
> Date: Tue, 15 Aug 2006 11:08:35 +0200
>
> > 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?
>
> Do we know if the ->writebuf_skb() method ever frees the skb?  If it
> never does, then yes your suggestion would be one way to handle this.
>
I'll look at it tonight and if writebuf_skb() method never frees the
skb I'll send a patch.

-- 
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: fix double free bug in isdn_net

[Karsten Keil]

On Tue, Aug 15, 2006 at 02:15:03AM -0700, David Miller wrote:
> From: "Jesper Juhl" <jesper.juhl@gmail.com>
> Date: Tue, 15 Aug 2006 11:08:35 +0200
> 
> > 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?
> 
> Do we know if the ->writebuf_skb() method ever frees the skb?  If it
> never does, then yes your suggestion would be one way to handle this.


It does if it consumes the skb (then it returns skb->len).
But the skb have not to be freed imediately in this case, it maybe
queued or used until all bytes are written to the physical device.

If it returns any other value the skb is not freed.

This logic came from using skb for transparent data too.
Here it was possible, that the hw driver only take some bytes from the
skb (so it returns < skb->len), then the isdn layer should requeue
the skb so no transparent data get lost.

But this mechanism was never used in drivers, only 3 states:

The driver accept the packet then it is responsible for the skb
and return skb->len or the driver do not accept it (e.g. buffer full,
conntection is going down), then it return 0 and does not free the
skb.

If some internal error in the HW driver occur, it should return a
negative value and it also do not free the skb.
 
-- 
Karsten Keil
SuSE Labs
ISDN development

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

[Jesper Juhl]

On 15/08/06, Karsten Keil <kkeil@suse.de> wrote:
> On Tue, Aug 15, 2006 at 02:15:03AM -0700, David Miller wrote:
> > From: "Jesper Juhl" <jesper.juhl@gmail.com>
> > Date: Tue, 15 Aug 2006 11:08:35 +0200
> >
> > > 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?
> >
> > Do we know if the ->writebuf_skb() method ever frees the skb?  If it
> > never does, then yes your suggestion would be one way to handle this.
>
>
> It does if it consumes the skb (then it returns skb->len).
> But the skb have not to be freed imediately in this case, it maybe
> queued or used until all bytes are written to the physical device.
>
> If it returns any other value the skb is not freed.
>
> This logic came from using skb for transparent data too.
> Here it was possible, that the hw driver only take some bytes from the
> skb (so it returns < skb->len), then the isdn layer should requeue
> the skb so no transparent data get lost.
>
> But this mechanism was never used in drivers, only 3 states:
>
> The driver accept the packet then it is responsible for the skb
> and return skb->len or the driver do not accept it (e.g. buffer full,
> conntection is going down), then it return 0 and does not free the
> skb.
>
> If some internal error in the HW driver occur, it should return a
> negative value and it also do not free the skb.
>

Ok, if I understand you correctly, then there's no actual problem here.
right?

-- 
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: fix double free bug in isdn_net

[Karsten Keil]

On Wed, Aug 16, 2006 at 10:22:46PM +0200, Jesper Juhl wrote:
> On 15/08/06, Karsten Keil <kkeil@suse.de> wrote:
> >On Tue, Aug 15, 2006 at 02:15:03AM -0700, David Miller wrote:
> >> From: "Jesper Juhl" <jesper.juhl@gmail.com>
> >> Date: Tue, 15 Aug 2006 11:08:35 +0200
> >>
> >> > 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?
> >>
> >> Do we know if the ->writebuf_skb() method ever frees the skb?  If it
> >> never does, then yes your suggestion would be one way to handle this.
> >
> >
> >It does if it consumes the skb (then it returns skb->len).
> >But the skb have not to be freed imediately in this case, it maybe
> >queued or used until all bytes are written to the physical device.
> >
> >If it returns any other value the skb is not freed.
> >
> >This logic came from using skb for transparent data too.
> >Here it was possible, that the hw driver only take some bytes from the
> >skb (so it returns < skb->len), then the isdn layer should requeue
> >the skb so no transparent data get lost.
> >
> >But this mechanism was never used in drivers, only 3 states:
> >
> >The driver accept the packet then it is responsible for the skb
> >and return skb->len or the driver do not accept it (e.g. buffer full,
> >conntection is going down), then it return 0 and does not free the
> >skb.
> >
> >If some internal error in the HW driver occur, it should return a
> >negative value and it also do not free the skb.
> >
> 
> Ok, if I understand you correctly, then there's no actual problem here.
> right?
> 

I not aware of any problems in this code (besides it should be cleaned up
and rewritten).

Was here any trigger for your patch ?

-- 
Karsten Keil
SuSE Labs
ISDN development

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

[David Miller]

From: Karsten Keil <kkeil@suse.de>
Date: Wed, 16 Aug 2006 22:39:35 +0200

> Was here any trigger for your patch ?

I think Jesper didn't have a test case, but rather spotted
this visually.

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

[Jesper Juhl]

On 16/08/06, Karsten Keil <kkeil@suse.de> wrote:
> On Wed, Aug 16, 2006 at 10:22:46PM +0200, Jesper Juhl wrote:
> > On 15/08/06, Karsten Keil <kkeil@suse.de> wrote:
> > >On Tue, Aug 15, 2006 at 02:15:03AM -0700, David Miller wrote:
> > >> From: "Jesper Juhl" <jesper.juhl@gmail.com>
> > >> Date: Tue, 15 Aug 2006 11:08:35 +0200
> > >>
> > >> > 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?
> > >>
> > >> Do we know if the ->writebuf_skb() method ever frees the skb?  If it
> > >> never does, then yes your suggestion would be one way to handle this.
> > >
> > >
> > >It does if it consumes the skb (then it returns skb->len).
> > >But the skb have not to be freed imediately in this case, it maybe
> > >queued or used until all bytes are written to the physical device.
> > >
> > >If it returns any other value the skb is not freed.
> > >
> > >This logic came from using skb for transparent data too.
> > >Here it was possible, that the hw driver only take some bytes from the
> > >skb (so it returns < skb->len), then the isdn layer should requeue
> > >the skb so no transparent data get lost.
> > >
> > >But this mechanism was never used in drivers, only 3 states:
> > >
> > >The driver accept the packet then it is responsible for the skb
> > >and return skb->len or the driver do not accept it (e.g. buffer full,
> > >conntection is going down), then it return 0 and does not free the
> > >skb.
> > >
> > >If some internal error in the HW driver occur, it should return a
> > >negative value and it also do not free the skb.
> > >
> >
> > Ok, if I understand you correctly, then there's no actual problem here.
> > right?
> >
>
> I not aware of any problems in this code (besides it should be cleaned up
> and rewritten).
>
> Was here any trigger for your patch ?
>

Well, the trigger for the initial patch was Coverity bug #1060 .
I looked at it and thought it looked valid, so I tried to cook up a
patch for was I believed looked like an actual problem.


Here's what Coverity had to say :

CID: 1060
Checker: USE_AFTER_FREE (help)
File: base/src/linux-2.6/drivers/isdn/i4l/isdn_net.c

...
1006 	void isdn_net_writebuf_skb(isdn_net_local *lp, struct sk_buff *skb)
1007 	{
1008 		int ret;
1009 		int len = skb->len;     /* save len */
1010 	
1011 		/* before obtaining the lock the caller should have checked that
1012 		   the lp isn't busy */
1013 		if (isdn_net_lp_busy(lp)) {
1014 			printk("isdn BUG at %s:%d!\n", __FILE__, __LINE__);
1015 			goto error;
1016 		}
1017 	
1018 		if (!(lp->flags & ISDN_NET_CONNECTED)) {
1019 			printk("isdn BUG at %s:%d!\n", __FILE__, __LINE__);
1020 			goto error;
1021 		}

Event freed_arg: Pointer "skb" freed by function
"isdn_writebuf_skb_stub" [model]
Also see events: [double_free]

1022 		ret = isdn_writebuf_skb_stub(lp->isdn_device, lp->isdn_channel, 1, skb);

At conditional (1): "ret != len" taking true path

1023 		if (ret != len) {
1024 			/* we should never get here */
1025 			printk(KERN_WARNING "%s: HL driver queue full\n", lp->name);
1026 			goto error;
1027 		}
1028 		
1029 		lp->transcount += len;
1030 		isdn_net_inc_frame_cnt(lp);
1031 		return;
1032 	
1033 	 error:

Event double_free: Double free of pointer "skb" in call to "kfree_skb" [model]
Also see events: [freed_arg]

1034 		dev_kfree_skb(skb);
1035 		lp->stats.tx_errors++;
1036 	
1037 	}
...


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