From patchwork Mon Jan 7 20:02:23 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stanislav Fomichev X-Patchwork-Id: 1021574 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming-netdev@ozlabs.org Delivered-To: patchwork-incoming-netdev@ozlabs.org Authentication-Results: ozlabs.org; spf=none (mailfrom) smtp.mailfrom=vger.kernel.org (client-ip=209.132.180.67; helo=vger.kernel.org; envelope-from=netdev-owner@vger.kernel.org; receiver=) Authentication-Results: ozlabs.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=google.com header.i=@google.com header.b="H96CFyc6"; dkim-atps=neutral Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 43YRBZ5phzz9sBQ for ; Tue, 8 Jan 2019 07:02:30 +1100 (AEDT) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726757AbfAGUC2 (ORCPT ); Mon, 7 Jan 2019 15:02:28 -0500 Received: from mail-qt1-f202.google.com ([209.85.160.202]:53291 "EHLO mail-qt1-f202.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726511AbfAGUC2 (ORCPT ); Mon, 7 Jan 2019 15:02:28 -0500 Received: by mail-qt1-f202.google.com with SMTP id d35so1315997qtd.20 for ; Mon, 07 Jan 2019 12:02:27 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:message-id:mime-version:subject:from:to:cc; bh=baVdTJGkepheHvmdiNMm7pJBGzsOANlfwnNNisDPOPU=; b=H96CFyc6J3NE84CM5XjuokEi7dpC7a/A/e7cs6bU/dsMIPaRC7xV7LfLhZtiSa0KsV bqpPZ0GUtSU/ABUY3KAarIlZ8lr6cKppxg5tmK7isIpbuRD966FqmwCG1JDCLSrlrH6w R3S+2gtx2U7BA9l351treSFZMZfcrYdls1ealpmh+RyM4RR6LnMC/sksG2dF/qoSN6+L Q70vfqOrn8GJ9EU2RvctqpDKbf2WqafCoab6Y+3oB85gMEX3Dwn4qjlVHnzZ2H9e/JJb smXT93x3aiCHm0T2X9ZAN812SWpOf3piYhoIQLVOotAHSx1WHWmwZb/2PEQ0k5nrfC4Z OPAg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:message-id:mime-version:subject:from:to:cc; bh=baVdTJGkepheHvmdiNMm7pJBGzsOANlfwnNNisDPOPU=; b=RC9VngSUV1szwtq+uHKxLCr6G5ZWfeMMhwJ/KAX5Jx9qw6vCPSZS10ZDwrF2hPEigb 33W3jYo/tLLKN4Q5oVvLRN0NJZRRIoEEb46fDOx99WGT6tlvYxaqjv0Nt57Ntm+ocu82 hjSPW+pr/qCUyoaKJeG/dN8grLiMFP61phTVqIF+OgXoFEDrIcmVguurjmqKBGffGt2E t73y5wz+6Uxo+fGzpySgX20Cb3BAJIfiC4Vcdjrs3zuQ7hpFNkai3yHO0dg1hmYFYHLN MeVzTFUSgbCjeNmskVFig9HyBlRuRevp3HNzy5VJw4D9hJ23sKyzHpykCIC8PMetkwBB 4sQw== X-Gm-Message-State: AJcUukclbyIvqjHdWYhRrwS8hNyVvUmXR9yVkTUOiBruUb/6odHsGg09 1s2SVzsReLG9QX7s9TWP0xYtFv7NLF3TX7CSeKeEHjvms42CFYK58Kvftme22XyCfHxXpgCz1Le 7QL4pxwj+Jwb8Tjo94yYoQeEgtb3tkN8HCYL+njS4l3uQq2h0BE6XXg== X-Google-Smtp-Source: AFSGD/WGnUg7PyFuZUOLZo0eG6VTIY/FrQbL5slC7NfeAWvc7a79jGzgaFHrNxm4KttxjwXr6YdveRc= X-Received: by 2002:ac8:2c0c:: with SMTP id d12mr46747447qta.36.1546891347108; Mon, 07 Jan 2019 12:02:27 -0800 (PST) Date: Mon, 7 Jan 2019 12:02:23 -0800 Message-Id: <20190107200224.220467-1-sdf@google.com> Mime-Version: 1.0 X-Mailer: git-send-email 2.20.1.97.g81188d93c3-goog Subject: [PATCH net 1/2] tun: hold napi_mutex for all napi operations From: Stanislav Fomichev To: netdev@vger.kernel.org Cc: davem@davemloft.net, jasowang@redhat.com, brouer@redhat.com, mst@redhat.com, edumazet@google.com, Stanislav Fomichev , syzbot Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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() To fix, do the following: * Move rcu_assign_pointer(tfile->tun, tun) 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 * As another safeguard, always grab napi_mutex whenever doing any napi operation; this should prevent napi state change between calls to napi_get_frags and napi_gro_frags Reported-by: syzbot Fixes: 90e33d459407 ("tun: enable napi_gro_frags() for TUN/TAP driver") Signed-off-by: Stanislav Fomichev --- drivers/net/tun.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/drivers/net/tun.c b/drivers/net/tun.c index a4fdad475594..7875f06011f2 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -323,22 +323,30 @@ static void tun_napi_init(struct tun_struct *tun, struct tun_file *tfile, tfile->napi_enabled = napi_en; tfile->napi_frags_enabled = napi_en && napi_frags; if (napi_en) { + mutex_lock(&tfile->napi_mutex); netif_napi_add(tun->dev, &tfile->napi, tun_napi_poll, NAPI_POLL_WEIGHT); napi_enable(&tfile->napi); + mutex_unlock(&tfile->napi_mutex); } } static void tun_napi_disable(struct tun_file *tfile) { - if (tfile->napi_enabled) + if (tfile->napi_enabled) { + mutex_lock(&tfile->napi_mutex); napi_disable(&tfile->napi); + mutex_unlock(&tfile->napi_mutex); + } } static void tun_napi_del(struct tun_file *tfile) { - if (tfile->napi_enabled) + if (tfile->napi_enabled) { + mutex_lock(&tfile->napi_mutex); netif_napi_del(&tfile->napi); + mutex_unlock(&tfile->napi_mutex); + } } static bool tun_napi_frags_enabled(const struct tun_file *tfile) @@ -856,7 +864,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++; @@ -876,6 +883,11 @@ static int tun_attach(struct tun_struct *tun, struct file *file, * refcnt. */ + /* All tun_fops depend on tun_get() returning non-null pointer. + * Thus, assigning tun to a tfile should be the last init operation, + * otherwise we risk using half-initialized object. + */ + rcu_assign_pointer(tfile->tun, tun); out: return err; }