From: Eric Biggers <ebiggers3@gmail.com>
To: "André Draszik" <git@andred.net>
Cc: linux-kernel@vger.kernel.org,
Mimi Zohar <zohar@linux.vnet.ibm.com>,
David Howells <dhowells@redhat.com>,
James Morris <james.l.morris@oracle.com>,
"Serge E. Hallyn" <serge@hallyn.com>,
"Theodore Y. Ts'o" <tytso@mit.edu>,
Jaegeuk Kim <jaegeuk@kernel.org>,
Kees Cook <keescook@chromium.org>,
Eric Biggers <ebiggers@google.com>,
linux-integrity@vger.kernel.org, keyrings@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-fscrypt@vger.kernel.org
Subject: Re: [PATCH v3] fscrypt: add support for the encrypted key type
Date: Fri, 26 Jan 2018 00:37:48 +0000 [thread overview]
Message-ID: <20180126003748.f2uhgwhulcltyok6@gmail.com> (raw)
In-Reply-To: <20180118131359.8365-1-git@andred.net>
On Thu, Jan 18, 2018 at 01:13:59PM +0000, André Draszik wrote:
> -static int validate_user_key(struct fscrypt_info *crypt_info,
> +static inline struct key *fscrypt_get_encrypted_key(const char *description)
> +{
> + if (IS_ENABLED(CONFIG_ENCRYPTED_KEYS))
> + return request_key(&key_type_encrypted, description, NULL);
> + return ERR_PTR(-ENOKEY);
> +}
> +
> +static int validate_keyring_key(struct fscrypt_info *crypt_info,
> struct fscrypt_context *ctx, u8 *raw_key,
> const char *prefix, int min_keysize)
> {
> char *description;
> struct key *keyring_key;
> - struct fscrypt_key *master_key;
> - const struct user_key_payload *ukp;
> + const u8 *master_key;
> + u32 master_key_len;
> int res;
>
> description = kasprintf(GFP_NOFS, "%s%*phN", prefix,
> @@ -83,39 +93,55 @@ static int validate_user_key(struct fscrypt_info *crypt_info,
> return -ENOMEM;
>
> keyring_key = request_key(&key_type_logon, description, NULL);
> + if (keyring_key = ERR_PTR(-ENOKEY))
> + keyring_key = fscrypt_get_encrypted_key(description);
> kfree(description);
> if (IS_ERR(keyring_key))
> return PTR_ERR(keyring_key);
> down_read(&keyring_key->sem);
>
> - if (keyring_key->type != &key_type_logon) {
> + if (keyring_key->type = &key_type_logon) {
> + const struct user_key_payload *ukp;
> + const struct fscrypt_key *fk;
> +
> + ukp = user_key_payload_locked(keyring_key);
> + if (!ukp) {
> + /* key was revoked before we acquired its semaphore */
> + res = -EKEYREVOKED;
> + goto out;
> + }
> + if (ukp->datalen != sizeof(struct fscrypt_key)) {
> + res = -EINVAL;
> + goto out;
> + }
> + fk = (struct fscrypt_key *)ukp->data;
> + master_key = fk->raw;
> + master_key_len = fk->size;
> + } else if (keyring_key->type = &key_type_encrypted) {
> + const struct encrypted_key_payload *ekp;
> +
> + ekp = keyring_key->payload.data[0];
> + master_key = ekp->decrypted_data;
> + master_key_len = ekp->decrypted_datalen;
> + } else {
> printk_once(KERN_WARNING
> - "%s: key type must be logon\n", __func__);
> + "%s: key type must be logon or encrypted\n",
> + __func__);
> res = -ENOKEY;
> goto out;
> }
> - ukp = user_key_payload_locked(keyring_key);
> - if (!ukp) {
> - /* key was revoked before we acquired its semaphore */
> - res = -EKEYREVOKED;
> - goto out;
> - }
> - if (ukp->datalen != sizeof(struct fscrypt_key)) {
> - res = -EINVAL;
> - goto out;
> - }
> - master_key = (struct fscrypt_key *)ukp->data;
> BUILD_BUG_ON(FS_AES_128_ECB_KEY_SIZE != FS_KEY_DERIVATION_NONCE_SIZE);
>
> - if (master_key->size < min_keysize || master_key->size > FS_MAX_KEY_SIZE
> - || master_key->size % AES_BLOCK_SIZE != 0) {
> + if (master_key_len < min_keysize || master_key_len > FS_MAX_KEY_SIZE
> + || master_key_len % AES_BLOCK_SIZE != 0) {
> printk_once(KERN_WARNING
> - "%s: key size incorrect: %d\n",
> - __func__, master_key->size);
> + "%s: key size incorrect: %u\n",
> + __func__, master_key_len);
> res = -ENOKEY;
> goto out;
> }
> - res = derive_key_aes(ctx->nonce, master_key, raw_key);
> + res = derive_key_aes(ctx->nonce, master_key, master_key_len, raw_key);
> +
> out:
> up_read(&keyring_key->sem);
> key_put(keyring_key);
> @@ -302,12 +328,12 @@ int fscrypt_get_encryption_info(struct inode *inode)
> if (!raw_key)
> goto out;
>
> - res = validate_user_key(crypt_info, &ctx, raw_key, FS_KEY_DESC_PREFIX,
> - keysize);
> + res = validate_keyring_key(crypt_info, &ctx, raw_key,
> + FS_KEY_DESC_PREFIX, keysize);
> if (res && inode->i_sb->s_cop->key_prefix) {
> - int res2 = validate_user_key(crypt_info, &ctx, raw_key,
> - inode->i_sb->s_cop->key_prefix,
> - keysize);
> + int res2 = validate_keyring_key(crypt_info, &ctx, raw_key,
> + inode->i_sb->s_cop->key_prefix,
> + keysize);
> if (res2) {
> if (res2 = -ENOKEY)
> res = -ENOKEY;
> --
> 2.15.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe keyrings" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
The code changes here look fine, but unfortunately there is a lock ordering
problem exposed by using a key type that implements the key_type ->read()
method. The problem is that encrypted_read() can take a page fault during
copy_to_user() while holding the key semaphore, which then (with ext4) can
trigger the start of a jbd2 transaction. Meanwhile
fscrypt_get_encryption_info() can be called from within a jbd2 transaction via
fscrypt_inherit_context(), which results in a different locking order. We
probably will need to fix this by removing the call to
fscrypt_get_encryption_info() from fscrypt_inherit_context().
I've pasted the lockdep report I got below:
[ 432.705339]
[ 432.705681] ===========================
[ 432.707015] WARNING: possible circular locking dependency detected
[ 432.708155] 4.15.0-rc8-next-20180119-00019-gab7ec46b6936 #31 Not tainted
[ 432.709365] ------------------------------------------------------
[ 432.710482] keyctl/877 is trying to acquire lock:
[ 432.711286] (&mm->mmap_sem){++++}, at: [<00000000e8bba1c7>] __might_fault+0xce/0x1a0
[ 432.712688]
[ 432.712688] but task is already holding lock:
[ 432.713684] (&type->lock_class#2){++++}, at: [<000000008ccfa156>] keyctl_read_key+0x174/0x240
[ 432.715176]
[ 432.715176] which lock already depends on the new lock.
[ 432.715176]
[ 432.716601]
[ 432.716601] the existing dependency chain (in reverse order) is:
[ 432.717901]
[ 432.717901] -> #3 (&type->lock_class#2){++++}:
[ 432.718924] lock_acquire+0xaa/0x170
[ 432.719632] down_read+0x37/0xa0
[ 432.720298] validate_keyring_key.isra.1+0x97/0x410
[ 432.721218] fscrypt_get_encryption_info+0x724/0x1200
[ 432.722233] fscrypt_inherit_context+0x2c1/0x330
[ 432.723067] __ext4_new_inode+0x34f5/0x44d0
[ 432.723830] ext4_create+0x195/0x4a0
[ 432.724499] lookup_open+0xbe2/0x1400
[ 432.725185] path_openat+0xb31/0x1e50
[ 432.725876] do_filp_open+0x175/0x250
[ 432.726572] do_sys_open+0x219/0x3f0
[ 432.727312] entry_SYSCALL_64_fastpath+0x1e/0x8b
[ 432.728153]
[ 432.728153] -> #2 (jbd2_handle){.+.+}:
[ 432.729008] lock_acquire+0xaa/0x170
[ 432.729680] start_this_handle+0x47b/0x1020
[ 432.730463] jbd2__journal_start+0x32e/0x5c0
[ 432.731276] __ext4_journal_start_sb+0xa8/0x190
[ 432.732183] ext4_truncate+0x4dd/0xd00
[ 432.732868] ext4_setattr+0xa62/0x1ac0
[ 432.733528] notify_change+0x672/0xdb0
[ 432.734198] do_truncate+0x111/0x1c0
[ 432.734831] path_openat+0xee6/0x1e50
[ 432.735476] do_filp_open+0x175/0x250
[ 432.736149] do_sys_open+0x219/0x3f0
[ 432.736785] entry_SYSCALL_64_fastpath+0x1e/0x8b
[ 432.737576]
[ 432.737576] -> #1 (&ei->i_mmap_sem){++++}:
[ 432.738508] lock_acquire+0xaa/0x170
[ 432.739148] down_read+0x37/0xa0
[ 432.739735] ext4_filemap_fault+0x7b/0xb0
[ 432.740446] __do_fault+0x7a/0x200
[ 432.741060] __handle_mm_fault+0x6e0/0x2040
[ 432.741801] handle_mm_fault+0x194/0x320
[ 432.742494] __do_page_fault+0x4e1/0xa70
[ 432.743184] page_fault+0x2c/0x60
[ 432.743784] __clear_user+0x3d/0x60
[ 432.744409] clear_user+0x76/0xa0
[ 432.745009] load_elf_binary+0x2bf6/0x2f10
[ 432.745753] search_binary_handler+0x136/0x450
[ 432.746522] do_execveat_common.isra.12+0x1241/0x19c0
[ 432.747339] do_execve+0x2c/0x40
[ 432.747881] try_to_run_init_process+0x12/0x40
[ 432.748611] kernel_init+0xf2/0x180
[ 432.749190] ret_from_fork+0x3a/0x50
[ 432.749785]
[ 432.749785] -> #0 (&mm->mmap_sem){++++}:
[ 432.750576] __lock_acquire+0x8d1/0x12d0
[ 432.751232] lock_acquire+0xaa/0x170
[ 432.751848] __might_fault+0x12b/0x1a0
[ 432.752478] _copy_to_user+0x27/0xc0
[ 432.753080] encrypted_read+0x54d/0x7b0
[ 432.753734] keyctl_read_key+0x1f2/0x240
[ 432.754396] SyS_keyctl+0x184/0x2a0
[ 432.754995] entry_SYSCALL_64_fastpath+0x1e/0x8b
[ 432.755778]
[ 432.755778] other info that might help us debug this:
[ 432.755778]
[ 432.756972] Chain exists of:
[ 432.756972] &mm->mmap_sem --> jbd2_handle --> &type->lock_class#2
[ 432.756972]
[ 432.758498] Possible unsafe locking scenario:
[ 432.758498]
[ 432.759302] CPU0 CPU1
[ 432.759919] ---- ----
[ 432.760536] lock(&type->lock_class#2);
[ 432.761100] lock(jbd2_handle);
[ 432.761926] lock(&type->lock_class#2);
[ 432.762836] lock(&mm->mmap_sem);
[ 432.763348]
[ 432.763348] *** DEADLOCK ***
[ 432.763348]
[ 432.764209] 1 lock held by keyctl/877:
[ 432.764765] #0: (&type->lock_class#2){++++}, at: [<000000008ccfa156>] keyctl_read_key+0x174/0x240
[ 432.766045]
[ 432.766045] stack backtrace:
[ 432.766671] CPU: 1 PID: 877 Comm: keyctl Not tainted 4.15.0-rc8-next-20180119-00019-gab7ec46b6936 #31
[ 432.768021] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
[ 432.769211] Call Trace:
[ 432.769559] dump_stack+0x8d/0xd5
[ 432.770041] print_circular_bug.isra.20+0x321/0x5e0
[ 432.770712] ? save_trace+0xd6/0x250
[ 432.771185] check_prev_add.constprop.27+0xa2a/0x1670
[ 432.771867] ? depot_save_stack+0x2d5/0x490
[ 432.772504] ? check_usage+0x13c0/0x13c0
[ 432.773047] ? trace_hardirqs_on_caller+0x3dc/0x550
[ 432.773712] ? _raw_spin_unlock_irqrestore+0x39/0x60
[ 432.774401] ? depot_save_stack+0x2d5/0x490
[ 432.774979] ? kasan_kmalloc+0x13a/0x170
[ 432.775522] ? validate_chain.isra.24+0x13ee/0x2410
[ 432.776187] validate_chain.isra.24+0x13ee/0x2410
[ 432.776831] ? check_prev_add.constprop.27+0x1670/0x1670
[ 432.777546] __lock_acquire+0x8d1/0x12d0
[ 432.778088] lock_acquire+0xaa/0x170
[ 432.778576] ? __might_fault+0xce/0x1a0
[ 432.779096] __might_fault+0x12b/0x1a0
[ 432.779608] ? __might_fault+0xce/0x1a0
[ 432.780029] _copy_to_user+0x27/0xc0
[ 432.780438] encrypted_read+0x54d/0x7b0
[ 432.780858] ? key_task_permission+0x16e/0x3b0
[ 432.781340] ? encrypted_instantiate+0x8b0/0x8b0
[ 432.781851] ? creds_are_invalid+0x9/0x50
[ 432.782287] ? lookup_user_key+0x19a/0xf50
[ 432.782736] ? __lock_acquire+0x8d1/0x12d0
[ 432.783182] ? key_validate+0xb0/0xb0
[ 432.783602] ? keyctl_read_key+0x174/0x240
[ 432.784058] keyctl_read_key+0x1f2/0x240
[ 432.784486] SyS_keyctl+0x184/0x2a0
[ 432.784875] entry_SYSCALL_64_fastpath+0x1e/0x8b
[ 432.785374] RIP: 0033:0x7f812b6c8259
--
To unsubscribe from this list: send the line "unsubscribe keyrings" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
WARNING: multiple messages have this Message-ID (diff)
From: Eric Biggers <ebiggers3@gmail.com>
To: "André Draszik" <git@andred.net>
Cc: linux-kernel@vger.kernel.org,
Mimi Zohar <zohar@linux.vnet.ibm.com>,
David Howells <dhowells@redhat.com>,
James Morris <james.l.morris@oracle.com>,
"Serge E. Hallyn" <serge@hallyn.com>,
"Theodore Y. Ts'o" <tytso@mit.edu>,
Jaegeuk Kim <jaegeuk@kernel.org>,
Kees Cook <keescook@chromium.org>,
Eric Biggers <ebiggers@google.com>,
linux-integrity@vger.kernel.org, keyrings@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-fscrypt@vger.kernel.org
Subject: Re: [PATCH v3] fscrypt: add support for the encrypted key type
Date: Thu, 25 Jan 2018 16:37:48 -0800 [thread overview]
Message-ID: <20180126003748.f2uhgwhulcltyok6@gmail.com> (raw)
In-Reply-To: <20180118131359.8365-1-git@andred.net>
On Thu, Jan 18, 2018 at 01:13:59PM +0000, Andr� Draszik wrote:
> -static int validate_user_key(struct fscrypt_info *crypt_info,
> +static inline struct key *fscrypt_get_encrypted_key(const char *description)
> +{
> + if (IS_ENABLED(CONFIG_ENCRYPTED_KEYS))
> + return request_key(&key_type_encrypted, description, NULL);
> + return ERR_PTR(-ENOKEY);
> +}
> +
> +static int validate_keyring_key(struct fscrypt_info *crypt_info,
> struct fscrypt_context *ctx, u8 *raw_key,
> const char *prefix, int min_keysize)
> {
> char *description;
> struct key *keyring_key;
> - struct fscrypt_key *master_key;
> - const struct user_key_payload *ukp;
> + const u8 *master_key;
> + u32 master_key_len;
> int res;
>
> description = kasprintf(GFP_NOFS, "%s%*phN", prefix,
> @@ -83,39 +93,55 @@ static int validate_user_key(struct fscrypt_info *crypt_info,
> return -ENOMEM;
>
> keyring_key = request_key(&key_type_logon, description, NULL);
> + if (keyring_key == ERR_PTR(-ENOKEY))
> + keyring_key = fscrypt_get_encrypted_key(description);
> kfree(description);
> if (IS_ERR(keyring_key))
> return PTR_ERR(keyring_key);
> down_read(&keyring_key->sem);
>
> - if (keyring_key->type != &key_type_logon) {
> + if (keyring_key->type == &key_type_logon) {
> + const struct user_key_payload *ukp;
> + const struct fscrypt_key *fk;
> +
> + ukp = user_key_payload_locked(keyring_key);
> + if (!ukp) {
> + /* key was revoked before we acquired its semaphore */
> + res = -EKEYREVOKED;
> + goto out;
> + }
> + if (ukp->datalen != sizeof(struct fscrypt_key)) {
> + res = -EINVAL;
> + goto out;
> + }
> + fk = (struct fscrypt_key *)ukp->data;
> + master_key = fk->raw;
> + master_key_len = fk->size;
> + } else if (keyring_key->type == &key_type_encrypted) {
> + const struct encrypted_key_payload *ekp;
> +
> + ekp = keyring_key->payload.data[0];
> + master_key = ekp->decrypted_data;
> + master_key_len = ekp->decrypted_datalen;
> + } else {
> printk_once(KERN_WARNING
> - "%s: key type must be logon\n", __func__);
> + "%s: key type must be logon or encrypted\n",
> + __func__);
> res = -ENOKEY;
> goto out;
> }
> - ukp = user_key_payload_locked(keyring_key);
> - if (!ukp) {
> - /* key was revoked before we acquired its semaphore */
> - res = -EKEYREVOKED;
> - goto out;
> - }
> - if (ukp->datalen != sizeof(struct fscrypt_key)) {
> - res = -EINVAL;
> - goto out;
> - }
> - master_key = (struct fscrypt_key *)ukp->data;
> BUILD_BUG_ON(FS_AES_128_ECB_KEY_SIZE != FS_KEY_DERIVATION_NONCE_SIZE);
>
> - if (master_key->size < min_keysize || master_key->size > FS_MAX_KEY_SIZE
> - || master_key->size % AES_BLOCK_SIZE != 0) {
> + if (master_key_len < min_keysize || master_key_len > FS_MAX_KEY_SIZE
> + || master_key_len % AES_BLOCK_SIZE != 0) {
> printk_once(KERN_WARNING
> - "%s: key size incorrect: %d\n",
> - __func__, master_key->size);
> + "%s: key size incorrect: %u\n",
> + __func__, master_key_len);
> res = -ENOKEY;
> goto out;
> }
> - res = derive_key_aes(ctx->nonce, master_key, raw_key);
> + res = derive_key_aes(ctx->nonce, master_key, master_key_len, raw_key);
> +
> out:
> up_read(&keyring_key->sem);
> key_put(keyring_key);
> @@ -302,12 +328,12 @@ int fscrypt_get_encryption_info(struct inode *inode)
> if (!raw_key)
> goto out;
>
> - res = validate_user_key(crypt_info, &ctx, raw_key, FS_KEY_DESC_PREFIX,
> - keysize);
> + res = validate_keyring_key(crypt_info, &ctx, raw_key,
> + FS_KEY_DESC_PREFIX, keysize);
> if (res && inode->i_sb->s_cop->key_prefix) {
> - int res2 = validate_user_key(crypt_info, &ctx, raw_key,
> - inode->i_sb->s_cop->key_prefix,
> - keysize);
> + int res2 = validate_keyring_key(crypt_info, &ctx, raw_key,
> + inode->i_sb->s_cop->key_prefix,
> + keysize);
> if (res2) {
> if (res2 == -ENOKEY)
> res = -ENOKEY;
> --
> 2.15.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe keyrings" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
The code changes here look fine, but unfortunately there is a lock ordering
problem exposed by using a key type that implements the key_type ->read()
method. The problem is that encrypted_read() can take a page fault during
copy_to_user() while holding the key semaphore, which then (with ext4) can
trigger the start of a jbd2 transaction. Meanwhile
fscrypt_get_encryption_info() can be called from within a jbd2 transaction via
fscrypt_inherit_context(), which results in a different locking order. We
probably will need to fix this by removing the call to
fscrypt_get_encryption_info() from fscrypt_inherit_context().
I've pasted the lockdep report I got below:
[ 432.705339]
[ 432.705681] ======================================================
[ 432.707015] WARNING: possible circular locking dependency detected
[ 432.708155] 4.15.0-rc8-next-20180119-00019-gab7ec46b6936 #31 Not tainted
[ 432.709365] ------------------------------------------------------
[ 432.710482] keyctl/877 is trying to acquire lock:
[ 432.711286] (&mm->mmap_sem){++++}, at: [<00000000e8bba1c7>] __might_fault+0xce/0x1a0
[ 432.712688]
[ 432.712688] but task is already holding lock:
[ 432.713684] (&type->lock_class#2){++++}, at: [<000000008ccfa156>] keyctl_read_key+0x174/0x240
[ 432.715176]
[ 432.715176] which lock already depends on the new lock.
[ 432.715176]
[ 432.716601]
[ 432.716601] the existing dependency chain (in reverse order) is:
[ 432.717901]
[ 432.717901] -> #3 (&type->lock_class#2){++++}:
[ 432.718924] lock_acquire+0xaa/0x170
[ 432.719632] down_read+0x37/0xa0
[ 432.720298] validate_keyring_key.isra.1+0x97/0x410
[ 432.721218] fscrypt_get_encryption_info+0x724/0x1200
[ 432.722233] fscrypt_inherit_context+0x2c1/0x330
[ 432.723067] __ext4_new_inode+0x34f5/0x44d0
[ 432.723830] ext4_create+0x195/0x4a0
[ 432.724499] lookup_open+0xbe2/0x1400
[ 432.725185] path_openat+0xb31/0x1e50
[ 432.725876] do_filp_open+0x175/0x250
[ 432.726572] do_sys_open+0x219/0x3f0
[ 432.727312] entry_SYSCALL_64_fastpath+0x1e/0x8b
[ 432.728153]
[ 432.728153] -> #2 (jbd2_handle){.+.+}:
[ 432.729008] lock_acquire+0xaa/0x170
[ 432.729680] start_this_handle+0x47b/0x1020
[ 432.730463] jbd2__journal_start+0x32e/0x5c0
[ 432.731276] __ext4_journal_start_sb+0xa8/0x190
[ 432.732183] ext4_truncate+0x4dd/0xd00
[ 432.732868] ext4_setattr+0xa62/0x1ac0
[ 432.733528] notify_change+0x672/0xdb0
[ 432.734198] do_truncate+0x111/0x1c0
[ 432.734831] path_openat+0xee6/0x1e50
[ 432.735476] do_filp_open+0x175/0x250
[ 432.736149] do_sys_open+0x219/0x3f0
[ 432.736785] entry_SYSCALL_64_fastpath+0x1e/0x8b
[ 432.737576]
[ 432.737576] -> #1 (&ei->i_mmap_sem){++++}:
[ 432.738508] lock_acquire+0xaa/0x170
[ 432.739148] down_read+0x37/0xa0
[ 432.739735] ext4_filemap_fault+0x7b/0xb0
[ 432.740446] __do_fault+0x7a/0x200
[ 432.741060] __handle_mm_fault+0x6e0/0x2040
[ 432.741801] handle_mm_fault+0x194/0x320
[ 432.742494] __do_page_fault+0x4e1/0xa70
[ 432.743184] page_fault+0x2c/0x60
[ 432.743784] __clear_user+0x3d/0x60
[ 432.744409] clear_user+0x76/0xa0
[ 432.745009] load_elf_binary+0x2bf6/0x2f10
[ 432.745753] search_binary_handler+0x136/0x450
[ 432.746522] do_execveat_common.isra.12+0x1241/0x19c0
[ 432.747339] do_execve+0x2c/0x40
[ 432.747881] try_to_run_init_process+0x12/0x40
[ 432.748611] kernel_init+0xf2/0x180
[ 432.749190] ret_from_fork+0x3a/0x50
[ 432.749785]
[ 432.749785] -> #0 (&mm->mmap_sem){++++}:
[ 432.750576] __lock_acquire+0x8d1/0x12d0
[ 432.751232] lock_acquire+0xaa/0x170
[ 432.751848] __might_fault+0x12b/0x1a0
[ 432.752478] _copy_to_user+0x27/0xc0
[ 432.753080] encrypted_read+0x54d/0x7b0
[ 432.753734] keyctl_read_key+0x1f2/0x240
[ 432.754396] SyS_keyctl+0x184/0x2a0
[ 432.754995] entry_SYSCALL_64_fastpath+0x1e/0x8b
[ 432.755778]
[ 432.755778] other info that might help us debug this:
[ 432.755778]
[ 432.756972] Chain exists of:
[ 432.756972] &mm->mmap_sem --> jbd2_handle --> &type->lock_class#2
[ 432.756972]
[ 432.758498] Possible unsafe locking scenario:
[ 432.758498]
[ 432.759302] CPU0 CPU1
[ 432.759919] ---- ----
[ 432.760536] lock(&type->lock_class#2);
[ 432.761100] lock(jbd2_handle);
[ 432.761926] lock(&type->lock_class#2);
[ 432.762836] lock(&mm->mmap_sem);
[ 432.763348]
[ 432.763348] *** DEADLOCK ***
[ 432.763348]
[ 432.764209] 1 lock held by keyctl/877:
[ 432.764765] #0: (&type->lock_class#2){++++}, at: [<000000008ccfa156>] keyctl_read_key+0x174/0x240
[ 432.766045]
[ 432.766045] stack backtrace:
[ 432.766671] CPU: 1 PID: 877 Comm: keyctl Not tainted 4.15.0-rc8-next-20180119-00019-gab7ec46b6936 #31
[ 432.768021] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
[ 432.769211] Call Trace:
[ 432.769559] dump_stack+0x8d/0xd5
[ 432.770041] print_circular_bug.isra.20+0x321/0x5e0
[ 432.770712] ? save_trace+0xd6/0x250
[ 432.771185] check_prev_add.constprop.27+0xa2a/0x1670
[ 432.771867] ? depot_save_stack+0x2d5/0x490
[ 432.772504] ? check_usage+0x13c0/0x13c0
[ 432.773047] ? trace_hardirqs_on_caller+0x3dc/0x550
[ 432.773712] ? _raw_spin_unlock_irqrestore+0x39/0x60
[ 432.774401] ? depot_save_stack+0x2d5/0x490
[ 432.774979] ? kasan_kmalloc+0x13a/0x170
[ 432.775522] ? validate_chain.isra.24+0x13ee/0x2410
[ 432.776187] validate_chain.isra.24+0x13ee/0x2410
[ 432.776831] ? check_prev_add.constprop.27+0x1670/0x1670
[ 432.777546] __lock_acquire+0x8d1/0x12d0
[ 432.778088] lock_acquire+0xaa/0x170
[ 432.778576] ? __might_fault+0xce/0x1a0
[ 432.779096] __might_fault+0x12b/0x1a0
[ 432.779608] ? __might_fault+0xce/0x1a0
[ 432.780029] _copy_to_user+0x27/0xc0
[ 432.780438] encrypted_read+0x54d/0x7b0
[ 432.780858] ? key_task_permission+0x16e/0x3b0
[ 432.781340] ? encrypted_instantiate+0x8b0/0x8b0
[ 432.781851] ? creds_are_invalid+0x9/0x50
[ 432.782287] ? lookup_user_key+0x19a/0xf50
[ 432.782736] ? __lock_acquire+0x8d1/0x12d0
[ 432.783182] ? key_validate+0xb0/0xb0
[ 432.783602] ? keyctl_read_key+0x174/0x240
[ 432.784058] keyctl_read_key+0x1f2/0x240
[ 432.784486] SyS_keyctl+0x184/0x2a0
[ 432.784875] entry_SYSCALL_64_fastpath+0x1e/0x8b
[ 432.785374] RIP: 0033:0x7f812b6c8259
WARNING: multiple messages have this Message-ID (diff)
From: Eric Biggers <ebiggers3@gmail.com>
To: "André Draszik" <git@andred.net>
Cc: linux-kernel@vger.kernel.org,
Mimi Zohar <zohar@linux.vnet.ibm.com>,
David Howells <dhowells@redhat.com>,
James Morris <james.l.morris@oracle.com>,
"Serge E. Hallyn" <serge@hallyn.com>,
"Theodore Y. Ts'o" <tytso@mit.edu>,
Jaegeuk Kim <jaegeuk@kernel.org>,
Kees Cook <keescook@chromium.org>,
Eric Biggers <ebiggers@google.com>,
linux-integrity@vger.kernel.org, keyrings@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-fscrypt@vger.kernel.org
Subject: Re: [PATCH v3] fscrypt: add support for the encrypted key type
Date: Thu, 25 Jan 2018 16:37:48 -0800 [thread overview]
Message-ID: <20180126003748.f2uhgwhulcltyok6@gmail.com> (raw)
In-Reply-To: <20180118131359.8365-1-git@andred.net>
On Thu, Jan 18, 2018 at 01:13:59PM +0000, Andre Draszik wrote:
> -static int validate_user_key(struct fscrypt_info *crypt_info,
> +static inline struct key *fscrypt_get_encrypted_key(const char *description)
> +{
> + if (IS_ENABLED(CONFIG_ENCRYPTED_KEYS))
> + return request_key(&key_type_encrypted, description, NULL);
> + return ERR_PTR(-ENOKEY);
> +}
> +
> +static int validate_keyring_key(struct fscrypt_info *crypt_info,
> struct fscrypt_context *ctx, u8 *raw_key,
> const char *prefix, int min_keysize)
> {
> char *description;
> struct key *keyring_key;
> - struct fscrypt_key *master_key;
> - const struct user_key_payload *ukp;
> + const u8 *master_key;
> + u32 master_key_len;
> int res;
>
> description = kasprintf(GFP_NOFS, "%s%*phN", prefix,
> @@ -83,39 +93,55 @@ static int validate_user_key(struct fscrypt_info *crypt_info,
> return -ENOMEM;
>
> keyring_key = request_key(&key_type_logon, description, NULL);
> + if (keyring_key == ERR_PTR(-ENOKEY))
> + keyring_key = fscrypt_get_encrypted_key(description);
> kfree(description);
> if (IS_ERR(keyring_key))
> return PTR_ERR(keyring_key);
> down_read(&keyring_key->sem);
>
> - if (keyring_key->type != &key_type_logon) {
> + if (keyring_key->type == &key_type_logon) {
> + const struct user_key_payload *ukp;
> + const struct fscrypt_key *fk;
> +
> + ukp = user_key_payload_locked(keyring_key);
> + if (!ukp) {
> + /* key was revoked before we acquired its semaphore */
> + res = -EKEYREVOKED;
> + goto out;
> + }
> + if (ukp->datalen != sizeof(struct fscrypt_key)) {
> + res = -EINVAL;
> + goto out;
> + }
> + fk = (struct fscrypt_key *)ukp->data;
> + master_key = fk->raw;
> + master_key_len = fk->size;
> + } else if (keyring_key->type == &key_type_encrypted) {
> + const struct encrypted_key_payload *ekp;
> +
> + ekp = keyring_key->payload.data[0];
> + master_key = ekp->decrypted_data;
> + master_key_len = ekp->decrypted_datalen;
> + } else {
> printk_once(KERN_WARNING
> - "%s: key type must be logon\n", __func__);
> + "%s: key type must be logon or encrypted\n",
> + __func__);
> res = -ENOKEY;
> goto out;
> }
> - ukp = user_key_payload_locked(keyring_key);
> - if (!ukp) {
> - /* key was revoked before we acquired its semaphore */
> - res = -EKEYREVOKED;
> - goto out;
> - }
> - if (ukp->datalen != sizeof(struct fscrypt_key)) {
> - res = -EINVAL;
> - goto out;
> - }
> - master_key = (struct fscrypt_key *)ukp->data;
> BUILD_BUG_ON(FS_AES_128_ECB_KEY_SIZE != FS_KEY_DERIVATION_NONCE_SIZE);
>
> - if (master_key->size < min_keysize || master_key->size > FS_MAX_KEY_SIZE
> - || master_key->size % AES_BLOCK_SIZE != 0) {
> + if (master_key_len < min_keysize || master_key_len > FS_MAX_KEY_SIZE
> + || master_key_len % AES_BLOCK_SIZE != 0) {
> printk_once(KERN_WARNING
> - "%s: key size incorrect: %d\n",
> - __func__, master_key->size);
> + "%s: key size incorrect: %u\n",
> + __func__, master_key_len);
> res = -ENOKEY;
> goto out;
> }
> - res = derive_key_aes(ctx->nonce, master_key, raw_key);
> + res = derive_key_aes(ctx->nonce, master_key, master_key_len, raw_key);
> +
> out:
> up_read(&keyring_key->sem);
> key_put(keyring_key);
> @@ -302,12 +328,12 @@ int fscrypt_get_encryption_info(struct inode *inode)
> if (!raw_key)
> goto out;
>
> - res = validate_user_key(crypt_info, &ctx, raw_key, FS_KEY_DESC_PREFIX,
> - keysize);
> + res = validate_keyring_key(crypt_info, &ctx, raw_key,
> + FS_KEY_DESC_PREFIX, keysize);
> if (res && inode->i_sb->s_cop->key_prefix) {
> - int res2 = validate_user_key(crypt_info, &ctx, raw_key,
> - inode->i_sb->s_cop->key_prefix,
> - keysize);
> + int res2 = validate_keyring_key(crypt_info, &ctx, raw_key,
> + inode->i_sb->s_cop->key_prefix,
> + keysize);
> if (res2) {
> if (res2 == -ENOKEY)
> res = -ENOKEY;
> --
> 2.15.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe keyrings" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
The code changes here look fine, but unfortunately there is a lock ordering
problem exposed by using a key type that implements the key_type ->read()
method. The problem is that encrypted_read() can take a page fault during
copy_to_user() while holding the key semaphore, which then (with ext4) can
trigger the start of a jbd2 transaction. Meanwhile
fscrypt_get_encryption_info() can be called from within a jbd2 transaction via
fscrypt_inherit_context(), which results in a different locking order. We
probably will need to fix this by removing the call to
fscrypt_get_encryption_info() from fscrypt_inherit_context().
I've pasted the lockdep report I got below:
[ 432.705339]
[ 432.705681] ======================================================
[ 432.707015] WARNING: possible circular locking dependency detected
[ 432.708155] 4.15.0-rc8-next-20180119-00019-gab7ec46b6936 #31 Not tainted
[ 432.709365] ------------------------------------------------------
[ 432.710482] keyctl/877 is trying to acquire lock:
[ 432.711286] (&mm->mmap_sem){++++}, at: [<00000000e8bba1c7>] __might_fault+0xce/0x1a0
[ 432.712688]
[ 432.712688] but task is already holding lock:
[ 432.713684] (&type->lock_class#2){++++}, at: [<000000008ccfa156>] keyctl_read_key+0x174/0x240
[ 432.715176]
[ 432.715176] which lock already depends on the new lock.
[ 432.715176]
[ 432.716601]
[ 432.716601] the existing dependency chain (in reverse order) is:
[ 432.717901]
[ 432.717901] -> #3 (&type->lock_class#2){++++}:
[ 432.718924] lock_acquire+0xaa/0x170
[ 432.719632] down_read+0x37/0xa0
[ 432.720298] validate_keyring_key.isra.1+0x97/0x410
[ 432.721218] fscrypt_get_encryption_info+0x724/0x1200
[ 432.722233] fscrypt_inherit_context+0x2c1/0x330
[ 432.723067] __ext4_new_inode+0x34f5/0x44d0
[ 432.723830] ext4_create+0x195/0x4a0
[ 432.724499] lookup_open+0xbe2/0x1400
[ 432.725185] path_openat+0xb31/0x1e50
[ 432.725876] do_filp_open+0x175/0x250
[ 432.726572] do_sys_open+0x219/0x3f0
[ 432.727312] entry_SYSCALL_64_fastpath+0x1e/0x8b
[ 432.728153]
[ 432.728153] -> #2 (jbd2_handle){.+.+}:
[ 432.729008] lock_acquire+0xaa/0x170
[ 432.729680] start_this_handle+0x47b/0x1020
[ 432.730463] jbd2__journal_start+0x32e/0x5c0
[ 432.731276] __ext4_journal_start_sb+0xa8/0x190
[ 432.732183] ext4_truncate+0x4dd/0xd00
[ 432.732868] ext4_setattr+0xa62/0x1ac0
[ 432.733528] notify_change+0x672/0xdb0
[ 432.734198] do_truncate+0x111/0x1c0
[ 432.734831] path_openat+0xee6/0x1e50
[ 432.735476] do_filp_open+0x175/0x250
[ 432.736149] do_sys_open+0x219/0x3f0
[ 432.736785] entry_SYSCALL_64_fastpath+0x1e/0x8b
[ 432.737576]
[ 432.737576] -> #1 (&ei->i_mmap_sem){++++}:
[ 432.738508] lock_acquire+0xaa/0x170
[ 432.739148] down_read+0x37/0xa0
[ 432.739735] ext4_filemap_fault+0x7b/0xb0
[ 432.740446] __do_fault+0x7a/0x200
[ 432.741060] __handle_mm_fault+0x6e0/0x2040
[ 432.741801] handle_mm_fault+0x194/0x320
[ 432.742494] __do_page_fault+0x4e1/0xa70
[ 432.743184] page_fault+0x2c/0x60
[ 432.743784] __clear_user+0x3d/0x60
[ 432.744409] clear_user+0x76/0xa0
[ 432.745009] load_elf_binary+0x2bf6/0x2f10
[ 432.745753] search_binary_handler+0x136/0x450
[ 432.746522] do_execveat_common.isra.12+0x1241/0x19c0
[ 432.747339] do_execve+0x2c/0x40
[ 432.747881] try_to_run_init_process+0x12/0x40
[ 432.748611] kernel_init+0xf2/0x180
[ 432.749190] ret_from_fork+0x3a/0x50
[ 432.749785]
[ 432.749785] -> #0 (&mm->mmap_sem){++++}:
[ 432.750576] __lock_acquire+0x8d1/0x12d0
[ 432.751232] lock_acquire+0xaa/0x170
[ 432.751848] __might_fault+0x12b/0x1a0
[ 432.752478] _copy_to_user+0x27/0xc0
[ 432.753080] encrypted_read+0x54d/0x7b0
[ 432.753734] keyctl_read_key+0x1f2/0x240
[ 432.754396] SyS_keyctl+0x184/0x2a0
[ 432.754995] entry_SYSCALL_64_fastpath+0x1e/0x8b
[ 432.755778]
[ 432.755778] other info that might help us debug this:
[ 432.755778]
[ 432.756972] Chain exists of:
[ 432.756972] &mm->mmap_sem --> jbd2_handle --> &type->lock_class#2
[ 432.756972]
[ 432.758498] Possible unsafe locking scenario:
[ 432.758498]
[ 432.759302] CPU0 CPU1
[ 432.759919] ---- ----
[ 432.760536] lock(&type->lock_class#2);
[ 432.761100] lock(jbd2_handle);
[ 432.761926] lock(&type->lock_class#2);
[ 432.762836] lock(&mm->mmap_sem);
[ 432.763348]
[ 432.763348] *** DEADLOCK ***
[ 432.763348]
[ 432.764209] 1 lock held by keyctl/877:
[ 432.764765] #0: (&type->lock_class#2){++++}, at: [<000000008ccfa156>] keyctl_read_key+0x174/0x240
[ 432.766045]
[ 432.766045] stack backtrace:
[ 432.766671] CPU: 1 PID: 877 Comm: keyctl Not tainted 4.15.0-rc8-next-20180119-00019-gab7ec46b6936 #31
[ 432.768021] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
[ 432.769211] Call Trace:
[ 432.769559] dump_stack+0x8d/0xd5
[ 432.770041] print_circular_bug.isra.20+0x321/0x5e0
[ 432.770712] ? save_trace+0xd6/0x250
[ 432.771185] check_prev_add.constprop.27+0xa2a/0x1670
[ 432.771867] ? depot_save_stack+0x2d5/0x490
[ 432.772504] ? check_usage+0x13c0/0x13c0
[ 432.773047] ? trace_hardirqs_on_caller+0x3dc/0x550
[ 432.773712] ? _raw_spin_unlock_irqrestore+0x39/0x60
[ 432.774401] ? depot_save_stack+0x2d5/0x490
[ 432.774979] ? kasan_kmalloc+0x13a/0x170
[ 432.775522] ? validate_chain.isra.24+0x13ee/0x2410
[ 432.776187] validate_chain.isra.24+0x13ee/0x2410
[ 432.776831] ? check_prev_add.constprop.27+0x1670/0x1670
[ 432.777546] __lock_acquire+0x8d1/0x12d0
[ 432.778088] lock_acquire+0xaa/0x170
[ 432.778576] ? __might_fault+0xce/0x1a0
[ 432.779096] __might_fault+0x12b/0x1a0
[ 432.779608] ? __might_fault+0xce/0x1a0
[ 432.780029] _copy_to_user+0x27/0xc0
[ 432.780438] encrypted_read+0x54d/0x7b0
[ 432.780858] ? key_task_permission+0x16e/0x3b0
[ 432.781340] ? encrypted_instantiate+0x8b0/0x8b0
[ 432.781851] ? creds_are_invalid+0x9/0x50
[ 432.782287] ? lookup_user_key+0x19a/0xf50
[ 432.782736] ? __lock_acquire+0x8d1/0x12d0
[ 432.783182] ? key_validate+0xb0/0xb0
[ 432.783602] ? keyctl_read_key+0x174/0x240
[ 432.784058] keyctl_read_key+0x1f2/0x240
[ 432.784486] SyS_keyctl+0x184/0x2a0
[ 432.784875] entry_SYSCALL_64_fastpath+0x1e/0x8b
[ 432.785374] RIP: 0033:0x7f812b6c8259
WARNING: multiple messages have this Message-ID (diff)
From: ebiggers3@gmail.com (Eric Biggers)
To: linux-security-module@vger.kernel.org
Subject: [PATCH v3] fscrypt: add support for the encrypted key type
Date: Thu, 25 Jan 2018 16:37:48 -0800 [thread overview]
Message-ID: <20180126003748.f2uhgwhulcltyok6@gmail.com> (raw)
In-Reply-To: <20180118131359.8365-1-git@andred.net>
On Thu, Jan 18, 2018 at 01:13:59PM +0000, Andr? Draszik wrote:
> -static int validate_user_key(struct fscrypt_info *crypt_info,
> +static inline struct key *fscrypt_get_encrypted_key(const char *description)
> +{
> + if (IS_ENABLED(CONFIG_ENCRYPTED_KEYS))
> + return request_key(&key_type_encrypted, description, NULL);
> + return ERR_PTR(-ENOKEY);
> +}
> +
> +static int validate_keyring_key(struct fscrypt_info *crypt_info,
> struct fscrypt_context *ctx, u8 *raw_key,
> const char *prefix, int min_keysize)
> {
> char *description;
> struct key *keyring_key;
> - struct fscrypt_key *master_key;
> - const struct user_key_payload *ukp;
> + const u8 *master_key;
> + u32 master_key_len;
> int res;
>
> description = kasprintf(GFP_NOFS, "%s%*phN", prefix,
> @@ -83,39 +93,55 @@ static int validate_user_key(struct fscrypt_info *crypt_info,
> return -ENOMEM;
>
> keyring_key = request_key(&key_type_logon, description, NULL);
> + if (keyring_key == ERR_PTR(-ENOKEY))
> + keyring_key = fscrypt_get_encrypted_key(description);
> kfree(description);
> if (IS_ERR(keyring_key))
> return PTR_ERR(keyring_key);
> down_read(&keyring_key->sem);
>
> - if (keyring_key->type != &key_type_logon) {
> + if (keyring_key->type == &key_type_logon) {
> + const struct user_key_payload *ukp;
> + const struct fscrypt_key *fk;
> +
> + ukp = user_key_payload_locked(keyring_key);
> + if (!ukp) {
> + /* key was revoked before we acquired its semaphore */
> + res = -EKEYREVOKED;
> + goto out;
> + }
> + if (ukp->datalen != sizeof(struct fscrypt_key)) {
> + res = -EINVAL;
> + goto out;
> + }
> + fk = (struct fscrypt_key *)ukp->data;
> + master_key = fk->raw;
> + master_key_len = fk->size;
> + } else if (keyring_key->type == &key_type_encrypted) {
> + const struct encrypted_key_payload *ekp;
> +
> + ekp = keyring_key->payload.data[0];
> + master_key = ekp->decrypted_data;
> + master_key_len = ekp->decrypted_datalen;
> + } else {
> printk_once(KERN_WARNING
> - "%s: key type must be logon\n", __func__);
> + "%s: key type must be logon or encrypted\n",
> + __func__);
> res = -ENOKEY;
> goto out;
> }
> - ukp = user_key_payload_locked(keyring_key);
> - if (!ukp) {
> - /* key was revoked before we acquired its semaphore */
> - res = -EKEYREVOKED;
> - goto out;
> - }
> - if (ukp->datalen != sizeof(struct fscrypt_key)) {
> - res = -EINVAL;
> - goto out;
> - }
> - master_key = (struct fscrypt_key *)ukp->data;
> BUILD_BUG_ON(FS_AES_128_ECB_KEY_SIZE != FS_KEY_DERIVATION_NONCE_SIZE);
>
> - if (master_key->size < min_keysize || master_key->size > FS_MAX_KEY_SIZE
> - || master_key->size % AES_BLOCK_SIZE != 0) {
> + if (master_key_len < min_keysize || master_key_len > FS_MAX_KEY_SIZE
> + || master_key_len % AES_BLOCK_SIZE != 0) {
> printk_once(KERN_WARNING
> - "%s: key size incorrect: %d\n",
> - __func__, master_key->size);
> + "%s: key size incorrect: %u\n",
> + __func__, master_key_len);
> res = -ENOKEY;
> goto out;
> }
> - res = derive_key_aes(ctx->nonce, master_key, raw_key);
> + res = derive_key_aes(ctx->nonce, master_key, master_key_len, raw_key);
> +
> out:
> up_read(&keyring_key->sem);
> key_put(keyring_key);
> @@ -302,12 +328,12 @@ int fscrypt_get_encryption_info(struct inode *inode)
> if (!raw_key)
> goto out;
>
> - res = validate_user_key(crypt_info, &ctx, raw_key, FS_KEY_DESC_PREFIX,
> - keysize);
> + res = validate_keyring_key(crypt_info, &ctx, raw_key,
> + FS_KEY_DESC_PREFIX, keysize);
> if (res && inode->i_sb->s_cop->key_prefix) {
> - int res2 = validate_user_key(crypt_info, &ctx, raw_key,
> - inode->i_sb->s_cop->key_prefix,
> - keysize);
> + int res2 = validate_keyring_key(crypt_info, &ctx, raw_key,
> + inode->i_sb->s_cop->key_prefix,
> + keysize);
> if (res2) {
> if (res2 == -ENOKEY)
> res = -ENOKEY;
> --
> 2.15.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe keyrings" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
The code changes here look fine, but unfortunately there is a lock ordering
problem exposed by using a key type that implements the key_type ->read()
method. The problem is that encrypted_read() can take a page fault during
copy_to_user() while holding the key semaphore, which then (with ext4) can
trigger the start of a jbd2 transaction. Meanwhile
fscrypt_get_encryption_info() can be called from within a jbd2 transaction via
fscrypt_inherit_context(), which results in a different locking order. We
probably will need to fix this by removing the call to
fscrypt_get_encryption_info() from fscrypt_inherit_context().
I've pasted the lockdep report I got below:
[ 432.705339]
[ 432.705681] ======================================================
[ 432.707015] WARNING: possible circular locking dependency detected
[ 432.708155] 4.15.0-rc8-next-20180119-00019-gab7ec46b6936 #31 Not tainted
[ 432.709365] ------------------------------------------------------
[ 432.710482] keyctl/877 is trying to acquire lock:
[ 432.711286] (&mm->mmap_sem){++++}, at: [<00000000e8bba1c7>] __might_fault+0xce/0x1a0
[ 432.712688]
[ 432.712688] but task is already holding lock:
[ 432.713684] (&type->lock_class#2){++++}, at: [<000000008ccfa156>] keyctl_read_key+0x174/0x240
[ 432.715176]
[ 432.715176] which lock already depends on the new lock.
[ 432.715176]
[ 432.716601]
[ 432.716601] the existing dependency chain (in reverse order) is:
[ 432.717901]
[ 432.717901] -> #3 (&type->lock_class#2){++++}:
[ 432.718924] lock_acquire+0xaa/0x170
[ 432.719632] down_read+0x37/0xa0
[ 432.720298] validate_keyring_key.isra.1+0x97/0x410
[ 432.721218] fscrypt_get_encryption_info+0x724/0x1200
[ 432.722233] fscrypt_inherit_context+0x2c1/0x330
[ 432.723067] __ext4_new_inode+0x34f5/0x44d0
[ 432.723830] ext4_create+0x195/0x4a0
[ 432.724499] lookup_open+0xbe2/0x1400
[ 432.725185] path_openat+0xb31/0x1e50
[ 432.725876] do_filp_open+0x175/0x250
[ 432.726572] do_sys_open+0x219/0x3f0
[ 432.727312] entry_SYSCALL_64_fastpath+0x1e/0x8b
[ 432.728153]
[ 432.728153] -> #2 (jbd2_handle){.+.+}:
[ 432.729008] lock_acquire+0xaa/0x170
[ 432.729680] start_this_handle+0x47b/0x1020
[ 432.730463] jbd2__journal_start+0x32e/0x5c0
[ 432.731276] __ext4_journal_start_sb+0xa8/0x190
[ 432.732183] ext4_truncate+0x4dd/0xd00
[ 432.732868] ext4_setattr+0xa62/0x1ac0
[ 432.733528] notify_change+0x672/0xdb0
[ 432.734198] do_truncate+0x111/0x1c0
[ 432.734831] path_openat+0xee6/0x1e50
[ 432.735476] do_filp_open+0x175/0x250
[ 432.736149] do_sys_open+0x219/0x3f0
[ 432.736785] entry_SYSCALL_64_fastpath+0x1e/0x8b
[ 432.737576]
[ 432.737576] -> #1 (&ei->i_mmap_sem){++++}:
[ 432.738508] lock_acquire+0xaa/0x170
[ 432.739148] down_read+0x37/0xa0
[ 432.739735] ext4_filemap_fault+0x7b/0xb0
[ 432.740446] __do_fault+0x7a/0x200
[ 432.741060] __handle_mm_fault+0x6e0/0x2040
[ 432.741801] handle_mm_fault+0x194/0x320
[ 432.742494] __do_page_fault+0x4e1/0xa70
[ 432.743184] page_fault+0x2c/0x60
[ 432.743784] __clear_user+0x3d/0x60
[ 432.744409] clear_user+0x76/0xa0
[ 432.745009] load_elf_binary+0x2bf6/0x2f10
[ 432.745753] search_binary_handler+0x136/0x450
[ 432.746522] do_execveat_common.isra.12+0x1241/0x19c0
[ 432.747339] do_execve+0x2c/0x40
[ 432.747881] try_to_run_init_process+0x12/0x40
[ 432.748611] kernel_init+0xf2/0x180
[ 432.749190] ret_from_fork+0x3a/0x50
[ 432.749785]
[ 432.749785] -> #0 (&mm->mmap_sem){++++}:
[ 432.750576] __lock_acquire+0x8d1/0x12d0
[ 432.751232] lock_acquire+0xaa/0x170
[ 432.751848] __might_fault+0x12b/0x1a0
[ 432.752478] _copy_to_user+0x27/0xc0
[ 432.753080] encrypted_read+0x54d/0x7b0
[ 432.753734] keyctl_read_key+0x1f2/0x240
[ 432.754396] SyS_keyctl+0x184/0x2a0
[ 432.754995] entry_SYSCALL_64_fastpath+0x1e/0x8b
[ 432.755778]
[ 432.755778] other info that might help us debug this:
[ 432.755778]
[ 432.756972] Chain exists of:
[ 432.756972] &mm->mmap_sem --> jbd2_handle --> &type->lock_class#2
[ 432.756972]
[ 432.758498] Possible unsafe locking scenario:
[ 432.758498]
[ 432.759302] CPU0 CPU1
[ 432.759919] ---- ----
[ 432.760536] lock(&type->lock_class#2);
[ 432.761100] lock(jbd2_handle);
[ 432.761926] lock(&type->lock_class#2);
[ 432.762836] lock(&mm->mmap_sem);
[ 432.763348]
[ 432.763348] *** DEADLOCK ***
[ 432.763348]
[ 432.764209] 1 lock held by keyctl/877:
[ 432.764765] #0: (&type->lock_class#2){++++}, at: [<000000008ccfa156>] keyctl_read_key+0x174/0x240
[ 432.766045]
[ 432.766045] stack backtrace:
[ 432.766671] CPU: 1 PID: 877 Comm: keyctl Not tainted 4.15.0-rc8-next-20180119-00019-gab7ec46b6936 #31
[ 432.768021] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
[ 432.769211] Call Trace:
[ 432.769559] dump_stack+0x8d/0xd5
[ 432.770041] print_circular_bug.isra.20+0x321/0x5e0
[ 432.770712] ? save_trace+0xd6/0x250
[ 432.771185] check_prev_add.constprop.27+0xa2a/0x1670
[ 432.771867] ? depot_save_stack+0x2d5/0x490
[ 432.772504] ? check_usage+0x13c0/0x13c0
[ 432.773047] ? trace_hardirqs_on_caller+0x3dc/0x550
[ 432.773712] ? _raw_spin_unlock_irqrestore+0x39/0x60
[ 432.774401] ? depot_save_stack+0x2d5/0x490
[ 432.774979] ? kasan_kmalloc+0x13a/0x170
[ 432.775522] ? validate_chain.isra.24+0x13ee/0x2410
[ 432.776187] validate_chain.isra.24+0x13ee/0x2410
[ 432.776831] ? check_prev_add.constprop.27+0x1670/0x1670
[ 432.777546] __lock_acquire+0x8d1/0x12d0
[ 432.778088] lock_acquire+0xaa/0x170
[ 432.778576] ? __might_fault+0xce/0x1a0
[ 432.779096] __might_fault+0x12b/0x1a0
[ 432.779608] ? __might_fault+0xce/0x1a0
[ 432.780029] _copy_to_user+0x27/0xc0
[ 432.780438] encrypted_read+0x54d/0x7b0
[ 432.780858] ? key_task_permission+0x16e/0x3b0
[ 432.781340] ? encrypted_instantiate+0x8b0/0x8b0
[ 432.781851] ? creds_are_invalid+0x9/0x50
[ 432.782287] ? lookup_user_key+0x19a/0xf50
[ 432.782736] ? __lock_acquire+0x8d1/0x12d0
[ 432.783182] ? key_validate+0xb0/0xb0
[ 432.783602] ? keyctl_read_key+0x174/0x240
[ 432.784058] keyctl_read_key+0x1f2/0x240
[ 432.784486] SyS_keyctl+0x184/0x2a0
[ 432.784875] entry_SYSCALL_64_fastpath+0x1e/0x8b
[ 432.785374] RIP: 0033:0x7f812b6c8259
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
WARNING: multiple messages have this Message-ID (diff)
From: Eric Biggers <ebiggers3@gmail.com>
To: "André Draszik" <git@andred.net>
Cc: linux-kernel@vger.kernel.org,
Mimi Zohar <zohar@linux.vnet.ibm.com>,
David Howells <dhowells@redhat.com>,
James Morris <james.l.morris@oracle.com>,
"Serge E. Hallyn" <serge@hallyn.com>,
"Theodore Y. Ts'o" <tytso@mit.edu>,
Jaegeuk Kim <jaegeuk@kernel.org>,
Kees Cook <keescook@chromium.org>,
Eric Biggers <ebiggers@google.com>,
linux-integrity@vger.kernel.org, keyrings@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-fscrypt@vger.kernel.org
Subject: Re: [PATCH v3] fscrypt: add support for the encrypted key type
Date: Thu, 25 Jan 2018 16:37:48 -0800 [thread overview]
Message-ID: <20180126003748.f2uhgwhulcltyok6@gmail.com> (raw)
In-Reply-To: <20180118131359.8365-1-git@andred.net>
On Thu, Jan 18, 2018 at 01:13:59PM +0000, André Draszik wrote:
> -static int validate_user_key(struct fscrypt_info *crypt_info,
> +static inline struct key *fscrypt_get_encrypted_key(const char *description)
> +{
> + if (IS_ENABLED(CONFIG_ENCRYPTED_KEYS))
> + return request_key(&key_type_encrypted, description, NULL);
> + return ERR_PTR(-ENOKEY);
> +}
> +
> +static int validate_keyring_key(struct fscrypt_info *crypt_info,
> struct fscrypt_context *ctx, u8 *raw_key,
> const char *prefix, int min_keysize)
> {
> char *description;
> struct key *keyring_key;
> - struct fscrypt_key *master_key;
> - const struct user_key_payload *ukp;
> + const u8 *master_key;
> + u32 master_key_len;
> int res;
>
> description = kasprintf(GFP_NOFS, "%s%*phN", prefix,
> @@ -83,39 +93,55 @@ static int validate_user_key(struct fscrypt_info *crypt_info,
> return -ENOMEM;
>
> keyring_key = request_key(&key_type_logon, description, NULL);
> + if (keyring_key == ERR_PTR(-ENOKEY))
> + keyring_key = fscrypt_get_encrypted_key(description);
> kfree(description);
> if (IS_ERR(keyring_key))
> return PTR_ERR(keyring_key);
> down_read(&keyring_key->sem);
>
> - if (keyring_key->type != &key_type_logon) {
> + if (keyring_key->type == &key_type_logon) {
> + const struct user_key_payload *ukp;
> + const struct fscrypt_key *fk;
> +
> + ukp = user_key_payload_locked(keyring_key);
> + if (!ukp) {
> + /* key was revoked before we acquired its semaphore */
> + res = -EKEYREVOKED;
> + goto out;
> + }
> + if (ukp->datalen != sizeof(struct fscrypt_key)) {
> + res = -EINVAL;
> + goto out;
> + }
> + fk = (struct fscrypt_key *)ukp->data;
> + master_key = fk->raw;
> + master_key_len = fk->size;
> + } else if (keyring_key->type == &key_type_encrypted) {
> + const struct encrypted_key_payload *ekp;
> +
> + ekp = keyring_key->payload.data[0];
> + master_key = ekp->decrypted_data;
> + master_key_len = ekp->decrypted_datalen;
> + } else {
> printk_once(KERN_WARNING
> - "%s: key type must be logon\n", __func__);
> + "%s: key type must be logon or encrypted\n",
> + __func__);
> res = -ENOKEY;
> goto out;
> }
> - ukp = user_key_payload_locked(keyring_key);
> - if (!ukp) {
> - /* key was revoked before we acquired its semaphore */
> - res = -EKEYREVOKED;
> - goto out;
> - }
> - if (ukp->datalen != sizeof(struct fscrypt_key)) {
> - res = -EINVAL;
> - goto out;
> - }
> - master_key = (struct fscrypt_key *)ukp->data;
> BUILD_BUG_ON(FS_AES_128_ECB_KEY_SIZE != FS_KEY_DERIVATION_NONCE_SIZE);
>
> - if (master_key->size < min_keysize || master_key->size > FS_MAX_KEY_SIZE
> - || master_key->size % AES_BLOCK_SIZE != 0) {
> + if (master_key_len < min_keysize || master_key_len > FS_MAX_KEY_SIZE
> + || master_key_len % AES_BLOCK_SIZE != 0) {
> printk_once(KERN_WARNING
> - "%s: key size incorrect: %d\n",
> - __func__, master_key->size);
> + "%s: key size incorrect: %u\n",
> + __func__, master_key_len);
> res = -ENOKEY;
> goto out;
> }
> - res = derive_key_aes(ctx->nonce, master_key, raw_key);
> + res = derive_key_aes(ctx->nonce, master_key, master_key_len, raw_key);
> +
> out:
> up_read(&keyring_key->sem);
> key_put(keyring_key);
> @@ -302,12 +328,12 @@ int fscrypt_get_encryption_info(struct inode *inode)
> if (!raw_key)
> goto out;
>
> - res = validate_user_key(crypt_info, &ctx, raw_key, FS_KEY_DESC_PREFIX,
> - keysize);
> + res = validate_keyring_key(crypt_info, &ctx, raw_key,
> + FS_KEY_DESC_PREFIX, keysize);
> if (res && inode->i_sb->s_cop->key_prefix) {
> - int res2 = validate_user_key(crypt_info, &ctx, raw_key,
> - inode->i_sb->s_cop->key_prefix,
> - keysize);
> + int res2 = validate_keyring_key(crypt_info, &ctx, raw_key,
> + inode->i_sb->s_cop->key_prefix,
> + keysize);
> if (res2) {
> if (res2 == -ENOKEY)
> res = -ENOKEY;
> --
> 2.15.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe keyrings" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
The code changes here look fine, but unfortunately there is a lock ordering
problem exposed by using a key type that implements the key_type ->read()
method. The problem is that encrypted_read() can take a page fault during
copy_to_user() while holding the key semaphore, which then (with ext4) can
trigger the start of a jbd2 transaction. Meanwhile
fscrypt_get_encryption_info() can be called from within a jbd2 transaction via
fscrypt_inherit_context(), which results in a different locking order. We
probably will need to fix this by removing the call to
fscrypt_get_encryption_info() from fscrypt_inherit_context().
I've pasted the lockdep report I got below:
[ 432.705339]
[ 432.705681] ======================================================
[ 432.707015] WARNING: possible circular locking dependency detected
[ 432.708155] 4.15.0-rc8-next-20180119-00019-gab7ec46b6936 #31 Not tainted
[ 432.709365] ------------------------------------------------------
[ 432.710482] keyctl/877 is trying to acquire lock:
[ 432.711286] (&mm->mmap_sem){++++}, at: [<00000000e8bba1c7>] __might_fault+0xce/0x1a0
[ 432.712688]
[ 432.712688] but task is already holding lock:
[ 432.713684] (&type->lock_class#2){++++}, at: [<000000008ccfa156>] keyctl_read_key+0x174/0x240
[ 432.715176]
[ 432.715176] which lock already depends on the new lock.
[ 432.715176]
[ 432.716601]
[ 432.716601] the existing dependency chain (in reverse order) is:
[ 432.717901]
[ 432.717901] -> #3 (&type->lock_class#2){++++}:
[ 432.718924] lock_acquire+0xaa/0x170
[ 432.719632] down_read+0x37/0xa0
[ 432.720298] validate_keyring_key.isra.1+0x97/0x410
[ 432.721218] fscrypt_get_encryption_info+0x724/0x1200
[ 432.722233] fscrypt_inherit_context+0x2c1/0x330
[ 432.723067] __ext4_new_inode+0x34f5/0x44d0
[ 432.723830] ext4_create+0x195/0x4a0
[ 432.724499] lookup_open+0xbe2/0x1400
[ 432.725185] path_openat+0xb31/0x1e50
[ 432.725876] do_filp_open+0x175/0x250
[ 432.726572] do_sys_open+0x219/0x3f0
[ 432.727312] entry_SYSCALL_64_fastpath+0x1e/0x8b
[ 432.728153]
[ 432.728153] -> #2 (jbd2_handle){.+.+}:
[ 432.729008] lock_acquire+0xaa/0x170
[ 432.729680] start_this_handle+0x47b/0x1020
[ 432.730463] jbd2__journal_start+0x32e/0x5c0
[ 432.731276] __ext4_journal_start_sb+0xa8/0x190
[ 432.732183] ext4_truncate+0x4dd/0xd00
[ 432.732868] ext4_setattr+0xa62/0x1ac0
[ 432.733528] notify_change+0x672/0xdb0
[ 432.734198] do_truncate+0x111/0x1c0
[ 432.734831] path_openat+0xee6/0x1e50
[ 432.735476] do_filp_open+0x175/0x250
[ 432.736149] do_sys_open+0x219/0x3f0
[ 432.736785] entry_SYSCALL_64_fastpath+0x1e/0x8b
[ 432.737576]
[ 432.737576] -> #1 (&ei->i_mmap_sem){++++}:
[ 432.738508] lock_acquire+0xaa/0x170
[ 432.739148] down_read+0x37/0xa0
[ 432.739735] ext4_filemap_fault+0x7b/0xb0
[ 432.740446] __do_fault+0x7a/0x200
[ 432.741060] __handle_mm_fault+0x6e0/0x2040
[ 432.741801] handle_mm_fault+0x194/0x320
[ 432.742494] __do_page_fault+0x4e1/0xa70
[ 432.743184] page_fault+0x2c/0x60
[ 432.743784] __clear_user+0x3d/0x60
[ 432.744409] clear_user+0x76/0xa0
[ 432.745009] load_elf_binary+0x2bf6/0x2f10
[ 432.745753] search_binary_handler+0x136/0x450
[ 432.746522] do_execveat_common.isra.12+0x1241/0x19c0
[ 432.747339] do_execve+0x2c/0x40
[ 432.747881] try_to_run_init_process+0x12/0x40
[ 432.748611] kernel_init+0xf2/0x180
[ 432.749190] ret_from_fork+0x3a/0x50
[ 432.749785]
[ 432.749785] -> #0 (&mm->mmap_sem){++++}:
[ 432.750576] __lock_acquire+0x8d1/0x12d0
[ 432.751232] lock_acquire+0xaa/0x170
[ 432.751848] __might_fault+0x12b/0x1a0
[ 432.752478] _copy_to_user+0x27/0xc0
[ 432.753080] encrypted_read+0x54d/0x7b0
[ 432.753734] keyctl_read_key+0x1f2/0x240
[ 432.754396] SyS_keyctl+0x184/0x2a0
[ 432.754995] entry_SYSCALL_64_fastpath+0x1e/0x8b
[ 432.755778]
[ 432.755778] other info that might help us debug this:
[ 432.755778]
[ 432.756972] Chain exists of:
[ 432.756972] &mm->mmap_sem --> jbd2_handle --> &type->lock_class#2
[ 432.756972]
[ 432.758498] Possible unsafe locking scenario:
[ 432.758498]
[ 432.759302] CPU0 CPU1
[ 432.759919] ---- ----
[ 432.760536] lock(&type->lock_class#2);
[ 432.761100] lock(jbd2_handle);
[ 432.761926] lock(&type->lock_class#2);
[ 432.762836] lock(&mm->mmap_sem);
[ 432.763348]
[ 432.763348] *** DEADLOCK ***
[ 432.763348]
[ 432.764209] 1 lock held by keyctl/877:
[ 432.764765] #0: (&type->lock_class#2){++++}, at: [<000000008ccfa156>] keyctl_read_key+0x174/0x240
[ 432.766045]
[ 432.766045] stack backtrace:
[ 432.766671] CPU: 1 PID: 877 Comm: keyctl Not tainted 4.15.0-rc8-next-20180119-00019-gab7ec46b6936 #31
[ 432.768021] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
[ 432.769211] Call Trace:
[ 432.769559] dump_stack+0x8d/0xd5
[ 432.770041] print_circular_bug.isra.20+0x321/0x5e0
[ 432.770712] ? save_trace+0xd6/0x250
[ 432.771185] check_prev_add.constprop.27+0xa2a/0x1670
[ 432.771867] ? depot_save_stack+0x2d5/0x490
[ 432.772504] ? check_usage+0x13c0/0x13c0
[ 432.773047] ? trace_hardirqs_on_caller+0x3dc/0x550
[ 432.773712] ? _raw_spin_unlock_irqrestore+0x39/0x60
[ 432.774401] ? depot_save_stack+0x2d5/0x490
[ 432.774979] ? kasan_kmalloc+0x13a/0x170
[ 432.775522] ? validate_chain.isra.24+0x13ee/0x2410
[ 432.776187] validate_chain.isra.24+0x13ee/0x2410
[ 432.776831] ? check_prev_add.constprop.27+0x1670/0x1670
[ 432.777546] __lock_acquire+0x8d1/0x12d0
[ 432.778088] lock_acquire+0xaa/0x170
[ 432.778576] ? __might_fault+0xce/0x1a0
[ 432.779096] __might_fault+0x12b/0x1a0
[ 432.779608] ? __might_fault+0xce/0x1a0
[ 432.780029] _copy_to_user+0x27/0xc0
[ 432.780438] encrypted_read+0x54d/0x7b0
[ 432.780858] ? key_task_permission+0x16e/0x3b0
[ 432.781340] ? encrypted_instantiate+0x8b0/0x8b0
[ 432.781851] ? creds_are_invalid+0x9/0x50
[ 432.782287] ? lookup_user_key+0x19a/0xf50
[ 432.782736] ? __lock_acquire+0x8d1/0x12d0
[ 432.783182] ? key_validate+0xb0/0xb0
[ 432.783602] ? keyctl_read_key+0x174/0x240
[ 432.784058] keyctl_read_key+0x1f2/0x240
[ 432.784486] SyS_keyctl+0x184/0x2a0
[ 432.784875] entry_SYSCALL_64_fastpath+0x1e/0x8b
[ 432.785374] RIP: 0033:0x7f812b6c8259
next prev parent reply other threads:[~2018-01-26 0:37 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-01-18 13:13 [PATCH v3] fscrypt: add support for the encrypted key type André Draszik
2018-01-18 13:13 ` André Draszik
2018-01-18 13:13 ` André Draszik
2018-01-18 13:13 ` André Draszik
2018-01-26 0:36 ` Eric Biggers
2018-01-26 0:36 ` Eric Biggers
2018-01-26 0:36 ` Eric Biggers
2018-01-26 0:36 ` Eric Biggers
2018-01-26 0:36 ` Eric Biggers
2018-01-26 0:37 ` Eric Biggers [this message]
2018-01-26 0:37 ` Eric Biggers
2018-01-26 0:37 ` Eric Biggers
2018-01-26 0:37 ` Eric Biggers
2018-01-26 0:37 ` Eric Biggers
2018-01-29 18:19 ` Eric Biggers
2018-01-29 18:19 ` Eric Biggers
2018-01-29 18:19 ` Eric Biggers
2018-01-29 18:19 ` Eric Biggers
2018-01-29 18:19 ` Eric Biggers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180126003748.f2uhgwhulcltyok6@gmail.com \
--to=ebiggers3@gmail.com \
--cc=dhowells@redhat.com \
--cc=ebiggers@google.com \
--cc=git@andred.net \
--cc=jaegeuk@kernel.org \
--cc=james.l.morris@oracle.com \
--cc=keescook@chromium.org \
--cc=keyrings@vger.kernel.org \
--cc=linux-fscrypt@vger.kernel.org \
--cc=linux-integrity@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=serge@hallyn.com \
--cc=tytso@mit.edu \
--cc=zohar@linux.vnet.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.