AX.25 and NET/ROM fixes and improvments.
Subsystems (3)
cover AX.25 and NET/ROM fixes and improvments.
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(-)
Signature · 1 line
1.7.4.4
PATCH 1/4 NET: AX.25: Check ioctl arguments to avoid overflows further down the road.
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 <[email protected]> and a first patch was sent by Xi Wang <[email protected]>.
--- net/ax25/af_ax25.c | 17 +++++++++++------ 1 files changed, 11 insertions(+), 6 deletions(-)
net/ax25/af_ax25.c +11 -6
@@ -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 ; }
Signature · 1 line
1.7.4.4
Re: [PATCH 1/4] NET: AX.25: Check ioctl arguments to avoid overflows further down the road.
From: Ralf Baechle <[email protected]>
Date: Thu, 24 Nov 2011 16:12:59 +0000
Quoted 4 lines
Very large, nonsenical arguments or use in very extreme conditions could result in integer overflows. Check ioctls arguments to avoid such overflows and return
-EINVALfor too large arguments.
Quoted 8 lines
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 <[email protected]> and a first patch was sent by Xi Wang <[email protected]>.
Signed-off-by Ralf Baechle <[email protected]>
Applied.
PATCH 2/4 NET: NETROM: When adding a route verify length of mnemonic string.
In reply to Ralf Baechlestruct 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 <[email protected]>.
--- net/netrom/nr_route.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
net/netrom/nr_route.c +5 -0
@@ -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,
Signature · 1 line
1.7.4.4
Re: [PATCH 2/4] NET: NETROM: When adding a route verify length of mnemonic string.
Quoted 5 lines · 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.
Quoted 7 lines
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_NODEto avoid breakingNETROM_NEIGHwhere userland might passing an uninitialized mnemonic field.
Good point... I missed that.
regards,
dan carpenter
Re: [PATCH 2/4] NET: NETROM: When adding a route verify length of mnemonic string.
In reply to Ralf BaechleFrom: Ralf Baechle <[email protected]>
Date: Fri, 25 Nov 2011 09:08:49 +0000
Quoted 9 lines
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.
Quoted 7 lines
Fixed by rejecting mnemonic strings that have no terminating \0 in the first 7 characters. Do this test only
NETROM_NODEto avoid breakingNETROM_NEIGHwhere userland might passing an uninitialized mnemonic field.Initial patch by Dan Carpenter <[email protected]>.
Signed-off-by Ralf Baechle <[email protected]>
Applied.
PATCH 3/4 NET: NETROM: Cleanup argument SIOCADDRT ioctl argument checking.
In reply to Ralf Baechlenr_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.
--- net/netrom/nr_route.c | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-)
net/netrom/nr_route.c +2 -4
@@ -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 ) {
Signature · 1 line
1.7.4.4
Re: [PATCH 3/4] NET: NETROM: Cleanup argument SIOCADDRT ioctl argument checking.
Am 25.11.2011 10:09, schrieb Ralf Baechle:
Quoted 19 lines
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_getcall later allows the nr_route.ndigis test to bail out without having todev_put.Signed-off-by Ralf Baechle <[email protected]>Cc Thomas Osterried <[email protected]>--- net/netrom/nr_route.c | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-)net/netrom/nr_route.c +0
-1@@ -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 )
net/netrom/nr_route.c int nr_rt_ioctl(unsigned int cmd, void __user *arg) · line 679
Quoted 10 lines
net/netrom/nr_route.c +2
-3+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.
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:
Quoted 49 lines
Am 25.11.2011 10:09, schrieb Ralf Baechle:
Quoted 19 lines
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_getcall later allows the nr_route.ndigis test to bail out without having todev_put.Signed-off-by Ralf Baechle <[email protected]>Cc Thomas Osterried <[email protected]>--- net/netrom/nr_route.c | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-)net/netrom/nr_route.c +0
-1@@ -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 )net/netrom/nr_route.c int nr_rt_ioctl(unsigned int cmd, void __user *arg) · line 679
Quoted 10 lines
net/netrom/nr_route.c +2
-3+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_DIGISas 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] NET: NETROM: Cleanup argument SIOCADDRT ioctl argument checking.
Am Freitag, den 25. November 2011 um 13:12:25 Uhr, schrieb walter harms <[email protected]> in <[email protected]>:
Quoted 4 lines
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
Quoted 56 lines
re,
whAm 25.11.2011 12:22, schrieb walter harms:
Quoted 49 lines
Am 25.11.2011 10:09, schrieb Ralf Baechle:
Quoted 19 lines
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_getcall later allows the nr_route.ndigis test to bail out without having todev_put.Signed-off-by Ralf Baechle <[email protected]>Cc Thomas Osterried <[email protected]>--- net/netrom/nr_route.c | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-)net/netrom/nr_route.c +0
-1@@ -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 )net/netrom/nr_route.c int nr_rt_ioctl(unsigned int cmd, void __user *arg) · line 679
Quoted 10 lines
net/netrom/nr_route.c +2
-3+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_DIGISas 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] NET: NETROM: Cleanup argument SIOCADDRT ioctl argument checking.
In reply to Ralf BaechleFrom: Ralf Baechle <[email protected]>
Date: Fri, 25 Nov 2011 09:09:00 +0000
Quoted 5 lines
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_getcall later allows the nr_route.ndigis test to bail out without having todev_put.Signed-off-by Ralf Baechle <[email protected]>
Applied.
PATCH 4/4 NET: NETROM: Fix formatting.
In reply to Ralf BaechleThe Linux coding style wants the return statement on its own line.
--- net/netrom/af_netrom.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
net/netrom/af_netrom.c +2 -1
@@ -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 :
Signature · 1 line
1.7.4.4
Re: [PATCH 4/4] NET: NETROM: Fix formatting.
From: Ralf Baechle <[email protected]>
Date: Fri, 25 Nov 2011 09:54:10 +0000
Quoted 3 lines
The Linux coding style wants the return statement on its own line.
Signed-off-by Ralf Baechle <[email protected]>
Applied.