All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] tun: bail out from tun_get_user() if the skb is empty
@ 2017-09-27 12:16 Alexander Potapenko
  2017-09-27 12:42 ` Eric Dumazet
  0 siblings, 1 reply; 6+ messages in thread
From: Alexander Potapenko @ 2017-09-27 12:16 UTC (permalink / raw)
  To: davem, edumazet; +Cc: dvyukov, syzkaller, netdev, linux-kernel

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 <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);
-- 
2.14.1.821.g8fa685d3b7-goog

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] tun: bail out from tun_get_user() if the skb is empty
  2017-09-27 12:16 [PATCH v2] tun: bail out from tun_get_user() if the skb is empty Alexander Potapenko
@ 2017-09-27 12:42 ` Eric Dumazet
  2017-09-27 12:45   ` Eric Dumazet
  0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2017-09-27 12:42 UTC (permalink / raw)
  To: Alexander Potapenko
  Cc: davem, edumazet, dvyukov, syzkaller, netdev, linux-kernel

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:

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] tun: bail out from tun_get_user() if the skb is empty
  2017-09-27 12:42 ` Eric Dumazet
@ 2017-09-27 12:45   ` Eric Dumazet
  2017-09-27 12:58     ` Alexander Potapenko
  0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2017-09-27 12:45 UTC (permalink / raw)
  To: Alexander Potapenko
  Cc: davem, edumazet, dvyukov, syzkaller, netdev, linux-kernel

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 ;)

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] tun: bail out from tun_get_user() if the skb is empty
  2017-09-27 12:45   ` Eric Dumazet
@ 2017-09-27 12:58     ` Alexander Potapenko
  2017-09-27 13:26       ` Eric Dumazet
  0 siblings, 1 reply; 6+ messages in thread
From: Alexander Potapenko @ 2017-09-27 12:58 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: David Miller, Eric Dumazet, Dmitriy Vyukov, syzkaller, Networking, LKML

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 ;)
>
>
>



-- 
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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] tun: bail out from tun_get_user() if the skb is empty
  2017-09-27 12:58     ` Alexander Potapenko
@ 2017-09-27 13:26       ` Eric Dumazet
  2017-09-27 14:31         ` Alexander Potapenko
  0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2017-09-27 13:26 UTC (permalink / raw)
  To: Alexander Potapenko
  Cc: Eric Dumazet, David Miller, Dmitriy Vyukov, syzkaller, Networking, LKML

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] tun: bail out from tun_get_user() if the skb is empty
  2017-09-27 13:26       ` Eric Dumazet
@ 2017-09-27 14:31         ` Alexander Potapenko
  0 siblings, 0 replies; 6+ messages in thread
From: Alexander Potapenko @ 2017-09-27 14:31 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Eric Dumazet, David Miller, Dmitriy Vyukov, syzkaller, Networking, LKML

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.



-- 
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

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-09-27 14:32 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-27 12:16 [PATCH v2] tun: bail out from tun_get_user() if the skb is empty Alexander Potapenko
2017-09-27 12:42 ` Eric Dumazet
2017-09-27 12:45   ` Eric Dumazet
2017-09-27 12:58     ` Alexander Potapenko
2017-09-27 13:26       ` Eric Dumazet
2017-09-27 14:31         ` Alexander Potapenko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.