Skip to contentSkip to conversation

netrom: check that user string is terminated

PATCH In review Dan Carpenter 3 participants 4 messages Nov 23, 2011

Subsystems (3)
Networking Network drivers Kernel janitors
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 netrom: check that user string is terminated

We do an strcpy() of mnemonic in nr_add_node().

Signed-off-by Dan Carpenter <[email protected]>
net/netrom/nr_route.c +2 -0
index 915a87b..e126c48 100644
@@ -670,6 +670,8 @@ 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 (strlen(nr_route.mnemonic) >= sizeof(nr_route.mnemonic))
+			return -EINVAL;
 		if ((dev = nr_ax25_dev_get(nr_route.device)) = NULL)
 			return -EINVAL;
 		if (nr_route.ndigis < 0 || nr_route.ndigis > AX25_MAX_DIGIS) {
#1 lore raw

Am 23.11.2011 07:52, schrieb Dan Carpenter:

Quoted 9 lines

We do an strcpy() of mnemonic in nr_add_node().

Signed-off-by Dan Carpenter <[email protected]>
net/netrom/nr_route.c +0 -0
index 915a87b..e126c48 100644

net/netrom/nr_route.c int nr_rt_ioctl(unsigned int cmd, void __user *arg) · line 674

Quoted 5 lines
net/netrom/nr_route.c +2 -0
@@ -670,6 +670,8 @@ 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 (strlen(nr_route.mnemonic) >= sizeof(nr_route.mnemonic))
+			return -EINVAL;

I am not sure that it does what you intends.
mnemonic is an array and a malicious use may fill it upto the last char causing strlen go beyond. perhaps this may help:

	nr_route.mnemonic[sizeof(nr_route.mnemonic)-1]=0;

this of cause makes the if() redundant.

net/netrom/nr_route.c int nr_rt_ioctl(unsigned int cmd, void __user *arg) · line 676

Quoted 2 lines
net/netrom/nr_route.c +0 -0
 		if ((dev = nr_ax25_dev_get(nr_route.device)) = NULL)
 			return -EINVAL;

while you are here:

	dev = nr_ax25_dev_get(nr_route.device);
	if ( dev = NULL )
		return -EINVAL;

net/netrom/nr_route.c int nr_rt_ioctl(unsigned int cmd, void __user *arg) · line 677

Quoted 1 line
net/netrom/nr_route.c +0 -0
 		if (nr_route.ndigis < 0 || nr_route.ndigis > AX25_MAX_DIGIS) {

if guess "nr_route.ndigis >= AX25_MAX_DIGIS" is intended ?

hope that helps,
wh

Quoted 5 lines

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

#2 lore raw
Quoted 3 lines · walter harms

I am not sure that it does what you intends.
mnemonic is an array and a malicious use may fill it upto the last char causing strlen go beyond. perhaps this may help:

Correct, it makes thigs worse. I'm going to reply in detail later tonight, have to bail out now.

Quoted 11 lines

net/netrom/nr_route.c int nr_rt_ioctl(unsigned int cmd, void __user *arg) · line 676

Quoted 2 lines
net/netrom/nr_route.c +0 -0
 		if ((dev = nr_ax25_dev_get(nr_route.device)) = NULL)
 			return -EINVAL;

while you are here:

	dev = nr_ax25_dev_get(nr_route.device);
	if ( dev = NULL )
		return -EINVAL;
Quoted 1 line

if (nr_route.ndigis < 0 || nr_route.ndigis > AX25_MAX_DIGIS) {

if guess "nr_route.ndigis >= AX25_MAX_DIGIS" is intended ?

No, values of 0 .. AX25_MAX_DIGIS are permitted with zero meaning no digipeater at all.

The actual bug if you want to call it that in this line is cosmetic - nr_route.ndigis is an unsigned int so nr_route.ndigis < 0 will never become true. There are other simplifications possible to the error checking here. I've whipped a cleanup patch for this part of the code.

Ralf

author
#3 lore raw
Quoted 9 lines · Ralf Baechle
Quoted 3 lines · walter harms

I am not sure that it does what you intends.
mnemonic is an array and a malicious use may fill it upto the last char causing strlen go beyond. perhaps this may help:

Correct, it makes thigs worse. I'm going to reply in detail later tonight, have to bail out now.

Ok. I said in a different thread that I was going to redo these using strnlen() but I'll wait to read your comments.

regards,
dan carpenter