[PATCH v2 0/2] KEYS: Read keys to internal buffer & then copy to userspace

[PATCH v2 0/2] KEYS: Read keys to internal buffer & then copy to userspace

[Waiman Long]

v2:
 - Handle NULL buffer and buflen properly in patch 1.
 - Fix a bug in big_key.c.
 - Add patch 2 to handle arbitrary large user-supplied buflen.

The current security key read methods are called with the key semaphore
held.  The methods then copy out the key data to userspace which is
subjected to page fault and may acquire the mmap semaphore. That can
result in circular lock dependency and hence a chance to get into
deadlock.

To avoid such a deadlock, an internal buffer is now allocated for getting
out the necessary data first. After releasing the key semaphore, the
key data are then copied out to userspace sidestepping the circular
lock dependency.

The keyutils test suite was run and the test passed with these patchset
applied without any falure.

Waiman Long (2):
  KEYS: Don't write out to userspace while holding key semaphore
  KEYS: Avoid false positive ENOMEM error on key read

 include/linux/key-type.h                  |  2 +-
 security/keys/big_key.c                   | 11 ++---
 security/keys/encrypted-keys/encrypted.c  |  7 ++-
 security/keys/keyctl.c                    | 54 ++++++++++++++++++++++-
 security/keys/keyring.c                   |  6 +--
 security/keys/request_key_auth.c          |  7 ++-
 security/keys/trusted-keys/trusted_tpm1.c | 14 +-----
 security/keys/user_defined.c              |  5 +--
 8 files changed, 68 insertions(+), 38 deletions(-)

-- 
2.18.1

[PATCH v2 1/2] KEYS: Don't write out to userspace while holding key semaphore

[Waiman Long]

A lockdep circular locking dependency report was seen when running a
keyutils test:

[12537.027242] ======================================================
[12537.059309] WARNING: possible circular locking dependency detected
[12537.088148] 4.18.0-147.7.1.el8_1.x86_64+debug #1 Tainted: G OE    --------- -  -
[12537.125253] ------------------------------------------------------
[12537.153189] keyctl/25598 is trying to acquire lock:
[12537.175087] 000000007c39f96c (&mm->mmap_sem){++++}, at: __might_fault+0xc4/0x1b0
[12537.208365]
[12537.208365] but task is already holding lock:
[12537.234507] 000000003de5b58d (&type->lock_class){++++}, at: keyctl_read_key+0x15a/0x220
[12537.270476]
[12537.270476] which lock already depends on the new lock.
[12537.270476]
[12537.307209]
[12537.307209] the existing dependency chain (in reverse order) is:
[12537.340754]
[12537.340754] -> #3 (&type->lock_class){++++}:
[12537.367434]        down_write+0x4d/0x110
[12537.385202]        __key_link_begin+0x87/0x280
[12537.405232]        request_key_and_link+0x483/0xf70
[12537.427221]        request_key+0x3c/0x80
[12537.444839]        dns_query+0x1db/0x5a5 [dns_resolver]
[12537.468445]        dns_resolve_server_name_to_ip+0x1e1/0x4d0 [cifs]
[12537.496731]        cifs_reconnect+0xe04/0x2500 [cifs]
[12537.519418]        cifs_readv_from_socket+0x461/0x690 [cifs]
[12537.546263]        cifs_read_from_socket+0xa0/0xe0 [cifs]
[12537.573551]        cifs_demultiplex_thread+0x311/0x2db0 [cifs]
[12537.601045]        kthread+0x30c/0x3d0
[12537.617906]        ret_from_fork+0x3a/0x50
[12537.636225]
[12537.636225] -> #2 (root_key_user.cons_lock){+.+.}:
[12537.664525]        __mutex_lock+0x105/0x11f0
[12537.683734]        request_key_and_link+0x35a/0xf70
[12537.705640]        request_key+0x3c/0x80
[12537.723304]        dns_query+0x1db/0x5a5 [dns_resolver]
[12537.746773]        dns_resolve_server_name_to_ip+0x1e1/0x4d0 [cifs]
[12537.775607]        cifs_reconnect+0xe04/0x2500 [cifs]
[12537.798322]        cifs_readv_from_socket+0x461/0x690 [cifs]
[12537.823369]        cifs_read_from_socket+0xa0/0xe0 [cifs]
[12537.847262]        cifs_demultiplex_thread+0x311/0x2db0 [cifs]
[12537.873477]        kthread+0x30c/0x3d0
[12537.890281]        ret_from_fork+0x3a/0x50
[12537.908649]
[12537.908649] -> #1 (&tcp_ses->srv_mutex){+.+.}:
[12537.935225]        __mutex_lock+0x105/0x11f0
[12537.954450]        cifs_call_async+0x102/0x7f0 [cifs]
[12537.977250]        smb2_async_readv+0x6c3/0xc90 [cifs]
[12538.000659]        cifs_readpages+0x120a/0x1e50 [cifs]
[12538.023920]        read_pages+0xf5/0x560
[12538.041583]        __do_page_cache_readahead+0x41d/0x4b0
[12538.067047]        ondemand_readahead+0x44c/0xc10
[12538.092069]        filemap_fault+0xec1/0x1830
[12538.111637]        __do_fault+0x82/0x260
[12538.129216]        do_fault+0x419/0xfb0
[12538.146390]        __handle_mm_fault+0x862/0xdf0
[12538.167408]        handle_mm_fault+0x154/0x550
[12538.187401]        __do_page_fault+0x42f/0xa60
[12538.207395]        do_page_fault+0x38/0x5e0
[12538.225777]        page_fault+0x1e/0x30
[12538.243010]
[12538.243010] -> #0 (&mm->mmap_sem){++++}:
[12538.267875]        lock_acquire+0x14c/0x420
[12538.286848]        __might_fault+0x119/0x1b0
[12538.306006]        keyring_read_iterator+0x7e/0x170
[12538.327936]        assoc_array_subtree_iterate+0x97/0x280
[12538.352154]        keyring_read+0xe9/0x110
[12538.370558]        keyctl_read_key+0x1b9/0x220
[12538.391470]        do_syscall_64+0xa5/0x4b0
[12538.410511]        entry_SYSCALL_64_after_hwframe+0x6a/0xdf
[12538.435535]
[12538.435535] other info that might help us debug this:
[12538.435535]
[12538.472829] Chain exists of:
[12538.472829]   &mm->mmap_sem --> root_key_user.cons_lock --> &type->lock_class
[12538.472829]
[12538.524820]  Possible unsafe locking scenario:
[12538.524820]
[12538.551431]        CPU0                    CPU1
[12538.572654]        ----                    ----
[12538.595865]   lock(&type->lock_class);
[12538.613737]                                lock(root_key_user.cons_lock);
[12538.644234]                                lock(&type->lock_class);
[12538.672410]   lock(&mm->mmap_sem);
[12538.687758]
[12538.687758]  *** DEADLOCK ***
[12538.687758]
[12538.714455] 1 lock held by keyctl/25598:
[12538.732097]  #0: 000000003de5b58d (&type->lock_class){++++}, at: keyctl_read_key+0x15a/0x220
[12538.770573]
[12538.770573] stack backtrace:
[12538.790136] CPU: 2 PID: 25598 Comm: keyctl Kdump: loaded Tainted: G
[12538.844855] Hardware name: HP ProLiant DL360 Gen9/ProLiant DL360 Gen9, BIOS P89 12/27/2015
[12538.881963] Call Trace:
[12538.892897]  dump_stack+0x9a/0xf0
[12538.907908]  print_circular_bug.isra.25.cold.50+0x1bc/0x279
[12538.932891]  ? save_trace+0xd6/0x250
[12538.948979]  check_prev_add.constprop.32+0xc36/0x14f0
[12538.971643]  ? keyring_compare_object+0x104/0x190
[12538.992738]  ? check_usage+0x550/0x550
[12539.009845]  ? sched_clock+0x5/0x10
[12539.025484]  ? sched_clock_cpu+0x18/0x1e0
[12539.043555]  __lock_acquire+0x1f12/0x38d0
[12539.061551]  ? trace_hardirqs_on+0x10/0x10
[12539.080554]  lock_acquire+0x14c/0x420
[12539.100330]  ? __might_fault+0xc4/0x1b0
[12539.119079]  __might_fault+0x119/0x1b0
[12539.135869]  ? __might_fault+0xc4/0x1b0
[12539.153234]  keyring_read_iterator+0x7e/0x170
[12539.172787]  ? keyring_read+0x110/0x110
[12539.190059]  assoc_array_subtree_iterate+0x97/0x280
[12539.211526]  keyring_read+0xe9/0x110
[12539.227561]  ? keyring_gc_check_iterator+0xc0/0xc0
[12539.249076]  keyctl_read_key+0x1b9/0x220
[12539.266660]  do_syscall_64+0xa5/0x4b0
[12539.283091]  entry_SYSCALL_64_after_hwframe+0x6a/0xdf

One way to prevent this deadlock scenario from happening is to not
allow writing to userspace while holding the key semaphore. Instead,
an internal buffer is allocated for getting the keys out from the
read method first before copying them out to userspace without holding
the lock.

That requires taking out the __user modifier from the read methods as
well as additional changes to not use any userspace write helpers.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 include/linux/key-type.h                  |  2 +-
 security/keys/big_key.c                   | 11 +++------
 security/keys/encrypted-keys/encrypted.c  |  7 +++---
 security/keys/keyctl.c                    | 30 +++++++++++++++++++++--
 security/keys/keyring.c                   |  6 +----
 security/keys/request_key_auth.c          |  7 +++---
 security/keys/trusted-keys/trusted_tpm1.c | 14 ++---------
 security/keys/user_defined.c              |  5 ++--
 8 files changed, 44 insertions(+), 38 deletions(-)

diff --git a/include/linux/key-type.h b/include/linux/key-type.h
index 4ded94bcf274..2ab2d6d6aeab 100644
--- a/include/linux/key-type.h
+++ b/include/linux/key-type.h
@@ -127,7 +127,7 @@ struct key_type {
 	 *   much is copied into the buffer
 	 * - shouldn't do the copy if the buffer is NULL
 	 */
-	long (*read)(const struct key *key, char __user *buffer, size_t buflen);
+	long (*read)(const struct key *key, char *buffer, size_t buflen);
 
 	/* handle request_key() for this type instead of invoking
 	 * /sbin/request-key (optional)
diff --git a/security/keys/big_key.c b/security/keys/big_key.c
index 001abe530a0d..82008f900930 100644
--- a/security/keys/big_key.c
+++ b/security/keys/big_key.c
@@ -352,7 +352,7 @@ void big_key_describe(const struct key *key, struct seq_file *m)
  * read the key data
  * - the key's semaphore is read-locked
  */
-long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
+long big_key_read(const struct key *key, char *buffer, size_t buflen)
 {
 	size_t datalen = (size_t)key->payload.data[big_key_len];
 	long ret;
@@ -391,9 +391,8 @@ long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
 
 		ret = datalen;
 
-		/* copy decrypted data to user */
-		if (copy_to_user(buffer, buf->virt, datalen) != 0)
-			ret = -EFAULT;
+		/* copy out decrypted data */
+		memcpy(buffer, buf->virt, datalen);
 
 err_fput:
 		fput(file);
@@ -401,9 +400,7 @@ long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
 		big_key_free_buffer(buf);
 	} else {
 		ret = datalen;
-		if (copy_to_user(buffer, key->payload.data[big_key_data],
-				 datalen) != 0)
-			ret = -EFAULT;
+		memcpy(buffer, key->payload.data[big_key_data], datalen);
 	}
 
 	return ret;
diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
index 60720f58cbe0..f6797ba44bf7 100644
--- a/security/keys/encrypted-keys/encrypted.c
+++ b/security/keys/encrypted-keys/encrypted.c
@@ -902,14 +902,14 @@ static int encrypted_update(struct key *key, struct key_preparsed_payload *prep)
 }
 
 /*
- * encrypted_read - format and copy the encrypted data to userspace
+ * encrypted_read - format and copy out the encrypted data
  *
  * The resulting datablob format is:
  * <master-key name> <decrypted data length> <encrypted iv> <encrypted data>
  *
  * On success, return to userspace the encrypted key datablob size.
  */
-static long encrypted_read(const struct key *key, char __user *buffer,
+static long encrypted_read(const struct key *key, char *buffer,
 			   size_t buflen)
 {
 	struct encrypted_key_payload *epayload;
@@ -957,8 +957,7 @@ static long encrypted_read(const struct key *key, char __user *buffer,
 	key_put(mkey);
 	memzero_explicit(derived_key, sizeof(derived_key));
 
-	if (copy_to_user(buffer, ascii_buf, asciiblob_len) != 0)
-		ret = -EFAULT;
+	memcpy(buffer, ascii_buf, asciiblob_len);
 	kzfree(ascii_buf);
 
 	return asciiblob_len;
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 9b898c969558..89a14e71eb0a 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -846,14 +846,40 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
 can_read_key:
 	ret = -EOPNOTSUPP;
 	if (key->type->read) {
-		/* Read the data with the semaphore held (since we might sleep)
+		/*
+		 * Read the data with the semaphore held (since we might sleep)
 		 * to protect against the key being updated or revoked.
+		 *
+		 * Allocating a temporary buffer to hold the keys before
+		 * transferring them to user buffer to avoid potential
+		 * deadlock involving page fault and mmap_sem.
 		 */
+		char *tmpbuf = NULL;
+
+		if (buffer && buflen) {
+			tmpbuf = kmalloc(buflen, GFP_KERNEL);
+			if (!tmpbuf) {
+				ret = -ENOMEM;
+				goto error2;
+			}
+		}
 		down_read(&key->sem);
 		ret = key_validate(key);
 		if (ret == 0)
-			ret = key->type->read(key, buffer, buflen);
+			ret = key->type->read(key, tmpbuf, buflen);
 		up_read(&key->sem);
+
+		/*
+		 * Read methods will just return the required length
+		 * without any copying if the provided length isn't big
+		 * enough.
+		 */
+		if ((ret > 0) && (ret <= buflen) && buffer &&
+		    copy_to_user(buffer, tmpbuf, ret))
+			ret = -EFAULT;
+
+		if (tmpbuf)
+			kzfree(tmpbuf);
 	}
 
 error2:
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index febf36c6ddc5..5ca620d31cd3 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -459,7 +459,6 @@ static int keyring_read_iterator(const void *object, void *data)
 {
 	struct keyring_read_iterator_context *ctx = data;
 	const struct key *key = keyring_ptr_to_key(object);
-	int ret;
 
 	kenter("{%s,%d},,{%zu/%zu}",
 	       key->type->name, key->serial, ctx->count, ctx->buflen);
@@ -467,10 +466,7 @@ static int keyring_read_iterator(const void *object, void *data)
 	if (ctx->count >= ctx->buflen)
 		return 1;
 
-	ret = put_user(key->serial, ctx->buffer);
-	if (ret < 0)
-		return ret;
-	ctx->buffer++;
+	*ctx->buffer++ = key->serial;
 	ctx->count += sizeof(key->serial);
 	return 0;
 }
diff --git a/security/keys/request_key_auth.c b/security/keys/request_key_auth.c
index ecba39c93fd9..41e9735006d0 100644
--- a/security/keys/request_key_auth.c
+++ b/security/keys/request_key_auth.c
@@ -22,7 +22,7 @@ static int request_key_auth_instantiate(struct key *,
 static void request_key_auth_describe(const struct key *, struct seq_file *);
 static void request_key_auth_revoke(struct key *);
 static void request_key_auth_destroy(struct key *);
-static long request_key_auth_read(const struct key *, char __user *, size_t);
+static long request_key_auth_read(const struct key *, char *, size_t);
 
 /*
  * The request-key authorisation key type definition.
@@ -80,7 +80,7 @@ static void request_key_auth_describe(const struct key *key,
  * - the key's semaphore is read-locked
  */
 static long request_key_auth_read(const struct key *key,
-				  char __user *buffer, size_t buflen)
+				  char *buffer, size_t buflen)
 {
 	struct request_key_auth *rka = dereference_key_locked(key);
 	size_t datalen;
@@ -97,8 +97,7 @@ static long request_key_auth_read(const struct key *key,
 		if (buflen > datalen)
 			buflen = datalen;
 
-		if (copy_to_user(buffer, rka->callout_info, buflen) != 0)
-			ret = -EFAULT;
+		memcpy(buffer, rka->callout_info, buflen);
 	}
 
 	return ret;
diff --git a/security/keys/trusted-keys/trusted_tpm1.c b/security/keys/trusted-keys/trusted_tpm1.c
index d2c5ec1e040b..8001ab07e63b 100644
--- a/security/keys/trusted-keys/trusted_tpm1.c
+++ b/security/keys/trusted-keys/trusted_tpm1.c
@@ -1130,11 +1130,10 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
  * trusted_read - copy the sealed blob data to userspace in hex.
  * On success, return to userspace the trusted key datablob size.
  */
-static long trusted_read(const struct key *key, char __user *buffer,
+static long trusted_read(const struct key *key, char *buffer,
 			 size_t buflen)
 {
 	const struct trusted_key_payload *p;
-	char *ascii_buf;
 	char *bufp;
 	int i;
 
@@ -1143,18 +1142,9 @@ static long trusted_read(const struct key *key, char __user *buffer,
 		return -EINVAL;
 
 	if (buffer && buflen >= 2 * p->blob_len) {
-		ascii_buf = kmalloc_array(2, p->blob_len, GFP_KERNEL);
-		if (!ascii_buf)
-			return -ENOMEM;
-
-		bufp = ascii_buf;
+		bufp = buffer;
 		for (i = 0; i < p->blob_len; i++)
 			bufp = hex_byte_pack(bufp, p->blob[i]);
-		if (copy_to_user(buffer, ascii_buf, 2 * p->blob_len) != 0) {
-			kzfree(ascii_buf);
-			return -EFAULT;
-		}
-		kzfree(ascii_buf);
 	}
 	return 2 * p->blob_len;
 }
diff --git a/security/keys/user_defined.c b/security/keys/user_defined.c
index 6f12de4ce549..07d4287e9084 100644
--- a/security/keys/user_defined.c
+++ b/security/keys/user_defined.c
@@ -168,7 +168,7 @@ EXPORT_SYMBOL_GPL(user_describe);
  * read the key data
  * - the key's semaphore is read-locked
  */
-long user_read(const struct key *key, char __user *buffer, size_t buflen)
+long user_read(const struct key *key, char *buffer, size_t buflen)
 {
 	const struct user_key_payload *upayload;
 	long ret;
@@ -181,8 +181,7 @@ long user_read(const struct key *key, char __user *buffer, size_t buflen)
 		if (buflen > upayload->datalen)
 			buflen = upayload->datalen;
 
-		if (copy_to_user(buffer, upayload->data, buflen) != 0)
-			ret = -EFAULT;
+		memcpy(buffer, upayload->data, buflen);
 	}
 
 	return ret;
-- 
2.18.1

[PATCH v2 2/2] KEYS: Avoid false positive ENOMEM error on key read

[Waiman Long]

By allocating a kernel buffer with an user-supplied buffer length, it
is possible that a false positive ENOMEM error may be returned because
the user-supplied length is just too large even if the system do have
enough memory to hold the actual key data.

To reduce this possibility, we set a threshold (1024) over which we
do check the actual key length first before allocating a buffer of the
right size to hold it.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 security/keys/keyctl.c | 46 ++++++++++++++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 11 deletions(-)

diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 89a14e71eb0a..662a638a680d 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -855,28 +855,52 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
 		 * deadlock involving page fault and mmap_sem.
 		 */
 		char *tmpbuf = NULL;
+		size_t tbuflen = buflen;
 
-		if (buffer && buflen) {
-			tmpbuf = kmalloc(buflen, GFP_KERNEL);
+		/*
+		 * We don't want an erronous -ENOMEM error due to an
+		 * arbitrary large user-supplied buflen. So if buflen
+		 * exceeds a threshold (1024 bytes in this case), we call
+		 * the read method twice. The first time to get the buffer
+		 * length and the second time to read out the key data.
+		 *
+		 * N.B. All the read methods will return the required
+		 *      buffer length with a NULL input buffer or when
+		 *      the input buffer length isn't large enough.
+		 */
+		if (buflen && buffer && (buflen <= 0x400)) {
+allocbuf:
+			tmpbuf = kmalloc(tbuflen, GFP_KERNEL);
 			if (!tmpbuf) {
 				ret = -ENOMEM;
 				goto error2;
 			}
 		}
+
 		down_read(&key->sem);
 		ret = key_validate(key);
 		if (ret == 0)
-			ret = key->type->read(key, tmpbuf, buflen);
+			ret = key->type->read(key, tmpbuf, tbuflen);
 		up_read(&key->sem);
 
-		/*
-		 * Read methods will just return the required length
-		 * without any copying if the provided length isn't big
-		 * enough.
-		 */
-		if ((ret > 0) && (ret <= buflen) && buffer &&
-		    copy_to_user(buffer, tmpbuf, ret))
-			ret = -EFAULT;
+		if ((ret > 0) && (ret <= buflen) && buffer) {
+			/*
+			 * It is possible, though unlikely, that the key
+			 * changes in between the up_read->down_read period.
+			 * If the key becomes longer, we will have to
+			 * allocate a larger buffer and redo the key read
+			 * again.
+			 */
+			if (!tmpbuf || unlikely(ret > tbuflen)) {
+				tbuflen = ret;
+				if (unlikely(tmpbuf))
+					kzfree(tmpbuf);
+				goto allocbuf;
+			}
+
+			if (copy_to_user(buffer, tmpbuf, ret))
+				ret = -EFAULT;
+		}
 
 		if (tmpbuf)
 			kzfree(tmpbuf);
-- 
2.18.1

Re: [PATCH v2 2/2] KEYS: Avoid false positive ENOMEM error on key read

[David Howells]

Waiman Long <longman@redhat.com> wrote:

> +			tmpbuf = kmalloc(tbuflen, GFP_KERNEL);

This would probably be better off using kvmalloc() - otherwise big objects
have to be constructed from runs of contiguous pages.  But since all we're
doing is buffering for userspace, we don't care about that.

If you agree, we can address it with an additional patch.

David

Re: [PATCH v2 2/2] KEYS: Avoid false positive ENOMEM error on key read

[Waiman Long]

On 3/9/20 12:32 PM, David Howells wrote:
> Waiman Long <longman@redhat.com> wrote:
>
>> +			tmpbuf = kmalloc(tbuflen, GFP_KERNEL);
> This would probably be better off using kvmalloc() - otherwise big objects
> have to be constructed from runs of contiguous pages.  But since all we're
> doing is buffering for userspace, we don't care about that.
>
> If you agree, we can address it with an additional patch.
>
> David

That is certainly fine with me. I don't care if the pages are contiguous
or not. Will add a patch 3 for that as suggested.

Thanks,
Longman

Re: [PATCH v2 2/2] KEYS: Avoid false positive ENOMEM error on key read

[Waiman Long]

On 3/10/20 11:45 AM, Waiman Long wrote:
> On 3/9/20 12:32 PM, David Howells wrote:
>> Waiman Long <longman@redhat.com> wrote:
>>
>>> +			tmpbuf = kmalloc(tbuflen, GFP_KERNEL);
>> This would probably be better off using kvmalloc() - otherwise big objects
>> have to be constructed from runs of contiguous pages.  But since all we're
>> doing is buffering for userspace, we don't care about that.
>>
>> If you agree, we can address it with an additional patch.
>>
>> David
> That is certainly fine with me. I don't care if the pages are contiguous
> or not. Will add a patch 3 for that as suggested.

That is not as simple as I thought. First of that, there is not an
equivalent kzvfree() helper to clear the buffer first before clearing.
Of course, I can do that manually.

With patch 2, the allocated buffer length will be max(1024, keylen). The
security code uses kmalloc() for allocation. If we use kvalloc() here,
perhaps we should also use that for allocation that can be potentially
large like that in big_key. What do you think?

Cheers,
Longman

Re: [PATCH v2 2/2] KEYS: Avoid false positive ENOMEM error on key read

[David Howells]

Waiman Long <longman@redhat.com> wrote:

> That is not as simple as I thought. First of that, there is not an
> equivalent kzvfree() helper to clear the buffer first before clearing.
> Of course, I can do that manually.

Yeah, the actual substance of vfree() may get deferred.  It may be worth
adding a kvzfree() that switches between kzfree() and memset(),vfree().

> With patch 2, the allocated buffer length will be max(1024, keylen). The
> security code uses kmalloc() for allocation. If we use kvalloc() here,
> perhaps we should also use that for allocation that can be potentially
> large like that in big_key. What do you think?

Not for big_key: if it's larger than BIG_KEY_FILE_THRESHOLD (~1KiB) it gets
written encrypted into shmem so that it can be swapped out to disk when not in
use.

However, other cases, sure - just be aware that on a 32-bit system,
vmalloc/vmap space is a strictly limited resource.

David

Re: [PATCH v2 2/2] KEYS: Avoid false positive ENOMEM error on key read

[Waiman Long]

[-- Attachment #1: Type: text/plain, Size: 1300 bytes --]

On 3/10/20 1:12 PM, David Howells wrote:
> Waiman Long <longman@redhat.com> wrote:
>
>> That is not as simple as I thought. First of that, there is not an
>> equivalent kzvfree() helper to clear the buffer first before clearing.
>> Of course, I can do that manually.
> Yeah, the actual substance of vfree() may get deferred.  It may be worth
> adding a kvzfree() that switches between kzfree() and memset(),vfree().
>
>> With patch 2, the allocated buffer length will be max(1024, keylen). The
>> security code uses kmalloc() for allocation. If we use kvalloc() here,
>> perhaps we should also use that for allocation that can be potentially
>> large like that in big_key. What do you think?
> Not for big_key: if it's larger than BIG_KEY_FILE_THRESHOLD (~1KiB) it gets
> written encrypted into shmem so that it can be swapped out to disk when not in
> use.
>
> However, other cases, sure - just be aware that on a 32-bit system,
> vmalloc/vmap space is a strictly limited resource.

Attached is an additional patch to make the transition from kmalloc() to
kvmalloc(). I put the __kvzfree() helper in internal.h for now. I plan
to send a patch later to add a kvzfree() API once there is a use case in
the kernel.

I am not going to touch other places for now to make thing simpler.

Cheers,
Longman


[-- Attachment #2: v2-0003-KEYS-Use-kvmalloc-to-better-handle-large-buffer-a.patch --]
[-- Type: text/x-patch, Size: 2954 bytes --]

From e2e73e2bc0c5cd168de273b0fe9df1e5c48cd232 Mon Sep 17 00:00:00 2001
From: Waiman Long <longman@redhat.com>
Date: Wed, 11 Mar 2020 11:01:59 -0400
Subject: [PATCH v2 3/3] KEYS: Use kvmalloc() to better handle large buffer
 allocation

For large multi-page temporary buffer allocation, the security/keys
subsystem don't need contiguous physical pages. It will work perfectly
fine with virtually mapped pages.

Replace the kmalloc() call by kvmalloc() and provide a __kvzfree()
helper function to clear and free the kvmalloc'ed buffer. This will
reduce the chance of memory allocation failure just because of highly
fragmented pages.

Suggested-by: David Howells <dhowells@redhat.com>
Signed-off-by: Waiman Long <longman@redhat.com>
---
 security/keys/internal.h | 14 ++++++++++++++
 security/keys/keyctl.c   | 12 ++++++------
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/security/keys/internal.h b/security/keys/internal.h
index ba3e2da14cef..1b6e2d66e378 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -16,6 +16,8 @@
 #include <linux/keyctl.h>
 #include <linux/refcount.h>
 #include <linux/compat.h>
+#include <linux/mm.h>
+#include <linux/vmalloc.h>
 
 struct iovec;
 
@@ -349,4 +351,16 @@ static inline void key_check(const struct key *key)
 
 #endif
 
+/*
+ * Helper function to clear and free a kvmalloc'ed memory object.
+ */
+static inline void __kvzfree(const void *addr, size_t len)
+{
+	if (is_vmalloc_addr(addr)) {
+		memset((char *)addr, 0, len);
+		vfree(addr);
+	} else {
+		kzfree(addr);
+	}
+}
 #endif /* _INTERNAL_H */
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 662a638a680d..ca05604bc9c0 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -339,7 +339,7 @@ long keyctl_update_key(key_serial_t id,
 	payload = NULL;
 	if (plen) {
 		ret = -ENOMEM;
-		payload = kmalloc(plen, GFP_KERNEL);
+		payload = kvmalloc(plen, GFP_KERNEL);
 		if (!payload)
 			goto error;
 
@@ -360,7 +360,7 @@ long keyctl_update_key(key_serial_t id,
 
 	key_ref_put(key_ref);
 error2:
-	kzfree(payload);
+	__kvzfree(payload, plen);
 error:
 	return ret;
 }
@@ -870,7 +870,7 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
 		 */
 		if (buflen && buffer && (buflen <= 0x400)) {
 allocbuf:
-			tmpbuf = kmalloc(tbuflen, GFP_KERNEL);
+			tmpbuf = kvmalloc(tbuflen, GFP_KERNEL);
 			if (!tmpbuf) {
 				ret = -ENOMEM;
 				goto error2;
@@ -892,9 +892,9 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
 			 * again.
 			 */
 			if (!tmpbuf || unlikely(ret > tbuflen)) {
-				tbuflen = ret;
 				if (unlikely(tmpbuf))
-					kzfree(tmpbuf);
+					__kvzfree(tmpbuf, tbuflen);
+				tbuflen = ret;
 				goto allocbuf;
 			}
 
@@ -903,7 +903,7 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
 		}
 
 		if (tmpbuf)
-			kzfree(tmpbuf);
+			__kvzfree(tmpbuf, tbuflen);
 	}
 
 error2:
-- 
2.18.1

Re: [PATCH v2 1/2] KEYS: Don't write out to userspace while holding key semaphore

[Jarkko Sakkinen]

On Sun, Mar 08, 2020 at 01:04:09PM -0400, Waiman Long wrote:
> +		/*
> +		 * Read methods will just return the required length
> +		 * without any copying if the provided length isn't big
> +		 * enough.
> +		 */
> +		if ((ret > 0) && (ret <= buflen) && buffer &&
> +		    copy_to_user(buffer, tmpbuf, ret))
> +			ret = -EFAULT;

Please, reorg and remove redundant parentheses:

/*
 * Read methods will just return the required length
 * without any copying if the provided length isn't big
 * enough.
 */
if (ret > 0 && ret <= buflen) {
	if (buffer && copy_to_user(buffer, tmpbuf, ret))
		ret = -EFAULT;
}

Now the comment is attached to the exact right thing. The previous
organization is a pain to look at when backtracking commits for
whatever reason in the future.

I'm also wondering, would it be possible to rework the code in a way
that you don't have check whether buffer is valid on a constant basis?

/Jarkko

Re: [PATCH v2 1/2] KEYS: Don't write out to userspace while holding key semaphore

[Waiman Long]

On 3/12/20 9:04 PM, Jarkko Sakkinen wrote:
> On Sun, Mar 08, 2020 at 01:04:09PM -0400, Waiman Long wrote:
>> +		/*
>> +		 * Read methods will just return the required length
>> +		 * without any copying if the provided length isn't big
>> +		 * enough.
>> +		 */
>> +		if ((ret > 0) && (ret <= buflen) && buffer &&
>> +		    copy_to_user(buffer, tmpbuf, ret))
>> +			ret = -EFAULT;
> Please, reorg and remove redundant parentheses:
>
> /*
>  * Read methods will just return the required length
>  * without any copying if the provided length isn't big
>  * enough.
>  */
> if (ret > 0 && ret <= buflen) {
> 	if (buffer && copy_to_user(buffer, tmpbuf, ret))
> 		ret = -EFAULT;
> }
>
> Now the comment is attached to the exact right thing. The previous
> organization is a pain to look at when backtracking commits for
> whatever reason in the future.
Yes, I can reorganize the code.
> I'm also wondering, would it be possible to rework the code in a way
> that you don't have check whether buffer is valid on a constant basis?

One way to do that is to extract the down_read/up_read block into a
helper function and then have 2 separate paths - one for length
retrieval and another one for reading the key. I think that will make
the code a bit easier easier to read.

Thanks,
Longman

Re: [PATCH v2 1/2] KEYS: Don't write out to userspace while holding key semaphore

[Jarkko Sakkinen]

On Fri, Mar 13, 2020 at 09:29:47AM -0400, Waiman Long wrote:
> One way to do that is to extract the down_read/up_read block into a
> helper function and then have 2 separate paths - one for length
> retrieval and another one for reading the key. I think that will make
> the code a bit easier easier to read.
> 
> Thanks,
> Longman

If it is not too much trouble for you, I think this would be a legit
cleanup to do.

/Jarkko

Re: [PATCH v2 1/2] KEYS: Don't write out to userspace while holding key semaphore

[Waiman Long]

On 3/13/20 11:28 AM, Jarkko Sakkinen wrote:
> On Fri, Mar 13, 2020 at 09:29:47AM -0400, Waiman Long wrote:
>> One way to do that is to extract the down_read/up_read block into a
>> helper function and then have 2 separate paths - one for length
>> retrieval and another one for reading the key. I think that will make
>> the code a bit easier easier to read.
>>
>> Thanks,
>> Longman
> If it is not too much trouble for you, I think this would be a legit
> cleanup to do.

Done. Please review the v3 patch.

Thanks,
Longman