From patchwork Thu Jun 7 20:39:49 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Cong Wang X-Patchwork-Id: 926519 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=none dis=none) header.from=gmail.com Authentication-Results: ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="CMb0R/5N"; dkim-atps=neutral Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 411y8w6hKfz9ry1 for ; Fri, 8 Jun 2018 06:40:16 +1000 (AEST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932589AbeFGUkB (ORCPT ); Thu, 7 Jun 2018 16:40:01 -0400 Received: from mail-pl0-f68.google.com ([209.85.160.68]:44734 "EHLO mail-pl0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932254AbeFGUj7 (ORCPT ); Thu, 7 Jun 2018 16:39:59 -0400 Received: by mail-pl0-f68.google.com with SMTP id z9-v6so6857232plk.11 for ; Thu, 07 Jun 2018 13:39:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id; bh=Szzg3CkOc3WiICR1Pv/gNcgP+K2Y7G+GLkGZU7LLKrw=; b=CMb0R/5NhO10ILa+GfhJjdC8Lt9sXsNWCkFDjuN00H3innHOSbsLoI8xhWn/oa4gSK +8fjf1ZUNsEANQjayTDI5K48tEPxBiPTqqd6EMXo4oGLXBhx7LMR75VnRJERqQtoUb0x tnhKSuGbGlAk+nZI6njidAanWxa3AZ3uXGewEresErEoRR6KuWDC2vfA97rvuEoirZVY 4AK1LNMMDrc1A0iW42ioVCONBVilWnD/AF4r9fW8We7Lf34kmwcnJkws4VqAd8d0Ab5a qXrDsgm131LGJxFqTz3G14qnPHelgZ61vHtjunmTLqQ5Aqp5pi/14mu2eoiZ+AnTK4d7 FyAw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=Szzg3CkOc3WiICR1Pv/gNcgP+K2Y7G+GLkGZU7LLKrw=; b=Xz050Pwx5d/RSGlWKcduBcpEw+qHC0A+AfAGsHGn8aQulLCECgcAKCFPSfvj/HENaT hlRSDiKIjUpGYR5k6+FR1OEinPBM4nDNU57zng5DB/Vyxvxn3mydr2Yo4A6mQZag8vK5 j2Vp24sQfiK0SVis8uqI7p31ElEc7QurpKPBIhaUo/aK1p/XzdQyA0JR40TA1aqM/N+g /hmmpPvdvJp6YjSxAScXIgnmpzcH8Mza51mQKkAjGPIs4biKPQ+8H0Idhp0ddxbNdAib nx1S1SEeN5cnwwOB9AmoOIQk1xbYSC4Gk3OfQFd/mAFJYR+JtljQz5BnosdrvTY/WiTz FeYw== X-Gm-Message-State: APt69E2NjiuxoevVouEAaHp97JdioMfQjJF2pOlARHeA/RbWOa9yryRe 6CQw7uGQl4XKeduVKGkZVIPNQmI/ X-Google-Smtp-Source: ADUXVKLGPc3PvlmQk/ugenuMCYNVY/lPyh1JjNKo4gcnHofzP6/jnBteRsliL5IfHvEU/0iRw+M/+g== X-Received: by 2002:a17:902:1004:: with SMTP id b4-v6mr3451659pla.82.1528403999216; Thu, 07 Jun 2018 13:39:59 -0700 (PDT) Received: from tw-172-25-29-37.office.twttr.net ([8.25.197.25]) by smtp.gmail.com with ESMTPSA id e16-v6sm17743535pff.185.2018.06.07.13.39.58 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 07 Jun 2018 13:39:58 -0700 (PDT) From: Cong Wang To: netdev@vger.kernel.org Cc: shankarapailoor@gmail.com, Cong Wang , Tetsuo Handa , Lorenzo Colitti , Al Viro Subject: [Patch net] socket: close race condition between sock_close() and sockfs_setattr() Date: Thu, 7 Jun 2018 13:39:49 -0700 Message-Id: <20180607203949.16945-1-xiyou.wangcong@gmail.com> X-Mailer: git-send-email 2.9.4 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org fchownat() doesn't even hold refcnt of fd until it figures out fd is really needed (otherwise is ignored) and releases it after it resolves the path. This means sock_close() could race with sockfs_setattr(), which leads to a NULL pointer dereference since typically we set sock->sk to NULL in ->release(). As pointed out by Al, this is unique to sockfs. So we can fix this in socket layer by acquiring inode_lock in sock_close() and checking against NULL in sockfs_setattr(). sock_release() is called in many places, only the sock_close() path matters here. And fortunately, this should not affect normal sock_close() as it is only called when the last fd refcnt is gone. It only affects sock_close() with a parallel sockfs_setattr() in progress, which is not common. Fixes: 86741ec25462 ("net: core: Add a UID field to struct sock.") Reported-by: shankarapailoor Cc: Tetsuo Handa Cc: Lorenzo Colitti Cc: Al Viro Signed-off-by: Cong Wang --- net/socket.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/net/socket.c b/net/socket.c index af57d85bcb48..8a109012608a 100644 --- a/net/socket.c +++ b/net/socket.c @@ -541,7 +541,10 @@ static int sockfs_setattr(struct dentry *dentry, struct iattr *iattr) if (!err && (iattr->ia_valid & ATTR_UID)) { struct socket *sock = SOCKET_I(d_inode(dentry)); - sock->sk->sk_uid = iattr->ia_uid; + if (sock->sk) + sock->sk->sk_uid = iattr->ia_uid; + else + err = -ENOENT; } return err; @@ -590,12 +593,16 @@ EXPORT_SYMBOL(sock_alloc); * an inode not a file. */ -void sock_release(struct socket *sock) +static void __sock_release(struct socket *sock, struct inode *inode) { if (sock->ops) { struct module *owner = sock->ops->owner; + if (inode) + inode_lock(inode); sock->ops->release(sock); + if (inode) + inode_unlock(inode); sock->ops = NULL; module_put(owner); } @@ -609,6 +616,11 @@ void sock_release(struct socket *sock) } sock->file = NULL; } + +void sock_release(struct socket *sock) +{ + __sock_release(sock, NULL); +} EXPORT_SYMBOL(sock_release); void __sock_tx_timestamp(__u16 tsflags, __u8 *tx_flags) @@ -1171,7 +1183,7 @@ static int sock_mmap(struct file *file, struct vm_area_struct *vma) static int sock_close(struct inode *inode, struct file *filp) { - sock_release(SOCKET_I(inode)); + __sock_release(SOCKET_I(inode), inode); return 0; }