[PATCH net v2 1/2] tun: publish tfile after it's fully initialized

[PATCH net v2 1/2] tun: publish tfile after it's fully initialized

[Stanislav Fomichev]

BUG: unable to handle kernel NULL pointer dereference at 00000000000000d1
Call Trace:
 ? napi_gro_frags+0xa7/0x2c0
 tun_get_user+0xb50/0xf20
 tun_chr_write_iter+0x53/0x70
 new_sync_write+0xff/0x160
 vfs_write+0x191/0x1e0
 __x64_sys_write+0x5e/0xd0
 do_syscall_64+0x47/0xf0
 entry_SYSCALL_64_after_hwframe+0x44/0xa9

I think there is a subtle race between sending a packet via tap and
attaching it:

CPU0:                    CPU1:
tun_chr_ioctl(TUNSETIFF)
  tun_set_iff
    tun_attach
      rcu_assign_pointer(tfile->tun, tun);
                         tun_fops->write_iter()
                           tun_chr_write_iter
                             tun_napi_alloc_frags
                               napi_get_frags
                                 napi->skb = napi_alloc_skb
      tun_napi_init
        netif_napi_add
          napi->skb = NULL
                              napi->skb is NULL here
                              napi_gro_frags
                                napi_frags_skb
				  skb = napi->skb
				  skb_reset_mac_header(skb)
				  panic()

Move rcu_assign_pointer(tfile->tun) and rcu_assign_pointer(tun->tfiles) to
be the last thing we do in tun_attach(); this should guarantee that when we
call tun_get() we always get an initialized object.

v2 changes:
* remove extra napi_mutex locks/unlocks for napi operations

Reported-by: syzbot <syzkaller@googlegroups.com>
Fixes: 90e33d459407 ("tun: enable napi_gro_frags() for TUN/TAP driver")

Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 drivers/net/tun.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index a4fdad475594..18656c4094b3 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -856,10 +856,6 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
 		err = 0;
 	}
 
-	rcu_assign_pointer(tfile->tun, tun);
-	rcu_assign_pointer(tun->tfiles[tun->numqueues], tfile);
-	tun->numqueues++;
-
 	if (tfile->detached) {
 		tun_enable_queue(tfile);
 	} else {
@@ -876,6 +872,13 @@ static int tun_attach(struct tun_struct *tun, struct file *file,
 	 * refcnt.
 	 */
 
+	/* Publish tfile->tun and tun->tfiles only after we've fully
+	 * initialized tfile; otherwise we risk using half-initialized
+	 * object.
+	 */
+	rcu_assign_pointer(tfile->tun, tun);
+	rcu_assign_pointer(tun->tfiles[tun->numqueues], tfile);
+	tun->numqueues++;
 out:
 	return err;
 }
-- 
2.20.1.97.g81188d93c3-goog

[PATCH net v2 2/2] tun: always set skb->dev to tun->dev

[Stanislav Fomichev]

While debugging previous issue I noticed that commit 90e33d459407 ("tun:
enable napi_gro_frags() for TUN/TAP driver") started conditionally
(!frags) calling eth_type_trans(skb, tun->dev) for IFF_TAP case. Since
eth_type_trans sets skb->dev, some skbs can now have NULL skb->dev.
Fix that by always setting skb->dev unconditionally.

The syzbot fails with the following trace:
WARNING: CPU: 0 PID: 11136 at net/core/flow_dissector.c:764
 skb_flow_dissect_flow_keys_basic include/linux/skbuff.h:1240 [inline]
 skb_probe_transport_header include/linux/skbuff.h:2403 [inline]
 tun_get_user+0x2d4a/0x4250 drivers/net/tun.c:1906
 tun_chr_write_iter+0xb9/0x160 drivers/net/tun.c:1993
 call_write_iter include/linux/fs.h:1808 [inline]
 new_sync_write fs/read_write.c:474 [inline]

But I don't think there is an actual issue since we exercise flow
dissector via eth_get_headlen which doesn't use skb (and hence BPF flow
dissector). But let's still properly set skb->dev so we don't have
any problems going forward.

Reported-by: syzbot <syzkaller@googlegroups.com>
Fixes: 90e33d459407 ("tun: enable napi_gro_frags() for TUN/TAP driver")

Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 drivers/net/tun.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 18656c4094b3..2dea2fb88b62 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1890,6 +1890,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 		return -EINVAL;
 	}
 
+	skb->dev = tun->dev;
 	switch (tun->flags & TUN_TYPE_MASK) {
 	case IFF_TUN:
 		if (tun->flags & IFF_NO_PI) {
@@ -1911,7 +1912,6 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 
 		skb_reset_mac_header(skb);
 		skb->protocol = pi.proto;
-		skb->dev = tun->dev;
 		break;
 	case IFF_TAP:
 		if (!frags)
-- 
2.20.1.97.g81188d93c3-goog

Re: [PATCH net v2 2/2] tun: always set skb->dev to tun->dev

[Willem de Bruijn]

On Mon, Jan 7, 2019 at 4:41 PM Stanislav Fomichev <sdf@google.com> wrote:
>
> While debugging previous issue I noticed that commit 90e33d459407 ("tun:
> enable napi_gro_frags() for TUN/TAP driver") started conditionally
> (!frags) calling eth_type_trans(skb, tun->dev) for IFF_TAP case. Since
> eth_type_trans sets skb->dev, some skbs can now have NULL skb->dev.
> Fix that by always setting skb->dev unconditionally.
>
> The syzbot fails with the following trace:
> WARNING: CPU: 0 PID: 11136 at net/core/flow_dissector.c:764
>  skb_flow_dissect_flow_keys_basic include/linux/skbuff.h:1240 [inline]
>  skb_probe_transport_header include/linux/skbuff.h:2403 [inline]
>  tun_get_user+0x2d4a/0x4250 drivers/net/tun.c:1906
>  tun_chr_write_iter+0xb9/0x160 drivers/net/tun.c:1993
>  call_write_iter include/linux/fs.h:1808 [inline]
>  new_sync_write fs/read_write.c:474 [inline]
>
> But I don't think there is an actual issue since we exercise flow
> dissector via eth_get_headlen which doesn't use skb (and hence BPF flow
> dissector).

Do you mean skb_probe_transport_header?

if frags, tun_napi_alloc_frags will return napi->skb, which has
skb->dev set by napi_reuse_skb.

I don't think this is needed.

Re: [PATCH net v2 2/2] tun: always set skb->dev to tun->dev

[Stanislav Fomichev]

On 01/07, Willem de Bruijn wrote:
> On Mon, Jan 7, 2019 at 4:41 PM Stanislav Fomichev <sdf@google.com> wrote:
> >
> > While debugging previous issue I noticed that commit 90e33d459407 ("tun:
> > enable napi_gro_frags() for TUN/TAP driver") started conditionally
> > (!frags) calling eth_type_trans(skb, tun->dev) for IFF_TAP case. Since
> > eth_type_trans sets skb->dev, some skbs can now have NULL skb->dev.
> > Fix that by always setting skb->dev unconditionally.
> >
> > The syzbot fails with the following trace:
> > WARNING: CPU: 0 PID: 11136 at net/core/flow_dissector.c:764
> >  skb_flow_dissect_flow_keys_basic include/linux/skbuff.h:1240 [inline]
> >  skb_probe_transport_header include/linux/skbuff.h:2403 [inline]
> >  tun_get_user+0x2d4a/0x4250 drivers/net/tun.c:1906
> >  tun_chr_write_iter+0xb9/0x160 drivers/net/tun.c:1993
> >  call_write_iter include/linux/fs.h:1808 [inline]
> >  new_sync_write fs/read_write.c:474 [inline]
> >
> > But I don't think there is an actual issue since we exercise flow
> > dissector via eth_get_headlen which doesn't use skb (and hence BPF flow
> > dissector).
> 
> Do you mean skb_probe_transport_header?
I was actually thinking about possible future conversion of
eth_get_headlen to be able to run bpf flow dissector, I missed the fact
that we already call dissector via skb_probe_transport_header :-(
> 
> if frags, tun_napi_alloc_frags will return napi->skb, which has
> skb->dev set by napi_reuse_skb.
> 
> I don't think this is needed.
Ah, indeed, napi_alloc_skb sets proper skb->dev.

Re: [PATCH net v2 1/2] tun: publish tfile after it's fully initialized

[David Miller]

From: Stanislav Fomichev <sdf@google.com>
Date: Mon,  7 Jan 2019 13:38:38 -0800

> BUG: unable to handle kernel NULL pointer dereference at 00000000000000d1
> Call Trace:
 ...
> 
> I think there is a subtle race between sending a packet via tap and
> attaching it:
> 
> CPU0:                    CPU1:
> tun_chr_ioctl(TUNSETIFF)
 ...
> Move rcu_assign_pointer(tfile->tun) and rcu_assign_pointer(tun->tfiles) to
> be the last thing we do in tun_attach(); this should guarantee that when we
> call tun_get() we always get an initialized object.
> 
> v2 changes:
> * remove extra napi_mutex locks/unlocks for napi operations
> 
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Fixes: 90e33d459407 ("tun: enable napi_gro_frags() for TUN/TAP driver")
> Signed-off-by: Stanislav Fomichev <sdf@google.com>

Applied and queued up for -stable.

Please, the next time you submit a patch series, provide a proper header
posting.

Thank you.