Message ID | 20170927121649.90557-1-glider@google.com |
---|---|
State | Superseded, archived |
Delegated to: | David Miller |
Headers | show |
Series | [v2] tun: bail out from tun_get_user() if the skb is empty | expand |
On Wed, 2017-09-27 at 14:16 +0200, Alexander Potapenko wrote: > 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 <glider@google.com> > --- > v2: free the skb > --- > drivers/net/tun.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index 3c9985f29950..0d60fd4ada9e 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -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); Acked-by: Eric Dumazet <edumazet@google.com> Or something cleaner to avoid copy/paste and focus on proper skb->data[0] access and meaning. Thanks. diff --git a/drivers/net/tun.c b/drivers/net/tun.c index 3c9985f299503ea65dad7eb3b47e2ab3bef87800..8ddb840687c1bdb24e4182612abc9e362624c3e9 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -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:
On Wed, 2017-09-27 at 05:42 -0700, Eric Dumazet wrote: > Or something cleaner to avoid copy/paste and focus on proper > skb->data[0] access and meaning. > > Thanks. > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index 3c9985f299503ea65dad7eb3b47e2ab3bef87800..8ddb840687c1bdb24e4182612abc9e362624c3e9 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -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 ;)
On Wed, Sep 27, 2017 at 2:45 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Wed, 2017-09-27 at 05:42 -0700, Eric Dumazet wrote: > >> 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. >> Thanks. >> >> diff --git a/drivers/net/tun.c b/drivers/net/tun.c >> index 3c9985f299503ea65dad7eb3b47e2ab3bef87800..8ddb840687c1bdb24e4182612abc9e362624c3e9 100644 >> --- a/drivers/net/tun.c >> +++ b/drivers/net/tun.c >> @@ -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 ;) > > >
On Wed, Sep 27, 2017 at 5:58 AM, Alexander Potapenko <glider@google.com> wrote: > On Wed, Sep 27, 2017 at 2:45 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: >> On Wed, 2017-09-27 at 05:42 -0700, Eric Dumazet wrote: >> >>> 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.
On Wed, Sep 27, 2017 at 3:26 PM, 'Eric Dumazet' via syzkaller <syzkaller@googlegroups.com> wrote: > On Wed, Sep 27, 2017 at 5:58 AM, Alexander Potapenko <glider@google.com> wrote: >> On Wed, Sep 27, 2017 at 2:45 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: >>> On Wed, 2017-09-27 at 05:42 -0700, Eric Dumazet wrote: >>> >>>> 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. > -- > 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 syzkaller+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/d/optout.
================================================ 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 <glider@google.com> --- v2: free the skb --- drivers/net/tun.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/net/tun.c b/drivers/net/tun.c index 3c9985f29950..0d60fd4ada9e 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -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);