[PATCH 0/4] AX.25 and NET/ROM fixes and improvments.

[PATCH 0/4] AX.25 and NET/ROM fixes and improvments.

[Ralf Baechle]

AX.25 ioctl didn't do sufficient argument checking.  The result of these
overflows is harmless as it will be dealt with further further down in the
stack but an application should get an error code when trying to set
such a bogus value.  To not restrict the more extreme use cases of AX.25
there is no attempt to clamp values to a "sensible" range.

The NET/ROM stack's routing ioctl didn't check the lengths of the mnemonic
string that is being passed as part of the nr_route_struct structure to the
kernel.  In theory this could result in an oops but no memory corruption
but again is fairly harmless because it requires CAP_NET_ADMIN priviledges
which in practice only root has and ax25-tools don't send malformed 
ioctls.

Two further patches simplify the checks at the beginning of nr_rt_ioctl
and do minor reformatting to nr_ioctl.

Patches 1 and 2 are meant for v3.2; 3 and 4 are only cosmetic and thus
are v3.3 material.

Ralf Baechle (4):
  NET: AX.25: Check ioctl arguments to avoid overflows further down the
    road.
  NET: NETROM: When adding a route verify length of mnemonic string.
  NET: NETROM: Cleanup argument SIOCADDRT ioctl argument checking.
  NET: NETROM: Fix formatting.

 net/ax25/af_ax25.c     |   17 +++++++++++------
 net/netrom/af_netrom.c |    3 ++-
 net/netrom/nr_route.c  |   11 +++++++----
 3 files changed, 20 insertions(+), 11 deletions(-)

-- 
1.7.4.4

[PATCH 1/4] NET: AX.25: Check ioctl arguments to avoid overflows further down the road.

[Ralf Baechle]

Very large, nonsenical arguments or use in very extreme conditions could
result in integer overflows.  Check ioctls arguments to avoid such
overflows and return -EINVAL for too large arguments.

To allow the use of AX.25 for even the most extreme setup (think packet
radio to the Phase 5E mars probe) we make no further attempt to clamp the
argument range.

Originally reported by Fan Long <longfancn@gmail.com> and a first patch
was sent by Xi Wang <xi.wang@gmail.com>.

Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
Cc: Xi Wang <xi.wang@gmail.com>
Cc: Joerg Reuter <jreuter@yaina.de>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Thomas Osterried <thomas@osterried.de>
---
 net/ax25/af_ax25.c |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index e7c69f4..b863c18 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -402,14 +402,14 @@ static int ax25_ctl_ioctl(const unsigned int cmd, void __user *arg)
 		break;
 
 	case AX25_T1:
-		if (ax25_ctl.arg < 1)
+		if (ax25_ctl.arg < 1 || ax25_ctl.arg > ULONG_MAX / HZ)
 			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)
+		if (ax25_ctl.arg < 1 || ax25_ctl.arg > ULONG_MAX / HZ)
 			goto einval_put;
 		ax25->t2 = ax25_ctl.arg * HZ;
 		break;
@@ -422,10 +422,15 @@ static int ax25_ctl_ioctl(const unsigned int cmd, void __user *arg)
 		break;
 
 	case AX25_T3:
+		if (ax25_ctl.arg > ULONG_MAX / HZ)
+			goto einval_put;
 		ax25->t3 = ax25_ctl.arg * HZ;
 		break;
 
 	case AX25_IDLE:
+		if (ax25_ctl.arg > ULONG_MAX / (60 * HZ))
+			goto einval_put;
+
 		ax25->idle = ax25_ctl.arg * 60 * HZ;
 		break;
 
@@ -571,7 +576,7 @@ static int ax25_setsockopt(struct socket *sock, int level, int optname,
 		break;
 
 	case AX25_T1:
-		if (opt < 1) {
+		if (opt < 1 || opt > ULONG_MAX / HZ) {
 			res = -EINVAL;
 			break;
 		}
@@ -580,7 +585,7 @@ static int ax25_setsockopt(struct socket *sock, int level, int optname,
 		break;
 
 	case AX25_T2:
-		if (opt < 1) {
+		if (opt < 1 || opt > ULONG_MAX / HZ) {
 			res = -EINVAL;
 			break;
 		}
@@ -596,7 +601,7 @@ static int ax25_setsockopt(struct socket *sock, int level, int optname,
 		break;
 
 	case AX25_T3:
-		if (opt < 1) {
+		if (opt < 1 || opt > ULONG_MAX / HZ) {
 			res = -EINVAL;
 			break;
 		}
@@ -604,7 +609,7 @@ static int ax25_setsockopt(struct socket *sock, int level, int optname,
 		break;
 
 	case AX25_IDLE:
-		if (opt < 0) {
+		if (opt < 0 || opt > ULONG_MAX / (60 * HZ)) {
 			res = -EINVAL;
 			break;
 		}
-- 
1.7.4.4

Re: [PATCH 1/4] NET: AX.25: Check ioctl arguments to avoid overflows further down the road.

[David Miller]

From: Ralf Baechle <ralf@linux-mips.org>
Date: Thu, 24 Nov 2011 16:12:59 +0000

> Very large, nonsenical arguments or use in very extreme conditions could
> result in integer overflows.  Check ioctls arguments to avoid such
> overflows and return -EINVAL for too large arguments.
> 
> To allow the use of AX.25 for even the most extreme setup (think packet
> radio to the Phase 5E mars probe) we make no further attempt to clamp the
> argument range.
> 
> Originally reported by Fan Long <longfancn@gmail.com> and a first patch
> was sent by Xi Wang <xi.wang@gmail.com>.
> 
> Signed-off-by: Ralf Baechle <ralf@linux-mips.org>

Applied.

[PATCH 2/4] NET: NETROM: When adding a route verify length of mnemonic string.

[Ralf Baechle]

struct nr_route_struct's mnemonic permits a string of up to 7 bytes to be
used.  If userland passes a not zero terminated string to the kernel adding
a node to the routing table might result in the kernel attempting to read
copy a too long string.

Mnemonic is part of the NET/ROM routing protocol; NET/ROM routing table
updates only broadcast 6 bytes.  The 7th byte in the mnemonic array exists
only as a \0 termination character for the kernel code's convenience.

Fixed by rejecting mnemonic strings that have no terminating \0 in the first
7 characters.  Do this test only NETROM_NODE to avoid breaking NETROM_NEIGH
where userland might passing an uninitialized mnemonic field.

Initial patch by Dan Carpenter <dan.carpenter@oracle.com>.

Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Walter Harms <wharms@bfs.de>
Cc: Thomas Osterried <thomas@osterried.de>
---
 net/netrom/nr_route.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/net/netrom/nr_route.c b/net/netrom/nr_route.c
index 915a87b..8d7716c 100644
--- a/net/netrom/nr_route.c
+++ b/net/netrom/nr_route.c
@@ -678,6 +678,11 @@ int nr_rt_ioctl(unsigned int cmd, void __user *arg)
 		}
 		switch (nr_route.type) {
 		case NETROM_NODE:
+			if (strnlen(nr_route.mnemonic, 7) == 7) {
+				ret = -EINVAL;
+				break;
+			}
+
 			ret = nr_add_node(&nr_route.callsign,
 				nr_route.mnemonic,
 				&nr_route.neighbour,
-- 
1.7.4.4

Re: [PATCH 2/4] NET: NETROM: When adding a route verify length of mnemonic string.

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 897 bytes --]

On Fri, Nov 25, 2011 at 09:08:49AM +0000, Ralf Baechle wrote:
> struct nr_route_struct's mnemonic permits a string of up to 7 bytes to be
> used.  If userland passes a not zero terminated string to the kernel adding
> a node to the routing table might result in the kernel attempting to read
> copy a too long string.
> 
> Mnemonic is part of the NET/ROM routing protocol; NET/ROM routing table
> updates only broadcast 6 bytes.  The 7th byte in the mnemonic array exists
> only as a \0 termination character for the kernel code's convenience.
> 
> Fixed by rejecting mnemonic strings that have no terminating \0 in the first
> 7 characters.  Do this test only NETROM_NODE to avoid breaking NETROM_NEIGH
> where userland might passing an uninitialized mnemonic field.

Good point...  I missed that.

Acked-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/4] NET: NETROM: When adding a route verify length of mnemonic string.

[David Miller]

From: Ralf Baechle <ralf@linux-mips.org>
Date: Fri, 25 Nov 2011 09:08:49 +0000

> struct nr_route_struct's mnemonic permits a string of up to 7 bytes to be
> used.  If userland passes a not zero terminated string to the kernel adding
> a node to the routing table might result in the kernel attempting to read
> copy a too long string.
> 
> Mnemonic is part of the NET/ROM routing protocol; NET/ROM routing table
> updates only broadcast 6 bytes.  The 7th byte in the mnemonic array exists
> only as a \0 termination character for the kernel code's convenience.
> 
> Fixed by rejecting mnemonic strings that have no terminating \0 in the first
> 7 characters.  Do this test only NETROM_NODE to avoid breaking NETROM_NEIGH
> where userland might passing an uninitialized mnemonic field.
> 
> Initial patch by Dan Carpenter <dan.carpenter@oracle.com>.
> 
> Signed-off-by: Ralf Baechle <ralf@linux-mips.org>

Applied.

[PATCH 3/4] NET: NETROM: Cleanup argument SIOCADDRT ioctl argument checking.

[Ralf Baechle]

nr_route.ndigis is unsigned int so the nr_route.ndigis < 0 expression is
never true and can be dropped.  Doing the nr_ax25_dev_get call later
allows the nr_route.ndigis test to bail out without having to dev_put.

Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
Cc: Thomas Osterried <thomas@osterried.de>
---
 net/netrom/nr_route.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/netrom/nr_route.c b/net/netrom/nr_route.c
index 8d7716c..2cf3301 100644
--- a/net/netrom/nr_route.c
+++ b/net/netrom/nr_route.c
@@ -670,12 +670,10 @@ int nr_rt_ioctl(unsigned int cmd, void __user *arg)
 	case SIOCADDRT:
 		if (copy_from_user(&nr_route, arg, sizeof(struct nr_route_struct)))
 			return -EFAULT;
-		if ((dev = nr_ax25_dev_get(nr_route.device)) == NULL)
+		if (nr_route.ndigis > AX25_MAX_DIGIS)
 			return -EINVAL;
-		if (nr_route.ndigis < 0 || nr_route.ndigis > AX25_MAX_DIGIS) {
-			dev_put(dev);
+		if ((dev = nr_ax25_dev_get(nr_route.device)) == NULL)
 			return -EINVAL;
-		}
 		switch (nr_route.type) {
 		case NETROM_NODE:
 			if (strnlen(nr_route.mnemonic, 7) == 7) {
-- 
1.7.4.4

Re: [PATCH 3/4] NET: NETROM: Cleanup argument SIOCADDRT ioctl argument checking.

[walter harms]

Am 25.11.2011 10:09, schrieb Ralf Baechle:
> nr_route.ndigis is unsigned int so the nr_route.ndigis < 0 expression is
> never true and can be dropped.  Doing the nr_ax25_dev_get call later
> allows the nr_route.ndigis test to bail out without having to dev_put.
> 
> Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
> Cc: Thomas Osterried <thomas@osterried.de>
> ---
>  net/netrom/nr_route.c |    6 ++----
>  1 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/net/netrom/nr_route.c b/net/netrom/nr_route.c
> index 8d7716c..2cf3301 100644
> --- a/net/netrom/nr_route.c
> +++ b/net/netrom/nr_route.c
> @@ -670,12 +670,10 @@ int nr_rt_ioctl(unsigned int cmd, void __user *arg)
>  	case SIOCADDRT:
>  		if (copy_from_user(&nr_route, arg, sizeof(struct nr_route_struct)))
>  			return -EFAULT;
> -		if ((dev = nr_ax25_dev_get(nr_route.device)) == NULL)
> +		if (nr_route.ndigis > AX25_MAX_DIGIS)
>  			return -EINVAL;
> -		if (nr_route.ndigis < 0 || nr_route.ndigis > AX25_MAX_DIGIS) {
> -			dev_put(dev);
> +		if ((dev = nr_ax25_dev_get(nr_route.device)) == NULL)
>  			return -EINVAL;
> -		}
>  		switch (nr_route.type) {
>  		case NETROM_NODE:
>  			if (strnlen(nr_route.mnemonic, 7) == 7) {

I realy do not know if that matters but some use AX25_MAX_DIGIS as array
and therefore it should be >=AX25_MAX_DIGIS.

struct rose_route_struct {
         rose_address    address;
        unsigned short  mask;
        ax25_address    neighbour;
        char            device[16];
         unsigned char   ndigis;
         ax25_address    digipeaters[AX25_MAX_DIGIS];
  };

Re: [PATCH 3/4] NET: NETROM: Cleanup argument SIOCADDRT ioctl argument checking.

[walter harms]

hi,
according to LXR there are several places where the check is >AX25_MAX_DIGIS instead of >=.

any takers ?

re,
 wh


Am 25.11.2011 12:22, schrieb walter harms:
> 
> 
> Am 25.11.2011 10:09, schrieb Ralf Baechle:
>> nr_route.ndigis is unsigned int so the nr_route.ndigis < 0 expression is
>> never true and can be dropped.  Doing the nr_ax25_dev_get call later
>> allows the nr_route.ndigis test to bail out without having to dev_put.
>>
>> Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
>> Cc: Thomas Osterried <thomas@osterried.de>
>> ---
>>  net/netrom/nr_route.c |    6 ++----
>>  1 files changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/netrom/nr_route.c b/net/netrom/nr_route.c
>> index 8d7716c..2cf3301 100644
>> --- a/net/netrom/nr_route.c
>> +++ b/net/netrom/nr_route.c
>> @@ -670,12 +670,10 @@ int nr_rt_ioctl(unsigned int cmd, void __user *arg)
>>  	case SIOCADDRT:
>>  		if (copy_from_user(&nr_route, arg, sizeof(struct nr_route_struct)))
>>  			return -EFAULT;
>> -		if ((dev = nr_ax25_dev_get(nr_route.device)) == NULL)
>> +		if (nr_route.ndigis > AX25_MAX_DIGIS)
>>  			return -EINVAL;
>> -		if (nr_route.ndigis < 0 || nr_route.ndigis > AX25_MAX_DIGIS) {
>> -			dev_put(dev);
>> +		if ((dev = nr_ax25_dev_get(nr_route.device)) == NULL)
>>  			return -EINVAL;
>> -		}
>>  		switch (nr_route.type) {
>>  		case NETROM_NODE:
>>  			if (strnlen(nr_route.mnemonic, 7) == 7) {
> 
> I realy do not know if that matters but some use AX25_MAX_DIGIS as array
> and therefore it should be >=AX25_MAX_DIGIS.
> 
> struct rose_route_struct {
>          rose_address    address;
>         unsigned short  mask;
>         ax25_address    neighbour;
>         char            device[16];
>          unsigned char   ndigis;
>          ax25_address    digipeaters[AX25_MAX_DIGIS];
>   };
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hams" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 3/4] NET: NETROM: Cleanup argument SIOCADDRT ioctl argument checking.

[Thomas Osterried]

Am Freitag, den 25. November 2011 um 13:12:25 Uhr, schrieb walter harms <wharms@bfs.de> in <4ECF8629.7000301@bfs.de>:
> hi,
> according to LXR there are several places where the check is >AX25_MAX_DIGIS instead of >=.
> 
> any takers ?


nr_route.ndigis is used at 
  nr_call_to_digi(&digi, nr_route.ndigis, nr_route.digipeaters),

Image nr_route.ndigis is 0.

static ax25_digi *nr_call_to_digi(ax25_digi *digi, int ndigis,
        ax25_address *digipeaters)
{
        int i;

        if (ndigis == 0)
                return NULL;

           ################ here we leave

        for (i = 0; i < ndigis; i++) {
                digi->calls[i]    = digipeaters[i];
                digi->repeated[i] = 0;
        }

        digi->ndigi      = ndigis;
        digi->lastrepeat = -1;

        return digi;
}


Image ndigi is 8 (AX25_MAX_DIGIS), as large as nr_route.digipeaters (because it's digipeaters[AX25_MAX_DIGIS]).

we fill the array from i = 0 to i < ndigis (=7) -> 8 times == sizeof(digipeaters)

-> everything is fine with that.

vy 73,
	- Thomas  dl9sau

> 
> re,
>  wh
> 
> 
> Am 25.11.2011 12:22, schrieb walter harms:
> > 
> > 
> > Am 25.11.2011 10:09, schrieb Ralf Baechle:
> >> nr_route.ndigis is unsigned int so the nr_route.ndigis < 0 expression is
> >> never true and can be dropped.  Doing the nr_ax25_dev_get call later
> >> allows the nr_route.ndigis test to bail out without having to dev_put.
> >>
> >> Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
> >> Cc: Thomas Osterried <thomas@osterried.de>
> >> ---
> >>  net/netrom/nr_route.c |    6 ++----
> >>  1 files changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/net/netrom/nr_route.c b/net/netrom/nr_route.c
> >> index 8d7716c..2cf3301 100644
> >> --- a/net/netrom/nr_route.c
> >> +++ b/net/netrom/nr_route.c
> >> @@ -670,12 +670,10 @@ int nr_rt_ioctl(unsigned int cmd, void __user *arg)
> >>  	case SIOCADDRT:
> >>  		if (copy_from_user(&nr_route, arg, sizeof(struct nr_route_struct)))
> >>  			return -EFAULT;
> >> -		if ((dev = nr_ax25_dev_get(nr_route.device)) == NULL)
> >> +		if (nr_route.ndigis > AX25_MAX_DIGIS)
> >>  			return -EINVAL;
> >> -		if (nr_route.ndigis < 0 || nr_route.ndigis > AX25_MAX_DIGIS) {
> >> -			dev_put(dev);
> >> +		if ((dev = nr_ax25_dev_get(nr_route.device)) == NULL)
> >>  			return -EINVAL;
> >> -		}
> >>  		switch (nr_route.type) {
> >>  		case NETROM_NODE:
> >>  			if (strnlen(nr_route.mnemonic, 7) == 7) {
> > 
> > I realy do not know if that matters but some use AX25_MAX_DIGIS as array
> > and therefore it should be >=AX25_MAX_DIGIS.
> > 
> > struct rose_route_struct {
> >          rose_address    address;
> >         unsigned short  mask;
> >         ax25_address    neighbour;
> >         char            device[16];
> >          unsigned char   ndigis;
> >          ax25_address    digipeaters[AX25_MAX_DIGIS];
> >   };
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-hams" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 

Re: [PATCH 3/4] NET: NETROM: Cleanup argument SIOCADDRT ioctl argument checking.

[David Miller]

From: Ralf Baechle <ralf@linux-mips.org>
Date: Fri, 25 Nov 2011 09:09:00 +0000

> nr_route.ndigis is unsigned int so the nr_route.ndigis < 0 expression is
> never true and can be dropped.  Doing the nr_ax25_dev_get call later
> allows the nr_route.ndigis test to bail out without having to dev_put.
> 
> Signed-off-by: Ralf Baechle <ralf@linux-mips.org>

Applied.

[PATCH 4/4] NET: NETROM: Fix formatting.

[Ralf Baechle]

The Linux coding style wants the return statement on its own line.

Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
---
 net/netrom/af_netrom.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/net/netrom/af_netrom.c b/net/netrom/af_netrom.c
index 732152f..c329b47 100644
--- a/net/netrom/af_netrom.c
+++ b/net/netrom/af_netrom.c
@@ -1244,7 +1244,8 @@ static int nr_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
 	case SIOCADDRT:
 	case SIOCDELRT:
 	case SIOCNRDECOBS:
-		if (!capable(CAP_NET_ADMIN)) return -EPERM;
+		if (!capable(CAP_NET_ADMIN))
+			return -EPERM;
 		return nr_rt_ioctl(cmd, argp);
 
 	default:
-- 
1.7.4.4

Re: [PATCH 4/4] NET: NETROM: Fix formatting.

[David Miller]

From: Ralf Baechle <ralf@linux-mips.org>
Date: Fri, 25 Nov 2011 09:54:10 +0000

> The Linux coding style wants the return statement on its own line.
> 
> Signed-off-by: Ralf Baechle <ralf@linux-mips.org>

Applied.