diff mbox

ax25: Fix ax25_cb refcounting in ax25_ctl_ioctl

Message ID 20090925131038.GA14778@ff.dom.local
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Jarek Poplawski Sept. 25, 2009, 1:10 p.m. UTC 
This bug isn't responsible for these oopses here, but looks quite
obviously. (I'm not sure if it's easy to test/hit with the common
tools.)

Jarek P.
------------>
[PATCH] ax25: Fix ax25_cb refcounting in ax25_ctl_ioctl

Use ax25_cb_put after ax25_find_cb in ax25_ctl_ioctl.

Reported-by: Bernard Pidoux F6BVP <f6bvp@free.fr>
Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
---

 net/ax25/af_ax25.c |   27 +++++++++++++++++----------
 1 files changed, 17 insertions(+), 10 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Ralf Baechle Sept. 25, 2009, 1:40 p.m. UTC | #1 
On Fri, Sep 25, 2009 at 01:10:38PM +0000, Jarek Poplawski wrote:

> This bug isn't responsible for these oopses here, but looks quite
> obviously. (I'm not sure if it's easy to test/hit with the common
> tools.)

The issue your patch fixes is obvious enough.

> Jarek P.
> ------------>
> [PATCH] ax25: Fix ax25_cb refcounting in ax25_ctl_ioctl
> 
> Use ax25_cb_put after ax25_find_cb in ax25_ctl_ioctl.
> 
> Reported-by: Bernard Pidoux F6BVP <f6bvp@free.fr>
> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

Reviewed-by: Ralf Baechle <ralf@linux-mips.org>

  Ralf
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarek Poplawski Sept. 25, 2009, 6:35 p.m. UTC | #2 
On Fri, Sep 25, 2009 at 02:40:52PM +0100, Ralf Baechle DL5RB wrote:
> On Fri, Sep 25, 2009 at 01:10:38PM +0000, Jarek Poplawski wrote:
> 
> > This bug isn't responsible for these oopses here, but looks quite
> > obviously. (I'm not sure if it's easy to test/hit with the common
> > tools.)
> 
> The issue your patch fixes is obvious enough.

Yes, with new code there would be no doubt. But here, if you know it's
worked for some time, you wonder if you're not blind. |-)
> 
> > Jarek P.
> > ------------>
> > [PATCH] ax25: Fix ax25_cb refcounting in ax25_ctl_ioctl
> > 
> > Use ax25_cb_put after ax25_find_cb in ax25_ctl_ioctl.
> > 
> > Reported-by: Bernard Pidoux F6BVP <f6bvp@free.fr>
> > Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
> 
> Reviewed-by: Ralf Baechle <ralf@linux-mips.org>
> 
Thanks for reviewing,
Jarek P.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Sept. 25, 2009, 7:10 p.m. UTC | #3 
From: Jarek Poplawski <jarkao2@gmail.com>
Date: Fri, 25 Sep 2009 20:35:04 +0200

> On Fri, Sep 25, 2009 at 02:40:52PM +0100, Ralf Baechle DL5RB wrote:
>> > [PATCH] ax25: Fix ax25_cb refcounting in ax25_ctl_ioctl
>> > 
>> > Use ax25_cb_put after ax25_find_cb in ax25_ctl_ioctl.
>> > 
>> > Reported-by: Bernard Pidoux F6BVP <f6bvp@free.fr>
>> > Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
>> 
>> Reviewed-by: Ralf Baechle <ralf@linux-mips.org>
>> 
> Thanks for reviewing,

Applied.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bernard Pidoux Sept. 25, 2009, 7:50 p.m. UTC | #4 
Hi Jarek,

patch applied to 2.6.31.1 kernel keeping your other AX25 debugging patches.

So far, so good !

Thanks.

Bernard Pidoux


Jarek Poplawski a écrit :
> On Fri, Sep 25, 2009 at 02:40:52PM +0100, Ralf Baechle DL5RB wrote:
>> On Fri, Sep 25, 2009 at 01:10:38PM +0000, Jarek Poplawski wrote:
>>
>>> This bug isn't responsible for these oopses here, but looks quite
>>> obviously. (I'm not sure if it's easy to test/hit with the common
>>> tools.)
>> The issue your patch fixes is obvious enough.
> 
> Yes, with new code there would be no doubt. But here, if you know it's
> worked for some time, you wonder if you're not blind. |-)
>>> Jarek P.
>>> ------------>
>>> [PATCH] ax25: Fix ax25_cb refcounting in ax25_ctl_ioctl
>>>
>>> Use ax25_cb_put after ax25_find_cb in ax25_ctl_ioctl.
>>>
>>> Reported-by: Bernard Pidoux F6BVP <f6bvp@free.fr>
>>> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
>> Reviewed-by: Ralf Baechle <ralf@linux-mips.org>
>>
> Thanks for reviewing,
> Jarek P.
> 

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarek Poplawski Sept. 25, 2009, 8:29 p.m. UTC | #5 
On Fri, Sep 25, 2009 at 09:50:11PM +0200, Bernard Pidoux wrote:
> Hi Jarek,
> 
> patch applied to 2.6.31.1 kernel keeping your other AX25 debugging patches.

Actually, only one of them is for debugging.
> 
> So far, so good !

Very strange...;-) It seems I might have been wrong and the previous
patch could actually fix those oopses. Then it would point at some
problems while starting netrom connections as the main suspect.

Thanks,
Jarek P.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ralf Baechle Sept. 27, 2009, 7:23 a.m. UTC | #6 
On Fri, Sep 25, 2009 at 08:35:04PM +0200, Jarek Poplawski wrote:

> > > This bug isn't responsible for these oopses here, but looks quite
> > > obviously. (I'm not sure if it's easy to test/hit with the common
> > > tools.)
> > 
> > The issue your patch fixes is obvious enough.
> 
> Yes, with new code there would be no doubt. But here, if you know it's
> worked for some time, you wonder if you're not blind. |-)

Most of of the ioctls are used by AX.25 userland which does error
checking on user supplied values so userland will never attempt invalid
ioctl calls.  So no surprise this went unnoticed.

  Ralf
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarek Poplawski Sept. 27, 2009, 5:10 p.m. UTC | #7 
On Sun, Sep 27, 2009 at 08:23:19AM +0100, Ralf Baechle DL5RB wrote:
> On Fri, Sep 25, 2009 at 08:35:04PM +0200, Jarek Poplawski wrote:
> 
> > > > This bug isn't responsible for these oopses here, but looks quite
> > > > obviously. (I'm not sure if it's easy to test/hit with the common
> > > > tools.)
> > > 
> > > The issue your patch fixes is obvious enough.
> > 
> > Yes, with new code there would be no doubt. But here, if you know it's
> > worked for some time, you wonder if you're not blind. |-)
> 
> Most of of the ioctls are used by AX.25 userland which does error
> checking on user supplied values so userland will never attempt invalid
> ioctl calls.  So no surprise this went unnoticed.

In this case valid calls (return 0) were affected too, so I guess the
whole thing is rarely used.

Jarek P.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarek Poplawski Sept. 27, 2009, 7:02 p.m. UTC | #8 
On Sun, Sep 27, 2009 at 07:10:29PM +0200, Jarek Poplawski wrote:
...
> In this case valid calls (return 0) were affected too, so I guess the
> whole thing is rarely used.

And, after all, there would be only a little, invisible memory leak.

Jarek P.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index d6b1b05..fbcac76 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -358,6 +358,7 @@  static int ax25_ctl_ioctl(const unsigned int cmd, void __user *arg)
 	ax25_dev *ax25_dev;
 	ax25_cb *ax25;
 	unsigned int k;
+	int ret = 0;
 
 	if (copy_from_user(&ax25_ctl, arg, sizeof(ax25_ctl)))
 		return -EFAULT;
@@ -388,57 +389,63 @@  static int ax25_ctl_ioctl(const unsigned int cmd, void __user *arg)
 	case AX25_WINDOW:
 		if (ax25->modulus == AX25_MODULUS) {
 			if (ax25_ctl.arg < 1 || ax25_ctl.arg > 7)
-				return -EINVAL;
+				goto einval_put;
 		} else {
 			if (ax25_ctl.arg < 1 || ax25_ctl.arg > 63)
-				return -EINVAL;
+				goto einval_put;
 		}
 		ax25->window = ax25_ctl.arg;
 		break;
 
 	case AX25_T1:
 		if (ax25_ctl.arg < 1)
-			return -EINVAL;
+			goto einval_put;
 		ax25->rtt = (ax25_ctl.arg * HZ) / 2;
 		ax25->t1  = ax25_ctl.arg * HZ;
 		break;
 
 	case AX25_T2:
 		if (ax25_ctl.arg < 1)
-			return -EINVAL;
+			goto einval_put;
 		ax25->t2 = ax25_ctl.arg * HZ;
 		break;
 
 	case AX25_N2:
 		if (ax25_ctl.arg < 1 || ax25_ctl.arg > 31)
-			return -EINVAL;
+			goto einval_put;
 		ax25->n2count = 0;
 		ax25->n2 = ax25_ctl.arg;
 		break;
 
 	case AX25_T3:
 		if (ax25_ctl.arg < 0)
-			return -EINVAL;
+			goto einval_put;
 		ax25->t3 = ax25_ctl.arg * HZ;
 		break;
 
 	case AX25_IDLE:
 		if (ax25_ctl.arg < 0)
-			return -EINVAL;
+			goto einval_put;
 		ax25->idle = ax25_ctl.arg * 60 * HZ;
 		break;
 
 	case AX25_PACLEN:
 		if (ax25_ctl.arg < 16 || ax25_ctl.arg > 65535)
-			return -EINVAL;
+			goto einval_put;
 		ax25->paclen = ax25_ctl.arg;
 		break;
 
 	default:
-		return -EINVAL;
+		goto einval_put;
 	  }
 
-	return 0;
+out_put:
+	ax25_cb_put(ax25);
+	return ret;
+
+einval_put:
+	ret = -EINVAL;
+	goto out_put;
 }
 
 static void ax25_fillin_cb_from_dev(ax25_cb *ax25, ax25_dev *ax25_dev)