Skip to contentSkip to conversation

IPV6: Check length of optval provided by user in setsockopt()

PATCH Applied Wang Chen 2 participants 7 messages Mar 31, 2008 – Apr 11, 2008

Subsystems (3)
Networking Network drivers IPv4/IPv6
lore
Share & tools
lore

Wang Chen

author
#0 lore raw

PATCH 1/1 IPV6: Check length of optval provided by user in setsockopt()

Check length of setsockopt's optval, which provided by user, before copy it from user space.

Signed-off-by Wang Chen <[email protected]>
---
 net/ipv6/ipv6_sockglue.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)
net/ipv6/ipv6_sockglue.c +6 -0
index bf2a686..1ad0ac9 100644
@@ -557,6 +557,9 @@ done:
 	{
 		struct ipv6_mreq mreq;
 
+		if (optlen != sizeof(struct ipv6_mreq))
+			goto e_inval;
+
 		retv = -EPROTO;
 		if (inet_sk(sk)->is_icsk)
 			break;

@@ -595,6 +598,9 @@ done:
 		struct group_req greq;
 		struct sockaddr_in6 *psin6;
 
+		if (optlen != sizeof(struct group_req))
+			goto e_inval;
+
 		retv = -EFAULT;
 		if (copy_from_user(&greq, optval, sizeof(struct group_req)))
 			break;
Signature · 1 line
1.5.3.4
author
#2 lore raw

YOSHIFUJI Hideaki / 吉藤英明 said the following on 2008-3-31 17:15:

Quoted 7 lines

In article <[email protected]> (at Mon, 31 Mar 2008 17:08:38 +0800), Wang Chen <[email protected]> says:

Quoted 2 lines

Check length of setsockopt's optval, which provided by user, before copy it from user space.

Please use "<" not "==".

There are two reasons that why I only check whether len of optval is equal to that one in user space.
1. We currently copy the entire structure from user to kernel.
2. In other cases, such as IPV6_LEAVE/JOIN_ANYCAST, we check whether the len is equal to struct's len.

So, Yoshifuji-san, can you tell me more about why we should use "<"? :)

--WCN

#3 lore raw

In article <[email protected]> (at Mon, 31 Mar 2008 18:46:25 +0800), Wang Chen <[email protected]> says:

Quoted 16 lines

YOSHIFUJI Hideaki / 吉藤英明 said the following on 2008-3-31 17:15:

Quoted 7 lines

In article <[email protected]> (at Mon, 31 Mar 2008 17:08:38 +0800), Wang Chen <[email protected]> says:

Quoted 2 lines

Check length of setsockopt's optval, which provided by user, before copy it from user space.

Please use "<" not "==".

There are two reasons that why I only check whether len of optval is equal to that one in user space.
1. We currently copy the entire structure from user to kernel.
2. In other cases, such as IPV6_LEAVE/JOIN_ANYCAST, we check whether the len is equal to struct's len.

Quoted 2 lines

So, Yoshifuji-san, can you tell me more about why we should use "<"? :)

POSIX, as we have some comments in that file ;-). If it is shorter than expected, -EINVAL, otherwise, no error. We need to fix other sites as well.

--yoshfuji

author
#4 lore raw

YOSHIFUJI Hideaki / 吉藤英明 said the following on 2008-3-31 18:53:

Quoted 11 lines
Quoted 11 lines
Quoted 4 lines
Quoted 2 lines

Check length of setsockopt's optval, which provided by user, before copy it from user space.

Please use "<" not "==".

There are two reasons that why I only check whether len of optval is equal to that one in user space.
1. We currently copy the entire structure from user to kernel.
2. In other cases, such as IPV6_LEAVE/JOIN_ANYCAST, we check whether the len is equal to struct's len.

Quoted 7 lines
Quoted 2 lines

So, Yoshifuji-san, can you tell me more about why we should use "<"? :)

POSIX, as we have some comments in that file ;-). If it is shorter than expected, -EINVAL, otherwise, no error. We need to fix other sites as well.

ooh, I see.
I will take a business trip to Japan tomorrow and I will finish this patch since I come back weekend.
Thank you for your explanation.

--WCN

author
#5 lore raw
In reply to YOSHIFUJI Hideaki / 吉藤英明

YOSHIFUJI Hideaki said the following on 2008-3-31 18:53:

Quoted 4 lines

POSIX, as we have some comments in that file ;-). If it is shorter than expected, -EINVAL, otherwise, no error. We need to fix other sites as well.

Check length of setsockopt's optval, which provided by user, before copy it from user space.
For POSIX compliant, return -EINVAL for setsockopt of short lengths.

Signed-off-by Wang Chen <[email protected]>
---
 net/ipv6/ipv6_sockglue.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)
net/ipv6/ipv6_sockglue.c +8 -2
index bf2a686..a5ac121 100644
@@ -557,6 +557,9 @@ done:
 	{
 		struct ipv6_mreq mreq;
 
+		if (optlen < sizeof(struct ipv6_mreq))
+			goto e_inval;
+
 		retv = -EPROTO;
 		if (inet_sk(sk)->is_icsk)
 			break;

@@ -576,7 +579,7 @@ done:
 	{
 		struct ipv6_mreq mreq;
 
-		if (optlen != sizeof(struct ipv6_mreq))
+		if (optlen < sizeof(struct ipv6_mreq))
 			goto e_inval;
 
 		retv = -EFAULT;

@@ -595,6 +598,9 @@ done:
 		struct group_req greq;
 		struct sockaddr_in6 *psin6;
 
+		if (optlen < sizeof(struct group_req))
+			goto e_inval;
+
 		retv = -EFAULT;
 		if (copy_from_user(&greq, optval, sizeof(struct group_req)))
 			break;

@@ -619,7 +625,7 @@ done:
 		struct group_source_req greqs;
 		int omode, add;
 
-		if (optlen != sizeof(struct group_source_req))
+		if (optlen < sizeof(struct group_source_req))
 			goto e_inval;
 		if (copy_from_user(&greqs, optval, sizeof(greqs))) {
 			retv = -EFAULT;
Signature · 1 line
1.5.4
Applied
#6 lore raw

In article <[email protected]> (at Mon, 07 Apr 2008 09:42:07 +0800), Wang Chen <[email protected]> says:

Quoted 11 lines

YOSHIFUJI Hideaki said the following on 2008-3-31 18:53:

Quoted 4 lines

POSIX, as we have some comments in that file ;-). If it is shorter than expected, -EINVAL, otherwise, no error. We need to fix other sites as well.

Check length of setsockopt's optval, which provided by user, before copy it from user space.
For POSIX compliant, return -EINVAL for setsockopt of short lengths.

Signed-off-by Wang Chen <[email protected]>

Applied, thanks.

--yoshfuji