From patchwork Mon Jun 29 18:31:09 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mauricio Faria de Oliveira X-Patchwork-Id: 1318972 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=none (no SPF record) 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-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 49wbfl10q9z9sRN; Tue, 30 Jun 2020 04:31:26 +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 1jpyYq-00073X-Sb; Mon, 29 Jun 2020 18:31:20 +0000 Received: from youngberry.canonical.com ([91.189.89.112]) by huckleberry.canonical.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1jpyYn-00072h-IK for kernel-team@lists.ubuntu.com; Mon, 29 Jun 2020 18:31:17 +0000 Received: from mail-qk1-f200.google.com ([209.85.222.200]) by youngberry.canonical.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1jpyYn-0008MK-7e for kernel-team@lists.ubuntu.com; Mon, 29 Jun 2020 18:31:17 +0000 Received: by mail-qk1-f200.google.com with SMTP id u186so12876022qka.4 for ; Mon, 29 Jun 2020 11:31:17 -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:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=AfTO9U3Ck3Dw6xkmDbwDa4Jlq7RzEKcKGOohflaCAh4=; b=SW7sK26SRJFfaGKXHpaoEjGbmEhMCRZwEWGMG06G00858qgFnYq/6EdwLYWN3mthZy 3TgKaPcOw2zYbmQydb+13kr6skQyd55+Yf+VY/cEHZ1BjqUaD3L/WLHe8fK3KhOL+2/H LY9/E4xIH3HH7WwWcE3rD46xpK6Q9gM9Y29m0RKcV5ugJOUF7rWdl/+t9sw3treVKFbx UekDEakpluhA9CY1F65VT6mF0S5Vyq6tfR7cT9yyCr/hn79uvYolacDxiXhbn/fSYPcq MjSXJot6J78e/NlDHx9Wy3PCQfQRCwk4BYLuxIR4NtnwS8d2JPXE/3RMAg+cw72aUMDS 59SQ== X-Gm-Message-State: AOAM53395c6Fl7Iumg8HIABoqDpNI7CBBxKs2nEBTsEbTiPQf+TuG2lK ge8h1H/HH+GPW+oSDLVgU3tU+i/0qmNUrvkuQMBwoQucrteeEcT63V2m8w4Rp16CukK2un1bkD8 x/mGKTcHcVqUoCoXvaNMvW+BaWKPIY3+HJ4CzDH1Lhg== X-Received: by 2002:ac8:2af0:: with SMTP id c45mr17007260qta.296.1593455476149; Mon, 29 Jun 2020 11:31:16 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzjpNI0sf/kxFGwlQ+f4r42UvtEI0Oxk21YzF6ZcsYLgTrTQ8RBgp6bkDu+mCE1jwutJb4Wcg== X-Received: by 2002:ac8:2af0:: with SMTP id c45mr17007238qta.296.1593455475864; Mon, 29 Jun 2020 11:31:15 -0700 (PDT) Received: from localhost.localdomain ([201.82.49.101]) by smtp.gmail.com with ESMTPSA id a25sm577477qtk.40.2020.06.29.11.31.14 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 29 Jun 2020 11:31:15 -0700 (PDT) From: Mauricio Faria de Oliveira To: kernel-team@lists.ubuntu.com Subject: [X/B/D/E][PATCH 1/2] aufs: do not call i_readcount_inc() Date: Mon, 29 Jun 2020 15:31:09 -0300 Message-Id: <20200629183110.1533365-2-mfo@canonical.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20200629183110.1533365-1-mfo@canonical.com> References: <20200629183110.1533365-1-mfo@canonical.com> MIME-Version: 1.0 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: , Errors-To: kernel-team-bounces@lists.ubuntu.com Sender: "kernel-team" The 'struct inode.i_readcount' field is maintained at the VFS, and should not be modified by filesystems. But aufs does in one place, which causes it to be unbalanced. This started with Linux v2.6.39 commit 890275b5eb79 ("IMA: maintain i_readcount in the VFS layer"), which moved the i_readcount updates from IMA into the VFS (at the same places IMA was called previously) and introduced 'mutex_lock(i_mutex)' in the ima_file_check() path. The former change is functionally equivalent, thus no changes are needed in response to it. The latter change, on the other hand, is _not_; and is reported to cause a deadlock in aufs (see below), thus it dropped the call to ima_file_check(). However, when dropping the ima_file_check() call, aufs introduced the i_readcount_inc() call as well, which according to the commit changes is not necessary. This can be observed in aufs2-standalone.git commit 1dbd1c864e455 ("aufs2.1 standalone version for linux-2.6."), announced to the aufs-users mailing list on 2011-04-04 [1]. diff --git a/ChangeLog b/ChangeLog ... +commit 17eac367b03334e57a93e8051eb712add24d2534 +Author: J. R. Okajima +Date: Fri Apr 1 16:31:22 2011 +0900 + + aufs: for 2.6.39, limit the support for IMA + + Since it acquires i_mutex and causes a deadlock, replace a + ima_file_check() call by i_readcount_inc(). + + Signed-off-by: J. R. Okajima ... diff --git a/fs/aufs/vfsub.c b/fs/aufs/vfsub.c ... struct file *vfsub_dentry_open(struct path *path, int flags) ... + if (!IS_ERR_OR_NULL(file) + && (file->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ) + i_readcount_inc(path->dentry->d_inode); - err = ima_file_check(file, au_conv_oflags(flags)); ... Apparently, this might have been a misunderstanding of one hunk in the 2.6.39 commit, that deletes the lines to increment i_readcount, and adds the lines to acquire i_mutex. It reuses code from the removed function ima_counts_get() to create ima_rdwr_violation_check(), and another hunk calls the new function from ima_file_check(). But note that the i_readcount increment was _not_ called from ima_file_check() previously, via ima_counts_get(): -void ima_counts_get(struct file *file) +static void ima_rdwr_violation_check(struct file *file) { ... + mutex_lock(&inode->i_mutex); /* file metadata: permissions, xattr */ ... - atomic_inc(&inode->i_readcount); #@@ -318,6 +308,7 @@ int ima_file_check(struct file *file, int mask) ... + ima_rdwr_violation_check(file); So, in order to avoid the unbalance caused to i_readcount, drop the i_readcount_inc() call. Note the issue is not the lack of a corresponding i_readcount_dec() call; it's the mere usage of these functions outside of VFS layer, where i_readcount is maintained. Links: [1] https://sourceforge.net/p/aufs/mailman/message/27304125/ snippet: """ aufs2 Monday GIT release From: - 2011-04-04 04:59:18 o news - begin supporting linux-2.6.39-rcN. ... - aufs2-2.6.git#aufs2.1 branch ... aufs: for 2.6.39, limit the support for IMA ... """ Signed-off-by: Mauricio Faria de Oliveira (cherry picked from commit 515a586eeef31e0717d5dea21e2c11a965340b3c aufs4-linux.git) CVE-2020-11935 Signed-off-by: Mauricio Faria de Oliveira --- fs/aufs/vfsub.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/fs/aufs/vfsub.c b/fs/aufs/vfsub.c index d2af7ce8dc84..672ea2d8c93f 100644 --- a/fs/aufs/vfsub.c +++ b/fs/aufs/vfsub.c @@ -48,15 +48,8 @@ int vfsub_update_h_iattr(struct path *h_path, int *did) struct file *vfsub_dentry_open(struct path *path, int flags) { - struct file *file; - - file = dentry_open(path, flags /* | __FMODE_NONOTIFY */, + return dentry_open(path, flags /* | __FMODE_NONOTIFY */, current_cred()); - if (!IS_ERR_OR_NULL(file) - && (file->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ) - i_readcount_inc(d_inode(path->dentry)); - - return file; } struct file *vfsub_filp_open(const char *path, int oflags, int mode)