tun: signedness bug in tun_get_user()

PATCH Applied Dan Carpenter 4 participants 7 messages Aug 15, 2013

Subsystems (4)

Networking Network drivers Kernel janitors TUN/TAP driver

lore

Share & tools

lore

Dan Carpenter

author

2013-08-15 12:52 UTC #0 lore raw

PATCH 1/1 tun: signedness bug in tun_get_user()

The recent fix d9bf5f1309 "tun: compare with 0 instead of total_len" is not totally correct. Because "len" and "sizeof()" are size_t type, that means they are never less than zero.

Signed-off-by Dan Carpenter <[email protected]>

drivers/net/tun.c +4 -2

index af987f0..7ed13cc 100644
@@ -977,8 +977,9 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 	u32 rxhash;
 
 	if (!(tun->flags & TUN_NO_PI)) {
-		if ((len -= sizeof(pi)) < 0)
+		if (len < sizeof(pi))
 			return -EINVAL;
+		len -= sizeof(pi);
 
 		if (memcpy_fromiovecend((void *)&pi, iv, 0, sizeof(pi)))
 			return -EFAULT;

@@ -986,8 +987,9 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 	}
 
 	if (tun->flags & TUN_VNET_HDR) {
-		if ((len -= tun->vnet_hdr_sz) < 0)
+		if (len < tun->vnet_hdr_sz)
 			return -EINVAL;
+		len -= tun->vnet_hdr_sz;
 
 		if (memcpy_fromiovecend((void *)&gso, iv, offset, sizeof(gso)))
 			return -EFAULT;

Michael S. Tsirkin

Acked-by on v1 1/1

2013-08-15 14:03 UTC #1 lore raw

Quoted 5 lines · Dan Carpenter

The recent fix d9bf5f1309 "tun: compare with 0 instead of total_len" is not totally correct. Because "len" and "sizeof()" are size_t type, that means they are never less than zero.
Signed-off-by Dan Carpenter <[email protected]>

Acked-by Michael S. Tsirkin <[email protected]>

Quoted 27 lines

drivers/net/tun.c +4 -2

index af987f0..7ed13cc 100644
@@ -977,8 +977,9 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 	u32 rxhash;
 
 	if (!(tun->flags & TUN_NO_PI)) {
-		if ((len -= sizeof(pi)) < 0)
+		if (len < sizeof(pi))
 			return -EINVAL;
+		len -= sizeof(pi);
 
 		if (memcpy_fromiovecend((void *)&pi, iv, 0, sizeof(pi)))
 			return -EFAULT;

@@ -986,8 +987,9 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 	}
 
 	if (tun->flags & TUN_VNET_HDR) {
-		if ((len -= tun->vnet_hdr_sz) < 0)
+		if (len < tun->vnet_hdr_sz)
 			return -EINVAL;
+		len -= tun->vnet_hdr_sz;
 
 		if (memcpy_fromiovecend((void *)&gso, iv, offset, sizeof(gso)))
 			return -EFAULT;

Michael S. Tsirkin

2013-08-15 14:57 UTC #2 lore raw

Quoted 8 lines · Michael S. Tsirkin

Quoted 5 lines · Dan Carpenter
The recent fix d9bf5f1309 "tun: compare with 0 instead of total_len" is not totally correct. Because "len" and "sizeof()" are size_t type, that means they are never less than zero.
Signed-off-by Dan Carpenter <[email protected]>
Acked-by Michael S. Tsirkin <[email protected]>

Alternatively how about we revert the original patch? This is not the only issue it introduced and it doesn't actually fix any bugs.

Quoted 27 lines

drivers/net/tun.c +4 -2

index af987f0..7ed13cc 100644
@@ -977,8 +977,9 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 	u32 rxhash;
 
 	if (!(tun->flags & TUN_NO_PI)) {
-		if ((len -= sizeof(pi)) < 0)
+		if (len < sizeof(pi))
 			return -EINVAL;
+		len -= sizeof(pi);
 
 		if (memcpy_fromiovecend((void *)&pi, iv, 0, sizeof(pi)))
 			return -EFAULT;

@@ -986,8 +987,9 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 	}
 
 	if (tun->flags & TUN_VNET_HDR) {
-		if ((len -= tun->vnet_hdr_sz) < 0)
+		if (len < tun->vnet_hdr_sz)
 			return -EINVAL;
+		len -= tun->vnet_hdr_sz;
 
 		if (memcpy_fromiovecend((void *)&gso, iv, offset, sizeof(gso)))
 			return -EFAULT;

Michael S. Tsirkin

2013-08-15 15:00 UTC #3 lore raw

Quoted 33 lines · Michael S. Tsirkin

Quoted 8 lines · Michael S. Tsirkin
Quoted 5 lines · Dan Carpenter
The recent fix d9bf5f1309 "tun: compare with 0 instead of total_len" is not totally correct. Because "len" and "sizeof()" are size_t type, that means they are never less than zero.
Signed-off-by Dan Carpenter <[email protected]>
Acked-by Michael S. Tsirkin <[email protected]>
Alternatively how about we revert the original patch? This is not the only issue it introduced and it doesn't actually fix any bugs.
Quoted 19 lines
Quoted 19 lines
drivers/net/tun.c +2 -1
index af987f0..7ed13cc 100644
@@ -977,8 +977,9 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 	u32 rxhash;
 
 	if (!(tun->flags & TUN_NO_PI)) {
-		if ((len -= sizeof(pi)) < 0)
+		if (len < sizeof(pi))
 			return -EINVAL;
+		len -= sizeof(pi);
 
 		if (memcpy_fromiovecend((void *)&pi, iv, 0, sizeof(pi)))
 			return -EFAULT;
@@ -986,8 +987,9 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 	}
 

Quoted 4 lines

Quoted 4 lines
drivers/net/tun.c static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, · line 991
Quoted 4 lines
drivers/net/tun.c +1 -1
 	if (tun->flags & TUN_VNET_HDR) {
-		if ((len -= tun->vnet_hdr_sz) < 0)
+		if (len < tun->vnet_hdr_sz)
 			return -EINVAL;

And to be even more explicit, this still doesn't handle the case vnet_hdr_sz < 0 properly.

Quoted 4 lines

drivers/net/tun.c +1 -0

+		len -= tun->vnet_hdr_sz;
 
 		if (memcpy_fromiovecend((void *)&gso, iv, offset, sizeof(gso)))
 			return -EFAULT;

Michael S. Tsirkin

2013-08-15 15:05 UTC #4 lore raw

Quoted 34 lines · Michael S. Tsirkin

Quoted 33 lines · Michael S. Tsirkin
Quoted 8 lines · Michael S. Tsirkin
Quoted 5 lines · Dan Carpenter
The recent fix d9bf5f1309 "tun: compare with 0 instead of total_len" is not totally correct. Because "len" and "sizeof()" are size_t type, that means they are never less than zero.
Signed-off-by Dan Carpenter <[email protected]>
Acked-by Michael S. Tsirkin <[email protected]>
Alternatively how about we revert the original patch? This is not the only issue it introduced and it doesn't actually fix any bugs.
Quoted 19 lines
Quoted 19 lines
drivers/net/tun.c +2 -1
index af987f0..7ed13cc 100644
@@ -977,8 +977,9 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 	u32 rxhash;
 
 	if (!(tun->flags & TUN_NO_PI)) {
-		if ((len -= sizeof(pi)) < 0)
+		if (len < sizeof(pi))
 			return -EINVAL;
+		len -= sizeof(pi);
 
 		if (memcpy_fromiovecend((void *)&pi, iv, 0, sizeof(pi)))
 			return -EFAULT;
@@ -986,8 +987,9 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 	}
 

Quoted 7 lines

Quoted 4 lines
Quoted 4 lines
drivers/net/tun.c static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, · line 991
Quoted 4 lines
drivers/net/tun.c +1 -1
 	if (tun->flags & TUN_VNET_HDR) {
-		if ((len -= tun->vnet_hdr_sz) < 0)
+		if (len < tun->vnet_hdr_sz)
 			return -EINVAL;
And to be even more explicit, this still doesn't handle the case vnet_hdr_sz < 0 properly.

Hmm ENOCOFFEE.
User can't make vnet_hdr_sz < 0 - we already catch that. So let's apply Dan's patch, it does fix all issues after all. Sorry about the noise.

Quoted 5 lines

drivers/net/tun.c +1 -0

+		len -= tun->vnet_hdr_sz;
 
 		if (memcpy_fromiovecend((void *)&gso, iv, offset, sizeof(gso)))
 			return -EFAULT;

Neil Horman

Acked-by on v1 1/1

2013-08-15 20:08 UTC #5 lore raw

In reply to Dan Carpenter

Quoted 23 lines · Dan Carpenter

The recent fix d9bf5f1309 "tun: compare with 0 instead of total_len" is not totally correct. Because "len" and "sizeof()" are size_t type, that means they are never less than zero.
Signed-off-by Dan Carpenter <[email protected]>
drivers/net/tun.c +2 -1
index af987f0..7ed13cc 100644
@@ -977,8 +977,9 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 	u32 rxhash;
 
 	if (!(tun->flags & TUN_NO_PI)) {
-		if ((len -= sizeof(pi)) < 0)
+		if (len < sizeof(pi))
 			return -EINVAL;
+		len -= sizeof(pi);
 
 		if (memcpy_fromiovecend((void *)&pi, iv, 0, sizeof(pi)))
 			return -EFAULT;

drivers/net/tun.c static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile, · line 995

Quoted 10 lines

drivers/net/tun.c +2 -1

@@ -986,8 +987,9 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 	}
 
 	if (tun->flags & TUN_VNET_HDR) {
-		if ((len -= tun->vnet_hdr_sz) < 0)
+		if (len < tun->vnet_hdr_sz)
 			return -EINVAL;
+		len -= tun->vnet_hdr_sz;
 
 		if (memcpy_fromiovecend((void *)&gso, iv, offset, sizeof(gso)))
 			return -EFAULT;

Acked-by Neil Horman <[email protected]>

David Miller

maintainer Applied

2013-08-15 21:51 UTC #6 lore raw

From: Neil Horman <[email protected]>
Date: Thu, 15 Aug 2013 16:08:35 -0400

Quoted 6 lines · Dan Carpenter

Quoted 5 lines
The recent fix d9bf5f1309 "tun: compare with 0 instead of total_len" is not totally correct. Because "len" and "sizeof()" are size_t type, that means they are never less than zero.
Signed-off-by Dan Carpenter <[email protected]>

...

Quoted 1 line

Acked-by Neil Horman <[email protected]>

Applied, thanks everyone.