From patchwork Tue Jul 21 22:59:16 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Biggers X-Patchwork-Id: 11676857 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 87E20618 for ; Tue, 21 Jul 2020 23:01:01 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 690BE2176B for ; Tue, 21 Jul 2020 23:01:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1595372461; bh=i10MleHpPiD+UlqvEBbCBk40R3U1eodRZ9LmKRTef48=; h=From:To:Cc:Subject:Date:In-Reply-To:References:List-ID:From; b=muDtRkG+rAp0d+okAkhNQMRemXggwiRf6aMalvy2LrG35ehlUfzKZVWnzRD6djUS4 xksdv6lFKa9vP/AAZurQ9j7TT5149lFk4cAQ/ooukIDroduOeZltfQibW22rLCf7Vh x3nE0ndYENPNrwzAXXaDj4zIAWJgI82ZuW7WvGpE= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731376AbgGUXBA (ORCPT ); Tue, 21 Jul 2020 19:01:00 -0400 Received: from mail.kernel.org ([198.145.29.99]:48972 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728014AbgGUXBA (ORCPT ); Tue, 21 Jul 2020 19:01:00 -0400 Received: from sol.hsd1.ca.comcast.net (c-107-3-166-239.hsd1.ca.comcast.net [107.3.166.239]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id ED9DF2073A; Tue, 21 Jul 2020 23:00:58 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1595372459; bh=i10MleHpPiD+UlqvEBbCBk40R3U1eodRZ9LmKRTef48=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=2UrKh8jVKP4YqMR+ott79ppUtxS+O7gHH9ShvxNTAuNFBZmTmqlCG5CWy9w79n+Cg XiIpP7yNESIvQqfvRZiaQZxEVS7x37eKyEqXzXOksAXo5PoR6DPlVSMqDfBnF9DnxQ 6p9F6n2gtDZpXH1niAqfrcTbH+cjpR+CZkOdM19g= From: Eric Biggers To: linux-fscrypt@vger.kernel.org Cc: linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, Daniel Rosenberg Subject: [PATCH 1/5] fscrypt: switch fscrypt_do_sha256() to use the SHA-256 library Date: Tue, 21 Jul 2020 15:59:16 -0700 Message-Id: <20200721225920.114347-2-ebiggers@kernel.org> X-Mailer: git-send-email 2.27.0 In-Reply-To: <20200721225920.114347-1-ebiggers@kernel.org> References: <20200721225920.114347-1-ebiggers@kernel.org> MIME-Version: 1.0 Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org From: Eric Biggers fscrypt_do_sha256() is only used for hashing encrypted filenames to create no-key tokens, which isn't performance-critical. Therefore a C implementation of SHA-256 is sufficient. Also, the logic to create no-key tokens is always potentially needed. This differs from fscrypt's other dependencies on crypto API algorithms, which are conditionally needed depending on what encryption policies userspace is using. Therefore, for fscrypt there isn't much benefit to allowing SHA-256 to be a loadable module. So, make fscrypt_do_sha256() use the SHA-256 library instead of the crypto_shash API. This is much simpler, since it avoids having to implement one-time-init (which is hard to do correctly, and in fact was implemented incorrectly) and handle failures to allocate the crypto_shash object. Fixes: edc440e3d27f ("fscrypt: improve format of no-key names") Cc: Daniel Rosenberg Signed-off-by: Eric Biggers --- fs/crypto/Kconfig | 2 +- fs/crypto/fname.c | 41 ++++++++++------------------------------- 2 files changed, 11 insertions(+), 32 deletions(-) diff --git a/fs/crypto/Kconfig b/fs/crypto/Kconfig index f1f11a6228eb..a5f5c30368a2 100644 --- a/fs/crypto/Kconfig +++ b/fs/crypto/Kconfig @@ -4,6 +4,7 @@ config FS_ENCRYPTION select CRYPTO select CRYPTO_HASH select CRYPTO_SKCIPHER + select CRYPTO_LIB_SHA256 select KEYS help Enable encryption of files and directories. This @@ -21,7 +22,6 @@ config FS_ENCRYPTION_ALGS select CRYPTO_CTS select CRYPTO_ECB select CRYPTO_HMAC - select CRYPTO_SHA256 select CRYPTO_SHA512 select CRYPTO_XTS diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c index d828e3df898b..011830f84d8d 100644 --- a/fs/crypto/fname.c +++ b/fs/crypto/fname.c @@ -61,30 +61,13 @@ struct fscrypt_nokey_name { */ #define FSCRYPT_NOKEY_NAME_MAX offsetofend(struct fscrypt_nokey_name, sha256) -static struct crypto_shash *sha256_hash_tfm; - -static int fscrypt_do_sha256(const u8 *data, unsigned int data_len, u8 *result) +static void fscrypt_do_sha256(const u8 *data, unsigned int data_len, u8 *result) { - struct crypto_shash *tfm = READ_ONCE(sha256_hash_tfm); - - if (unlikely(!tfm)) { - struct crypto_shash *prev_tfm; - - tfm = crypto_alloc_shash("sha256", 0, 0); - if (IS_ERR(tfm)) { - fscrypt_err(NULL, - "Error allocating SHA-256 transform: %ld", - PTR_ERR(tfm)); - return PTR_ERR(tfm); - } - prev_tfm = cmpxchg(&sha256_hash_tfm, NULL, tfm); - if (prev_tfm) { - crypto_free_shash(tfm); - tfm = prev_tfm; - } - } + struct sha256_state sctx; - return crypto_shash_tfm_digest(tfm, data, data_len, result); + sha256_init(&sctx); + sha256_update(&sctx, data, data_len); + sha256_final(&sctx, result); } static inline bool fscrypt_is_dot_dotdot(const struct qstr *str) @@ -349,7 +332,6 @@ int fscrypt_fname_disk_to_usr(const struct inode *inode, const struct qstr qname = FSTR_TO_QSTR(iname); struct fscrypt_nokey_name nokey_name; u32 size; /* size of the unencoded no-key name */ - int err; if (fscrypt_is_dot_dotdot(&qname)) { oname->name[0] = '.'; @@ -387,11 +369,9 @@ int fscrypt_fname_disk_to_usr(const struct inode *inode, } else { memcpy(nokey_name.bytes, iname->name, sizeof(nokey_name.bytes)); /* Compute strong hash of remaining part of name. */ - err = fscrypt_do_sha256(&iname->name[sizeof(nokey_name.bytes)], - iname->len - sizeof(nokey_name.bytes), - nokey_name.sha256); - if (err) - return err; + fscrypt_do_sha256(&iname->name[sizeof(nokey_name.bytes)], + iname->len - sizeof(nokey_name.bytes), + nokey_name.sha256); size = FSCRYPT_NOKEY_NAME_MAX; } oname->len = base64_encode((const u8 *)&nokey_name, size, oname->name); @@ -530,9 +510,8 @@ bool fscrypt_match_name(const struct fscrypt_name *fname, return false; if (memcmp(de_name, nokey_name->bytes, sizeof(nokey_name->bytes))) return false; - if (fscrypt_do_sha256(&de_name[sizeof(nokey_name->bytes)], - de_name_len - sizeof(nokey_name->bytes), sha256)) - return false; + fscrypt_do_sha256(&de_name[sizeof(nokey_name->bytes)], + de_name_len - sizeof(nokey_name->bytes), sha256); return !memcmp(sha256, nokey_name->sha256, sizeof(sha256)); } EXPORT_SYMBOL_GPL(fscrypt_match_name); From patchwork Tue Jul 21 22:59:17 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Biggers X-Patchwork-Id: 11676863 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 7FDD4618 for ; Tue, 21 Jul 2020 23:01:04 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 63CB120B1F for ; Tue, 21 Jul 2020 23:01:04 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1595372464; bh=+JzszfrnukA7rp34K4rJ987oveUxD8x6PQPTeTcR6H0=; h=From:To:Cc:Subject:Date:In-Reply-To:References:List-ID:From; b=ny1G0nPAvGl8NWXid9fQ0CCqjWJUJP3vPo7ZDHnUNRCo5EQkOD2cjNhDaesiWxZQG hC+vqRwj/JrUqDZbYAPgsx2RUljTQuXkwjoioY/nvsAbcLXTBefG+Pdty3f8hvRIqj R/+k7F4+nZHjA2kHOubtbBSZwhJaACdepe+QbUdE= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731353AbgGUXBA (ORCPT ); Tue, 21 Jul 2020 19:01:00 -0400 Received: from mail.kernel.org ([198.145.29.99]:48984 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1731305AbgGUXBA (ORCPT ); Tue, 21 Jul 2020 19:01:00 -0400 Received: from sol.hsd1.ca.comcast.net (c-107-3-166-239.hsd1.ca.comcast.net [107.3.166.239]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 47D762077D; Tue, 21 Jul 2020 23:00:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1595372459; bh=+JzszfrnukA7rp34K4rJ987oveUxD8x6PQPTeTcR6H0=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=UD7wGV6o8j/007/nURLRwUgh3FtROGYfwXlRmz2E+hWc/ErhCaZg7QhEk0J6rE/0G BJHPNkKnY4vns9lZKCHYAn0AyRVj/MdMod0spy99kT4HspV3dOEXWcfNm82loSvY4+ Kl/7BHiom0gFkPFuluRnlrqwmYTBIXRPyd2QkAtk= From: Eric Biggers To: linux-fscrypt@vger.kernel.org Cc: linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, Satya Tangirala Subject: [PATCH 2/5] fscrypt: use smp_load_acquire() for fscrypt_prepared_key Date: Tue, 21 Jul 2020 15:59:17 -0700 Message-Id: <20200721225920.114347-3-ebiggers@kernel.org> X-Mailer: git-send-email 2.27.0 In-Reply-To: <20200721225920.114347-1-ebiggers@kernel.org> References: <20200721225920.114347-1-ebiggers@kernel.org> MIME-Version: 1.0 Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org From: Eric Biggers Normally smp_store_release() or cmpxchg_release() is paired with smp_load_acquire(). Sometimes smp_load_acquire() can be replaced with the more lightweight READ_ONCE(). However, for this to be safe, all the published memory must only be accessed in a way that involves the pointer itself. This may not be the case if allocating the object also involves initializing a static or global variable, for example. fscrypt_prepared_key includes a pointer to a crypto_skcipher object, which is internal to and is allocated by the crypto subsystem. By using READ_ONCE() for it, we're relying on internal implementation details of the crypto subsystem. Remove this fragile assumption by using smp_load_acquire() instead. (Note: I haven't seen any real-world problems here. This change is just fixing the code to be guaranteed correct and less fragile.) Fixes: 5fee36095cda ("fscrypt: add inline encryption support") Cc: Satya Tangirala Signed-off-by: Eric Biggers --- fs/crypto/fscrypt_private.h | 15 +++++++++------ fs/crypto/inline_crypt.c | 6 ++++-- fs/crypto/keysetup.c | 6 ++++-- 3 files changed, 17 insertions(+), 10 deletions(-) diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h index bc1a3fcd45ed..8117a61b6f55 100644 --- a/fs/crypto/fscrypt_private.h +++ b/fs/crypto/fscrypt_private.h @@ -351,13 +351,16 @@ fscrypt_is_key_prepared(struct fscrypt_prepared_key *prep_key, const struct fscrypt_info *ci) { /* - * The READ_ONCE() here pairs with the smp_store_release() in - * fscrypt_prepare_key(). (This only matters for the per-mode keys, - * which are shared by multiple inodes.) + * The two smp_load_acquire()'s here pair with the smp_store_release()'s + * in fscrypt_prepare_inline_crypt_key() and fscrypt_prepare_key(). + * I.e., in some cases (namely, if this prep_key is a per-mode + * encryption key) another task can publish blk_key or tfm concurrently, + * executing a RELEASE barrier. We need to use smp_load_acquire() here + * to safely ACQUIRE the memory the other task published. */ if (fscrypt_using_inline_encryption(ci)) - return READ_ONCE(prep_key->blk_key) != NULL; - return READ_ONCE(prep_key->tfm) != NULL; + return smp_load_acquire(&prep_key->blk_key) != NULL; + return smp_load_acquire(&prep_key->tfm) != NULL; } #else /* CONFIG_FS_ENCRYPTION_INLINE_CRYPT */ @@ -391,7 +394,7 @@ static inline bool fscrypt_is_key_prepared(struct fscrypt_prepared_key *prep_key, const struct fscrypt_info *ci) { - return READ_ONCE(prep_key->tfm) != NULL; + return smp_load_acquire(&prep_key->tfm) != NULL; } #endif /* !CONFIG_FS_ENCRYPTION_INLINE_CRYPT */ diff --git a/fs/crypto/inline_crypt.c b/fs/crypto/inline_crypt.c index d7aecadf33c1..dfb06375099a 100644 --- a/fs/crypto/inline_crypt.c +++ b/fs/crypto/inline_crypt.c @@ -176,8 +176,10 @@ int fscrypt_prepare_inline_crypt_key(struct fscrypt_prepared_key *prep_key, } } /* - * Pairs with READ_ONCE() in fscrypt_is_key_prepared(). (Only matters - * for the per-mode keys, which are shared by multiple inodes.) + * Pairs with the smp_load_acquire() in fscrypt_is_key_prepared(). + * I.e., here we publish ->blk_key with a RELEASE barrier so that + * concurrent tasks can ACQUIRE it. Note that this concurrency is only + * possible for per-mode keys, not for per-file keys. */ smp_store_release(&prep_key->blk_key, blk_key); return 0; diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c index 22a94b18fe70..7f85fc645602 100644 --- a/fs/crypto/keysetup.c +++ b/fs/crypto/keysetup.c @@ -129,8 +129,10 @@ int fscrypt_prepare_key(struct fscrypt_prepared_key *prep_key, if (IS_ERR(tfm)) return PTR_ERR(tfm); /* - * Pairs with READ_ONCE() in fscrypt_is_key_prepared(). (Only matters - * for the per-mode keys, which are shared by multiple inodes.) + * Pairs with the smp_load_acquire() in fscrypt_is_key_prepared(). + * I.e., here we publish ->tfm with a RELEASE barrier so that + * concurrent tasks can ACQUIRE it. Note that this concurrency is only + * possible for per-mode keys, not for per-file keys. */ smp_store_release(&prep_key->tfm, tfm); return 0; From patchwork Tue Jul 21 22:59:18 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Biggers X-Patchwork-Id: 11676867 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 9CCB1138C for ; Tue, 21 Jul 2020 23:01:05 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 80CFC20B1F for ; Tue, 21 Jul 2020 23:01:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1595372465; bh=HkY2xoW+5h/KPN4NirPSCI6mZ1dMQTytJsLYKF1i9Kk=; h=From:To:Cc:Subject:Date:In-Reply-To:References:List-ID:From; b=i3TKwPnoJv/AppYFc8fZRLHb2SZMpOF5JLsyDhCpcLMwAfhF4pnkxi/8fC+5RbY1i VGWKNvf9IryQbogCkM4/4U99DD/Mb0vEAjCq2FA18hAjHFTyfLp8+Uw8UrPCAn76Bp mT1d2jp5afOYI/ob4lyKHVWK5TAUOj1/v0yiNU3c= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731406AbgGUXBE (ORCPT ); Tue, 21 Jul 2020 19:01:04 -0400 Received: from mail.kernel.org ([198.145.29.99]:48990 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726555AbgGUXBA (ORCPT ); Tue, 21 Jul 2020 19:01:00 -0400 Received: from sol.hsd1.ca.comcast.net (c-107-3-166-239.hsd1.ca.comcast.net [107.3.166.239]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 9616A207BB; Tue, 21 Jul 2020 23:00:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1595372459; bh=HkY2xoW+5h/KPN4NirPSCI6mZ1dMQTytJsLYKF1i9Kk=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=k3Gk9VGnsqGB5y+WMg5lND+8JiNxzRrc4VjLmUb1EIy9UlP7HqQV7CTx+ehe/RuD1 XuaTIIvbwwLc5tFsjkcZoEyxBDu3L+o2yMrmwuMjDcbypkZuUM4Or6eNsG2RGZ5mhI VdEvSYtK16hT7PpUn/1yWdPuPeao//sgvXC/ASxo= From: Eric Biggers To: linux-fscrypt@vger.kernel.org Cc: linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net Subject: [PATCH 3/5] fscrypt: use smp_load_acquire() for ->s_master_keys Date: Tue, 21 Jul 2020 15:59:18 -0700 Message-Id: <20200721225920.114347-4-ebiggers@kernel.org> X-Mailer: git-send-email 2.27.0 In-Reply-To: <20200721225920.114347-1-ebiggers@kernel.org> References: <20200721225920.114347-1-ebiggers@kernel.org> MIME-Version: 1.0 Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org From: Eric Biggers Normally smp_store_release() or cmpxchg_release() is paired with smp_load_acquire(). Sometimes smp_load_acquire() can be replaced with the more lightweight READ_ONCE(). However, for this to be safe, all the published memory must only be accessed in a way that involves the pointer itself. This may not be the case if allocating the object also involves initializing a static or global variable, for example. super_block::s_master_keys is a keyring, which is internal to and is allocated by the keyrings subsystem. By using READ_ONCE() for it, we're relying on internal implementation details of the keyrings subsystem. Remove this fragile assumption by using smp_load_acquire() instead. (Note: I haven't seen any real-world problems here. This change is just fixing the code to be guaranteed correct and less fragile.) Fixes: 22d94f493bfb ("fscrypt: add FS_IOC_ADD_ENCRYPTION_KEY ioctl") Signed-off-by: Eric Biggers --- fs/crypto/keyring.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/fs/crypto/keyring.c b/fs/crypto/keyring.c index 7f8ac61a20d6..71d56f8e2870 100644 --- a/fs/crypto/keyring.c +++ b/fs/crypto/keyring.c @@ -213,7 +213,11 @@ static int allocate_filesystem_keyring(struct super_block *sb) if (IS_ERR(keyring)) return PTR_ERR(keyring); - /* Pairs with READ_ONCE() in fscrypt_find_master_key() */ + /* + * Pairs with the smp_load_acquire() in fscrypt_find_master_key(). + * I.e., here we publish ->s_master_keys with a RELEASE barrier so that + * concurrent tasks can ACQUIRE it. + */ smp_store_release(&sb->s_master_keys, keyring); return 0; } @@ -234,8 +238,13 @@ struct key *fscrypt_find_master_key(struct super_block *sb, struct key *keyring; char description[FSCRYPT_MK_DESCRIPTION_SIZE]; - /* pairs with smp_store_release() in allocate_filesystem_keyring() */ - keyring = READ_ONCE(sb->s_master_keys); + /* + * Pairs with the smp_store_release() in allocate_filesystem_keyring(). + * I.e., another task can publish ->s_master_keys concurrently, + * executing a RELEASE barrier. We need to use smp_load_acquire() here + * to safely ACQUIRE the memory the other task published. + */ + keyring = smp_load_acquire(&sb->s_master_keys); if (keyring == NULL) return ERR_PTR(-ENOKEY); /* No keyring yet, so no keys yet. */ From patchwork Tue Jul 21 22:59:19 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Biggers X-Patchwork-Id: 11676859 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id A7AEB618 for ; Tue, 21 Jul 2020 23:01:02 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 8B39C20B1F for ; Tue, 21 Jul 2020 23:01:02 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1595372462; bh=8CFq23ApT2mi7bzagu5otAQAJtf6QR9J0mMn4N8FXeY=; h=From:To:Cc:Subject:Date:In-Reply-To:References:List-ID:From; b=hFNOrL29Sxs8eW/xvRc+ECbzlu4ew+k9/KSWwsqQhGVLfcz2nwuZ7Ws8NbEEKvkfP kIRF+jgFNeRbgXiURlwIA1sRsA5wcVbGlesmfT8iPmYtGTTNvtFdlI3HzNSEbkDKvS U940XXeE3GQ2HZmZTPswjtwUPpeo0UD498/HkR9g= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731386AbgGUXBB (ORCPT ); Tue, 21 Jul 2020 19:01:01 -0400 Received: from mail.kernel.org ([198.145.29.99]:48998 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1731328AbgGUXBA (ORCPT ); Tue, 21 Jul 2020 19:01:00 -0400 Received: from sol.hsd1.ca.comcast.net (c-107-3-166-239.hsd1.ca.comcast.net [107.3.166.239]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id E00EE207DD; Tue, 21 Jul 2020 23:00:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1595372460; bh=8CFq23ApT2mi7bzagu5otAQAJtf6QR9J0mMn4N8FXeY=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=SR6ircD6vFmclMJka0AoeoQ9p3CzMhFXM0C/JTPKEDNRmpvX+YWX2C6WaCXWupSs0 887U4yLW+4Zwi3HLY0Smlz3Zc0Kmv4SmZz4fMM8qfuia9S23AKRjvGCGfVKjsAMXWK kaW5huWBgPPHacSfH1KGUwxg9hb9Wg0y3Bc0t6pE= From: Eric Biggers To: linux-fscrypt@vger.kernel.org Cc: linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net Subject: [PATCH 4/5] fscrypt: use smp_load_acquire() for ->i_crypt_info Date: Tue, 21 Jul 2020 15:59:19 -0700 Message-Id: <20200721225920.114347-5-ebiggers@kernel.org> X-Mailer: git-send-email 2.27.0 In-Reply-To: <20200721225920.114347-1-ebiggers@kernel.org> References: <20200721225920.114347-1-ebiggers@kernel.org> MIME-Version: 1.0 Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org From: Eric Biggers Normally smp_store_release() or cmpxchg_release() is paired with smp_load_acquire(). Sometimes smp_load_acquire() can be replaced with the more lightweight READ_ONCE(). However, for this to be safe, all the published memory must only be accessed in a way that involves the pointer itself. This may not be the case if allocating the object also involves initializing a static or global variable, for example. fscrypt_info includes various sub-objects which are internal to and are allocated by other kernel subsystems such as keyrings and crypto. So by using READ_ONCE() for ->i_crypt_info, we're relying on internal implementation details of these other kernel subsystems. Remove this fragile assumption by using smp_load_acquire() instead. (Note: I haven't seen any real-world problems here. This change is just fixing the code to be guaranteed correct and less fragile.) Fixes: e37a784d8b6a ("fscrypt: use READ_ONCE() to access ->i_crypt_info") Signed-off-by: Eric Biggers --- fs/crypto/keysetup.c | 12 +++++++++++- fs/crypto/policy.c | 4 ++-- include/linux/fscrypt.h | 29 ++++++++++++++++++++++++----- 3 files changed, 37 insertions(+), 8 deletions(-) diff --git a/fs/crypto/keysetup.c b/fs/crypto/keysetup.c index 7f85fc645602..fea6226afc2b 100644 --- a/fs/crypto/keysetup.c +++ b/fs/crypto/keysetup.c @@ -518,7 +518,17 @@ int fscrypt_get_encryption_info(struct inode *inode) if (res) goto out; + /* + * Multiple tasks may race to set ->i_crypt_info, so use + * cmpxchg_release(). This pairs with the smp_load_acquire() in + * fscrypt_get_info(). I.e., here we publish ->i_crypt_info with a + * RELEASE barrier so that other tasks can ACQUIRE it. + */ if (cmpxchg_release(&inode->i_crypt_info, NULL, crypt_info) == NULL) { + /* + * We won the race and set ->i_crypt_info to our crypt_info. + * Now link it into the master key's inode list. + */ if (master_key) { struct fscrypt_master_key *mk = master_key->payload.data[0]; @@ -589,7 +599,7 @@ EXPORT_SYMBOL(fscrypt_free_inode); */ int fscrypt_drop_inode(struct inode *inode) { - const struct fscrypt_info *ci = READ_ONCE(inode->i_crypt_info); + const struct fscrypt_info *ci = fscrypt_get_info(inode); const struct fscrypt_master_key *mk; /* diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c index 8a8ad0e44bb8..2a2d0c06147b 100644 --- a/fs/crypto/policy.c +++ b/fs/crypto/policy.c @@ -338,7 +338,7 @@ static int fscrypt_get_policy(struct inode *inode, union fscrypt_policy *policy) union fscrypt_context ctx; int ret; - ci = READ_ONCE(inode->i_crypt_info); + ci = fscrypt_get_info(inode); if (ci) { /* key available, use the cached policy */ *policy = ci->ci_policy; @@ -627,7 +627,7 @@ int fscrypt_inherit_context(struct inode *parent, struct inode *child, if (res < 0) return res; - ci = READ_ONCE(parent->i_crypt_info); + ci = fscrypt_get_info(parent); if (ci == NULL) return -ENOKEY; diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h index bb257411365f..991ff8575d0e 100644 --- a/include/linux/fscrypt.h +++ b/include/linux/fscrypt.h @@ -74,10 +74,15 @@ struct fscrypt_operations { struct request_queue **devs); }; -static inline bool fscrypt_has_encryption_key(const struct inode *inode) +static inline struct fscrypt_info *fscrypt_get_info(const struct inode *inode) { - /* pairs with cmpxchg_release() in fscrypt_get_encryption_info() */ - return READ_ONCE(inode->i_crypt_info) != NULL; + /* + * Pairs with the cmpxchg_release() in fscrypt_get_encryption_info(). + * I.e., another task may publish ->i_crypt_info concurrently, executing + * a RELEASE barrier. We need to use smp_load_acquire() here to safely + * ACQUIRE the memory the other task published. + */ + return smp_load_acquire(&inode->i_crypt_info); } /** @@ -234,9 +239,9 @@ static inline void fscrypt_set_ops(struct super_block *sb, } #else /* !CONFIG_FS_ENCRYPTION */ -static inline bool fscrypt_has_encryption_key(const struct inode *inode) +static inline struct fscrypt_info *fscrypt_get_info(const struct inode *inode) { - return false; + return NULL; } static inline bool fscrypt_needs_contents_encryption(const struct inode *inode) @@ -619,6 +624,20 @@ static inline bool fscrypt_inode_uses_fs_layer_crypto(const struct inode *inode) !__fscrypt_inode_uses_inline_crypto(inode); } +/** + * fscrypt_has_encryption_key() - check whether an inode has had its key set up + * @inode: the inode to check + * + * Return: %true if the inode has had its encryption key set up, else %false. + * + * Usually this should be preceded by fscrypt_get_encryption_info() to try to + * set up the key first. + */ +static inline bool fscrypt_has_encryption_key(const struct inode *inode) +{ + return fscrypt_get_info(inode) != NULL; +} + /** * fscrypt_require_key() - require an inode's encryption key * @inode: the inode we need the key for From patchwork Tue Jul 21 22:59:20 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Biggers X-Patchwork-Id: 11676869 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 5D4EA618 for ; Tue, 21 Jul 2020 23:01:07 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 44DD7207BB for ; Tue, 21 Jul 2020 23:01:07 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1595372467; bh=bm/93v1WSvC6J93NR1nVeFT9ERB8Qnq16lzUB1BPqPE=; h=From:To:Cc:Subject:Date:In-Reply-To:References:List-ID:From; b=xS3ZiGkUmtKERKlh6p7B275VqkpQzgM2fjzYpKSmizSbbsuwc6fwaptnY42qIoOQH Vqscq9cLiAg9pGbLjuqnMqLaEqvRbgybFyJKPohLtVk2MUnHBv7uBl8OR/tIzC+hdE VvwjmwrhnXD0cb5bvA+Sd+mwO5/7K/Gle+Z1olxc= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731400AbgGUXBE (ORCPT ); Tue, 21 Jul 2020 19:01:04 -0400 Received: from mail.kernel.org ([198.145.29.99]:48984 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1731334AbgGUXBA (ORCPT ); Tue, 21 Jul 2020 19:01:00 -0400 Received: from sol.hsd1.ca.comcast.net (c-107-3-166-239.hsd1.ca.comcast.net [107.3.166.239]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 359C42080D; Tue, 21 Jul 2020 23:01:00 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1595372460; bh=bm/93v1WSvC6J93NR1nVeFT9ERB8Qnq16lzUB1BPqPE=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=yTeekrEWE34bWmgyoc8iAznT5bvWxJDueciA3ij6RH6rLKNuOnEk0MOy7iokq1aFB RuBNvzBsxiqhd7gPu+ZphlSWCK0lXcg6Pxg9KqXbYpGT8njiwXoXfX+quheoo8oXmE bhMmzYGK06j+1a+jUGVTDO7CMxfvYQHgh4nIFytI= From: Eric Biggers To: linux-fscrypt@vger.kernel.org Cc: linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net Subject: [PATCH 5/5] fs-verity: use smp_load_acquire() for ->i_verity_info Date: Tue, 21 Jul 2020 15:59:20 -0700 Message-Id: <20200721225920.114347-6-ebiggers@kernel.org> X-Mailer: git-send-email 2.27.0 In-Reply-To: <20200721225920.114347-1-ebiggers@kernel.org> References: <20200721225920.114347-1-ebiggers@kernel.org> MIME-Version: 1.0 Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org From: Eric Biggers Normally smp_store_release() or cmpxchg_release() is paired with smp_load_acquire(). Sometimes smp_load_acquire() can be replaced with the more lightweight READ_ONCE(). However, for this to be safe, all the published memory must only be accessed in a way that involves the pointer itself. This may not be the case if allocating the object also involves initializing a static or global variable, for example. fsverity_info::tree_params.hash_alg->tfm is a crypto_ahash object that's internal to and is allocated by the crypto subsystem. So by using READ_ONCE() for ->i_verity_info, we're relying on internal implementation details of the crypto subsystem. Remove this fragile assumption by using smp_load_acquire() instead. Also fix the cmpxchg logic to correctly execute an ACQUIRE barrier when losing the cmpxchg race, since cmpxchg doesn't guarantee a memory barrier on failure. (Note: I haven't seen any real-world problems here. This change is just fixing the code to be guaranteed correct and less fragile.) Fixes: fd2d1acfcadf ("fs-verity: add the hook for file ->open()") Signed-off-by: Eric Biggers --- fs/verity/open.c | 15 ++++++++++++--- include/linux/fsverity.h | 9 +++++++-- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/fs/verity/open.c b/fs/verity/open.c index d007db0c9304..bfe0280c14e4 100644 --- a/fs/verity/open.c +++ b/fs/verity/open.c @@ -221,11 +221,20 @@ struct fsverity_info *fsverity_create_info(const struct inode *inode, void fsverity_set_info(struct inode *inode, struct fsverity_info *vi) { /* - * Multiple processes may race to set ->i_verity_info, so use cmpxchg. - * This pairs with the READ_ONCE() in fsverity_get_info(). + * Multiple tasks may race to set ->i_verity_info, so use + * cmpxchg_release(). This pairs with the smp_load_acquire() in + * fsverity_get_info(). I.e., here we publish ->i_verity_info with a + * RELEASE barrier so that other tasks can ACQUIRE it. */ - if (cmpxchg(&inode->i_verity_info, NULL, vi) != NULL) + if (cmpxchg_release(&inode->i_verity_info, NULL, vi) != NULL) { + /* Lost the race, so free the fsverity_info we allocated. */ fsverity_free_info(vi); + /* + * Afterwards, the caller may access ->i_verity_info directly, + * so make sure to ACQUIRE the winning fsverity_info. + */ + (void)fsverity_get_info(inode); + } } void fsverity_free_info(struct fsverity_info *vi) diff --git a/include/linux/fsverity.h b/include/linux/fsverity.h index 78201a6d35f6..c1144a450392 100644 --- a/include/linux/fsverity.h +++ b/include/linux/fsverity.h @@ -115,8 +115,13 @@ struct fsverity_operations { static inline struct fsverity_info *fsverity_get_info(const struct inode *inode) { - /* pairs with the cmpxchg() in fsverity_set_info() */ - return READ_ONCE(inode->i_verity_info); + /* + * Pairs with the cmpxchg_release() in fsverity_set_info(). + * I.e., another task may publish ->i_verity_info concurrently, + * executing a RELEASE barrier. We need to use smp_load_acquire() here + * to safely ACQUIRE the memory the other task published. + */ + return smp_load_acquire(&inode->i_verity_info); } /* enable.c */