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 <[email protected]>
---
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;
}
From: Jesper Juhl <[email protected]>
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 <[email protected]>
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. :-/
On 15/08/06, David Miller <[email protected]> wrote:
> From: Jesper Juhl <[email protected]>
> 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 <[email protected]>
>
> 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 <[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
From: "Jesper Juhl" <[email protected]>
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.
On 15/08/06, David Miller <[email protected]> wrote:
> From: "Jesper Juhl" <[email protected]>
> 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 <[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, Aug 15, 2006 at 02:15:03AM -0700, David Miller wrote:
> From: "Jesper Juhl" <[email protected]>
> 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
On 15/08/06, Karsten Keil <[email protected]> wrote:
> On Tue, Aug 15, 2006 at 02:15:03AM -0700, David Miller wrote:
> > From: "Jesper Juhl" <[email protected]>
> > 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 <[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 Wed, Aug 16, 2006 at 10:22:46PM +0200, Jesper Juhl wrote:
> On 15/08/06, Karsten Keil <[email protected]> wrote:
> >On Tue, Aug 15, 2006 at 02:15:03AM -0700, David Miller wrote:
> >> From: "Jesper Juhl" <[email protected]>
> >> 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
From: Karsten Keil <[email protected]>
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.
On 16/08/06, Karsten Keil <[email protected]> wrote:
> On Wed, Aug 16, 2006 at 10:22:46PM +0200, Jesper Juhl wrote:
> > On 15/08/06, Karsten Keil <[email protected]> wrote:
> > >On Tue, Aug 15, 2006 at 02:15:03AM -0700, David Miller wrote:
> > >> From: "Jesper Juhl" <[email protected]>
> > >> 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 <[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