tun: bail out from tun_get_user() if the skb is empty

PATCH Superseded Alexander Potapenko 3 participants 6 messages Sep 27, 2017

Subsystems (3)

Networking Network drivers TUN/TAP driver

lore

Share & tools

lore

Alexander Potapenko

author

2017-09-27 12:16 UTC #0 lore raw

PATCH v2 1/1 tun: bail out from tun_get_user() if the skb is empty

KMSAN (https://github.com/google/kmsan) reported accessing uninitialized skb->data[0] in the case the skb is empty (i.e. skb->len is 0):

================================================

BUG: KMSAN: use of uninitialized memory in tun_get_user+0x19ba/0x3770
CPU: 0 PID: 3051 Comm: probe Not tainted 4.13.0+ #3140
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
Call Trace:
...
 __msan_warning_32+0x66/0xb0 mm/kmsan/kmsan_instr.c:477
 tun_get_user+0x19ba/0x3770 drivers/net/tun.c:1301
 tun_chr_write_iter+0x19f/0x300 drivers/net/tun.c:1365
 call_write_iter ./include/linux/fs.h:1743
 new_sync_write fs/read_write.c:457
 __vfs_write+0x6c3/0x7f0 fs/read_write.c:470
 vfs_write+0x3e4/0x770 fs/read_write.c:518
 SYSC_write+0x12f/0x2b0 fs/read_write.c:565
 SyS_write+0x55/0x80 fs/read_write.c:557
 do_syscall_64+0x242/0x330 arch/x86/entry/common.c:284
 entry_SYSCALL64_slow_path+0x25/0x25 arch/x86/entry/entry_64.S:245
...
origin:
...
 kmsan_poison_shadow+0x6e/0xc0 mm/kmsan/kmsan.c:211
 slab_alloc_node mm/slub.c:2732
 __kmalloc_node_track_caller+0x351/0x370 mm/slub.c:4351
 __kmalloc_reserve net/core/skbuff.c:138
 __alloc_skb+0x26a/0x810 net/core/skbuff.c:231
 alloc_skb ./include/linux/skbuff.h:903
 alloc_skb_with_frags+0x1d7/0xc80 net/core/skbuff.c:4756
 sock_alloc_send_pskb+0xabf/0xfe0 net/core/sock.c:2037
 tun_alloc_skb drivers/net/tun.c:1144
 tun_get_user+0x9a8/0x3770 drivers/net/tun.c:1274
 tun_chr_write_iter+0x19f/0x300 drivers/net/tun.c:1365
 call_write_iter ./include/linux/fs.h:1743
 new_sync_write fs/read_write.c:457
 __vfs_write+0x6c3/0x7f0 fs/read_write.c:470
 vfs_write+0x3e4/0x770 fs/read_write.c:518
 SYSC_write+0x12f/0x2b0 fs/read_write.c:565
 SyS_write+0x55/0x80 fs/read_write.c:557
 do_syscall_64+0x242/0x330 arch/x86/entry/common.c:284
 return_from_SYSCALL_64+0x0/0x6a arch/x86/entry/entry_64.S:245
================================================

Make sure tun_get_user() doesn't touch skb->data[0] unless there is actual data.

C reproducer below:
==========================

    // autogenerated by syzkaller (http://github.com/google/syzkaller)

    #define _GNU_SOURCE

    #include <fcntl.h>
    #include <linux/if_tun.h>
    #include <netinet/ip.h>
    #include <net/if.h>
    #include <string.h>
    #include <sys/ioctl.h>

    int main()
    {
      int sock = socket(PF_INET, SOCK_STREAM, IPPROTO_IP);
      int tun_fd = open("/dev/net/tun", O_RDWR);
      struct ifreq req;
      memset(&req, 0, sizeof(struct ifreq));
      strcpy((char*)&req.ifr_name, "gre0");
      req.ifr_flags = IFF_UP | IFF_MULTICAST;
      ioctl(tun_fd, TUNSETIFF, &req);
      ioctl(sock, SIOCSIFFLAGS, "gre0");
      write(tun_fd, "hi", 0);
      return 0;
    }

==========================

Signed-off-by Alexander Potapenko <[email protected]>

---
v2: free the skb

---
 drivers/net/tun.c | 5 +++++
 1 file changed, 5 insertions(+)

drivers/net/tun.c +5 -0

index 3c9985f29950..0d60fd4ada9e 100644
@@ -1496,6 +1496,11 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 	switch (tun->flags & TUN_TYPE_MASK) {
 	case IFF_TUN:
 		if (tun->flags & IFF_NO_PI) {
+			if (!skb->len) {
+				this_cpu_inc(tun->pcpu_stats->rx_dropped);
+				kfree_skb(skb);
+				return -EINVAL;
+			}
 			switch (skb->data[0] & 0xf0) {
 			case 0x40:
 				pi.proto = htons(ETH_P_IP);

Signature · 1 line

2.14.1.821.g8fa685d3b7-goog

Eric Dumazet

Acked-by on v2 1/1

2017-09-27 12:42 UTC #1 lore raw

Quoted 17 lines · Alexander Potapenko

KMSAN (https://github.com/google/kmsan) reported accessing uninitialized skb->data[0] in the case the skb is empty (i.e. skb->len is 0):
Signed-off-by Alexander Potapenko <[email protected]>
---
v2: free the skb
---
 drivers/net/tun.c | 5 +++++
 1 file changed, 5 insertions(+)
drivers/net/tun.c +0 -0
index 3c9985f29950..0d60fd4ada9e 100644
@@ -1496,6 +1496,11 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 	switch (tun->flags & TUN_TYPE_MASK) {

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

Quoted 10 lines

drivers/net/tun.c +5 -0

 	case IFF_TUN:
 		if (tun->flags & IFF_NO_PI) {
+			if (!skb->len) {
+				this_cpu_inc(tun->pcpu_stats->rx_dropped);
+				kfree_skb(skb);
+				return -EINVAL;
+			}
 			switch (skb->data[0] & 0xf0) {
 			case 0x40:
 				pi.proto = htons(ETH_P_IP);

Acked-by Eric Dumazet <[email protected]>

Or something cleaner to avoid copy/paste and focus on proper skb->data[0] access and meaning.

Thanks.

drivers/net/tun.c +5 -3

index 3c9985f299503ea65dad7eb3b47e2ab3bef87800..8ddb840687c1bdb24e4182612abc9e362624c3e9 100644
@@ -1496,11 +1496,13 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 	switch (tun->flags & TUN_TYPE_MASK) {
 	case IFF_TUN:
 		if (tun->flags & IFF_NO_PI) {
-			switch (skb->data[0] & 0xf0) {
-			case 0x40:
+			u8 ip_proto = skb->len ? (skb->data[0] >> 4) : 0;
+
+			switch (ip_proto) {
+			case 4:
 				pi.proto = htons(ETH_P_IP);
 				break;
-			case 0x60:
+			case 6:
 				pi.proto = htons(ETH_P_IPV6);
 				break;
 			default:

Eric Dumazet

2017-09-27 12:45 UTC #2 lore raw

Quoted 10 lines · Eric Dumazet

Or something cleaner to avoid copy/paste and focus on proper skb->data[0] access and meaning.
Thanks.
drivers/net/tun.c +0 -0
index 3c9985f299503ea65dad7eb3b47e2ab3bef87800..8ddb840687c1bdb24e4182612abc9e362624c3e9 100644

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

Quoted 6 lines

drivers/net/tun.c +1 -2

@@ -1496,11 +1496,13 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 	switch (tun->flags & TUN_TYPE_MASK) {
 	case IFF_TUN:
 		if (tun->flags & IFF_NO_PI) {
-			switch (skb->data[0] & 0xf0) {
-			case 0x40:
+			u8 ip_proto = skb->len ? (skb->data[0] >> 4) : 0;

And name this variable ip_version ;)

Alexander Potapenko

author

2017-09-27 12:58 UTC #3 lore raw

Quoted 4 lines · Eric Dumazet

Quoted 2 lines · Eric Dumazet
Or something cleaner to avoid copy/paste and focus on proper skb->data[0] access and meaning.

By the way I'm wondering if this is the only place where skb->data is being accessed.
Isn't eth_type_trans() under IFF_TAP also touching it? Then we need to check the size earlier.

Quoted 18 lines

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

Quoted 13 lines

Thanks.

drivers/net/tun.c +1 -2

index 3c9985f299503ea65dad7eb3b47e2ab3bef87800..8ddb840687c1bdb24e4182612abc9e362624c3e9 100644
@@ -1496,11 +1496,13 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
      switch (tun->flags & TUN_TYPE_MASK) {
      case IFF_TUN:
              if (tun->flags & IFF_NO_PI) {
-                     switch (skb->data[0] & 0xf0) {
-                     case 0x40:
+                     u8 ip_proto = skb->len ? (skb->data[0] >> 4) : 0;

And name this variable ip_version ;)

Signature · 10 lines

Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Eric Dumazet

2017-09-27 13:26 UTC #4 lore raw

Quoted 9 lines · Alexander Potapenko

Quoted 4 lines · Eric Dumazet
Quoted 2 lines · Eric Dumazet
Or something cleaner to avoid copy/paste and focus on proper skb->data[0] access and meaning.
By the way I'm wondering if this is the only place where skb->data is being accessed.
Isn't eth_type_trans() under IFF_TAP also touching it? Then we need to check the size earlier.

It is already checked.

Alexander Potapenko

author

2017-09-27 14:31 UTC #5 lore raw

Quoted 4 lines · Eric Dumazet' via syzkaller

Quoted 3 lines · Alexander Potapenko
Quoted 2 lines · Eric Dumazet
On Wed, 2017-09-27 at 05:42 -0700, Eric Dumazet wrote:

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
Or something cleaner to avoid copy/paste and focus on proper skb->data[0] access and meaning.
By the way I'm wondering if this is the only place where skb->data is being accessed.
Isn't eth_type_trans() under IFF_TAP also touching it? Then we need to check the size earlier.
It is already checked.

Indeed, thanks.

Quoted 4 lines

--
You received this message because you are subscribed to the Google Groups "syzkaller" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.

Signature · 10 lines

Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg