Skip to contentSkip to conversation

decnet: incorrect optlen size

PATCH In review Roel Kluin 3 participants 3 messages Jan 29, 2009 – Jan 30, 2009

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Roel Kluin

author
#0 lore raw

PATCH 1/1 decnet: incorrect optlen size

Several functions with something like this occur:

int sock_set_foo(int optlen, ...)

{
	struct food foo;

	if (optlen < sizeof(foo))
		return -EINVAL;

	if (copy_from_user(&foo, optval, sizeof(foo)))
		return -EFAULT;
	...
}

see for instance:
grep -C5 -E -R -n "copy_from_user\(&([a-zA-Z0-9]*), optval, sizeof\(\1\)\)" net

but in __dn_setsockopt, below, the checks are slightly different. Should maybe the changes below be apllied?

-------------->8----------------8<----------------------- fix size checks before copy_from_user

Signed-off-by Roel Kluin <[email protected]>

---

net/decnet/af_decnet.c +2 -2
index cf0e184..45b9199 100644
@@ -1359,10 +1359,10 @@ static int __dn_setsockopt(struct socket *sock, int level,int optname, char __us
 	if (optlen && !optval)
 		return -EINVAL;
 
-	if (optlen > sizeof(u))
+	if (optlen < sizeof(u))
 		return -EINVAL;
 
-	if (copy_from_user(&u, optval, optlen))
+	if (copy_from_user(&u, optval, sizeof(u)))
 		return -EFAULT;
 
 	switch(optname) {
#1 lore raw

Hi,

Quoted 30 lines · Roel Kluin

Several functions with something like this occur:

int sock_set_foo(int optlen, ...)

{
	struct food foo;

	if (optlen < sizeof(foo))
		return -EINVAL;

	if (copy_from_user(&foo, optval, sizeof(foo)))
		return -EFAULT;
	...
}

see for instance:
grep -C5 -E -R -n "copy_from_user\(&([a-zA-Z0-9]*), optval, sizeof\(\1\)\)" net

but in __dn_setsockopt, below, the checks are slightly different. Should maybe the changes below be apllied?

-------------->8----------------8<----------------------- fix size checks before copy_from_user

Signed-off-by Roel Kluin <[email protected]>

---

net/decnet/af_decnet.c +0 -0
index cf0e184..45b9199 100644

net/decnet/af_decnet.c static int __dn_setsockopt(struct socket *sock, int level,int optname, char __us · line 1364

Quoted 7 lines
net/decnet/af_decnet.c +1 -1
@@ -1359,10 +1359,10 @@ static int __dn_setsockopt(struct socket *sock, int level,int optname, char __us
 	if (optlen && !optval)
 		return -EINVAL;
 
-	if (optlen > sizeof(u))
+	if (optlen < sizeof(u))
 		return -EINVAL;

I don't see that this makes sense... we want to ensure that the passed length is less than the size of the union which we are going to use as a buffer.

net/decnet/af_decnet.c static int __dn_setsockopt(struct socket *sock, int level,int optname, char __us · line 1367

Quoted 4 lines
net/decnet/af_decnet.c +1 -1
-	if (copy_from_user(&u, optval, optlen))
+	if (copy_from_user(&u, optval, sizeof(u)))
 		return -EFAULT;

... and here we only want to copy the amount of data that has actually
been supplied, not the whole buffer size since in many cases the amount of data is less than the total buffer size.

The only sensible addition that I can see, would be to zero out the buffer before the copy to ensure that we don't land up using data from the stack in the case that the supplied data is less than that required for a particular command.

Steve.

maintainer
#2 lore raw

From: [email protected]
Date: Thu, 29 Jan 2009 08:58:58 +0000

Quoted 13 lines · Roel Kluin

net/decnet/af_decnet.c static int __dn_setsockopt(struct socket *sock, int level,int optname, char __us · line 1364

Quoted 8 lines
net/decnet/af_decnet.c +1 -1
@@ -1359,10 +1359,10 @@ static int __dn_setsockopt(struct socket *sock, int level,int optname, char __us
 	if (optlen && !optval)
 		return -EINVAL;
 
-	if (optlen > sizeof(u))
+	if (optlen < sizeof(u))
 		return -EINVAL;

I don't see that this makes sense... we want to ensure that the passed length is less than the size of the union which we are going to use as a buffer.

Quoted 7 lines

net/decnet/af_decnet.c static int __dn_setsockopt(struct socket *sock, int level,int optname, char __us · line 1367

Quoted 4 lines
net/decnet/af_decnet.c +1 -1
-	if (copy_from_user(&u, optval, optlen))
+	if (copy_from_user(&u, optval, sizeof(u)))
 		return -EFAULT;

... and here we only want to copy the amount of data that has actually
been supplied, not the whole buffer size since in many cases the amount of data is less than the total buffer size.

This code is fine as-is, every single case statement below this code makes an explicit optlen equality check before deferencing any member of the union object we copy into.

I'm therefore dropping this patch.