* [patch] netrom: check that user string is terminated
@ 2011-11-23 6:52 Dan Carpenter
2011-11-23 8:22 ` walter harms
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2011-11-23 6:52 UTC (permalink / raw)
To: Ralf Baechle; +Cc: David S. Miller, linux-hams, netdev, kernel-janitors
We do an strcpy() of mnemonic in nr_add_node().
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/net/netrom/nr_route.c b/net/netrom/nr_route.c
index 915a87b..e126c48 100644
--- a/net/netrom/nr_route.c
+++ b/net/netrom/nr_route.c
@@ -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) {
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [patch] netrom: check that user string is terminated
2011-11-23 6:52 [patch] netrom: check that user string is terminated Dan Carpenter
@ 2011-11-23 8:22 ` walter harms
2011-11-23 19:12 ` Ralf Baechle
0 siblings, 1 reply; 4+ messages in thread
From: walter harms @ 2011-11-23 8:22 UTC (permalink / raw)
To: Dan Carpenter
Cc: Ralf Baechle, David S. Miller, linux-hams, netdev,
kernel-janitors
Am 23.11.2011 07:52, schrieb Dan Carpenter:
> We do an strcpy() of mnemonic in nr_add_node().
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/net/netrom/nr_route.c b/net/netrom/nr_route.c
> index 915a87b..e126c48 100644
> --- a/net/netrom/nr_route.c
> +++ b/net/netrom/nr_route.c
> @@ -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.
> 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;
> 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
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [patch] netrom: check that user string is terminated
2011-11-23 8:22 ` walter harms
@ 2011-11-23 19:12 ` Ralf Baechle
2011-11-23 19:54 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Ralf Baechle @ 2011-11-23 19:12 UTC (permalink / raw)
To: walter harms
Cc: Dan Carpenter, David S. Miller, linux-hams, netdev,
kernel-janitors
On Wed, Nov 23, 2011 at 09:22:16AM +0100, walter harms wrote:
> 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.
> > 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;
>
> > 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [patch] netrom: check that user string is terminated
2011-11-23 19:12 ` Ralf Baechle
@ 2011-11-23 19:54 ` Dan Carpenter
0 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2011-11-23 19:54 UTC (permalink / raw)
To: Ralf Baechle
Cc: walter harms, David S. Miller, linux-hams, netdev,
kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 577 bytes --]
On Wed, Nov 23, 2011 at 07:12:49PM +0000, Ralf Baechle wrote:
> On Wed, Nov 23, 2011 at 09:22:16AM +0100, walter harms wrote:
>
> > 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
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-11-23 19:54 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-11-23 6:52 [patch] netrom: check that user string is terminated Dan Carpenter
2011-11-23 8:22 ` walter harms
2011-11-23 19:12 ` Ralf Baechle
2011-11-23 19:54 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).