From patchwork Thu Jul 5 03:52:26 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Khalid Elmously X-Patchwork-Id: 939634 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=none (mailfrom) smtp.mailfrom=lists.ubuntu.com (client-ip=91.189.94.19; helo=huckleberry.canonical.com; envelope-from=kernel-team-bounces@lists.ubuntu.com; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=canonical.com Received: from huckleberry.canonical.com (huckleberry.canonical.com [91.189.94.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 41LkTg63lbz9s2g; Thu, 5 Jul 2018 13:52:55 +1000 (AEST) Received: from localhost ([127.0.0.1] helo=huckleberry.canonical.com) by huckleberry.canonical.com with esmtp (Exim 4.86_2) (envelope-from ) id 1favK4-0001uW-2A; Thu, 05 Jul 2018 03:52:48 +0000 Received: from youngberry.canonical.com ([91.189.89.112]) by huckleberry.canonical.com with esmtps (TLS1.0:DHE_RSA_AES_128_CBC_SHA1:128) (Exim 4.86_2) (envelope-from ) id 1favK2-0001uC-HL for kernel-team@lists.ubuntu.com; Thu, 05 Jul 2018 03:52:46 +0000 Received: from mail-it0-f71.google.com ([209.85.214.71]) by youngberry.canonical.com with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.76) (envelope-from ) id 1favK2-0003Rh-6h for kernel-team@lists.ubuntu.com; Thu, 05 Jul 2018 03:52:46 +0000 Received: by mail-it0-f71.google.com with SMTP id k13-v6so4490852ite.5 for ; Wed, 04 Jul 2018 20:52:46 -0700 (PDT) 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:in-reply-to :references; bh=906d0E/Kimmo5TJC/XnQ1+0WLaAtJfB667dhjj8iUyI=; b=F8jcr933YFfyT7BtYTKchuG/ig3OOsAU9U5m3BM5Qer24j24DJMGrBxj3dW6+/zbrK dlp7OIaH0Rf/8RXC3XHzHmTb8nKz2FD6vPdgXLU9rRSKSywJZVBzQ0wjBMKT9pxW2cFc cpFqvFLmqYPtHZGl9WDOXkVy2G35NUkzgqLmAkLK4CCMd+qGmv3G3+6itM7G0pp4kGA8 JRfkV0XE2qdtqXXpvJXuiYLDz1q+1UOnpXg/DZA4X3AK0YTPqaJDCZNquqN2WrVAJXaa xKSgxHXr/RALGqyAXb2WnpyE01ZHM3LeO1teV81D1YV/JFyizjJYyAOgq1YWHsxqEUaV Lq8g== X-Gm-Message-State: AOUpUlH6KBWNhRYSt7ksv0+yfLKfx3zk5yQur2PQQ/mlbjlyncqI7p/n 14akDhPoIitldo86uzYUUwzGFU/i9/ir1kaWBMDjS0yMFqD2Fl8+l/dGq3XfuOQx8y8f3KpqMZr naEgqUYv3nC+t+D9sWt52vAdMO17MUhzyCGazNi0Kyw== X-Received: by 2002:a6b:9fcb:: with SMTP id i194-v6mr3532253ioe.286.1530762764925; Wed, 04 Jul 2018 20:52:44 -0700 (PDT) X-Google-Smtp-Source: AAOMgpfOxQzUcqFUZ7FqRrLm/t4dsp+WRwXybFZkcz+FYmm2BiUGmULK6aHrRVQxNGOmBQbXELFDWQ== X-Received: by 2002:a6b:9fcb:: with SMTP id i194-v6mr3532248ioe.286.1530762764752; Wed, 04 Jul 2018 20:52:44 -0700 (PDT) Received: from kbuntu.fuzzbuzz.org (198-84-180-15.cpe.teksavvy.com. [198.84.180.15]) by smtp.gmail.com with ESMTPSA id f129-v6sm2953075itc.33.2018.07.04.20.52.43 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 04 Jul 2018 20:52:43 -0700 (PDT) From: Khalid Elmously To: kernel-team@lists.ubuntu.com Subject: [SRU][[T/X][PATCH 1/1] socket: close race condition between sock_close() and sockfs_setattr() Date: Wed, 4 Jul 2018 23:52:26 -0400 Message-Id: <20180705035226.7695-3-khalid.elmously@canonical.com> X-Mailer: git-send-email 2.17.1 In-Reply-To: <20180705035226.7695-1-khalid.elmously@canonical.com> References: <20180705035226.7695-1-khalid.elmously@canonical.com> X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.20 Precedence: list List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: kernel-team-bounces@lists.ubuntu.com Sender: "kernel-team" From: Cong Wang 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 Signed-off-by: David S. Miller (backported from 6d8c50dcb029872b298eea68cc6209c866fd3e14) Signed-off-by: Khalid Elmously --- net/socket.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/net/socket.c b/net/socket.c index 1895b9eff43b..338d64756cb2 100644 --- a/net/socket.c +++ b/net/socket.c @@ -564,12 +564,16 @@ static struct socket *sock_alloc(void) * 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); } @@ -584,6 +588,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(const struct sock *sk, __u8 *tx_flags) @@ -1019,7 +1028,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; }