diff mbox series

[v7,2/2] KEYS: Avoid false positive ENOMEM error on key read

Message ID 20200321184932.16579-3-longman@redhat.com (mailing list archive)
State New, archived
Headers show
Series [v7,1/2] KEYS: Don't write out to userspace while holding key semaphore | expand

Commit Message

Waiman Long March 21, 2020, 6:49 p.m. UTC
By allocating a kernel buffer with a 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.

Moreover, if the buffer length is larger than the maximum amount of
memory that can be returned by kmalloc() (2^(MAX_ORDER-1) number of
pages), a warning message will also be printed.

To reduce this possibility, we set a threshold (PAGE_SIZE) over which we
do check the actual key length first before allocating a buffer of the
right size to hold it. The threshold is arbitrary, it is just used to
trigger a buffer length check. It does not limit the actual key length
as long as there is enough memory to satisfy the memory request.

To further avoid large buffer allocation failure due to page
fragmentation, kvmalloc() is used to allocate the buffer so that vmapped
pages can be used when there is not a large enough contiguous set of
pages available for allocation.

In the extremely unlikely scenario that the key keeps on being changed
and made longer (still <= buflen) in between 2 __keyctl_read_key()
calls, the __keyctl_read_key() calling loop in keyctl_read_key() may
have to be iterated a large number of times, but definitely not infinite.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 security/keys/internal.h | 12 ++++++++
 security/keys/keyctl.c   | 59 +++++++++++++++++++++++++++++-----------
 2 files changed, 55 insertions(+), 16 deletions(-)

Comments

Tetsuo Handa March 22, 2020, 12:31 a.m. UTC | #1
On 2020/03/22 3:49, Waiman Long wrote:
> +	do {

> +		if (ret > key_data_len) {
> +			if (unlikely(key_data))
> +				__kvzfree(key_data, key_data_len);
> +			key_data_len = ret;
> +			continue;	/* Allocate buffer */

Excuse me, but "continue;" inside "do { ... } while (0);" means "break;"
because "while (0)" is evaluated before continuing the loop.

----------
#include <stdio.h>

int main(int argc, char *argv[])
{
        do {
                printf("step 1\n");
                if (1) {
                        printf("step 2\n");
                        continue;
                }
                printf("step 3\n");
        } while (0);
        printf("step 4\n");
        return 0;
}
----------

----------
step 1
step 2
step 4
----------

> +		}

> +	} while (0);
Waiman Long March 22, 2020, 12:46 a.m. UTC | #2
On 3/21/20 8:31 PM, Tetsuo Handa wrote:
> On 2020/03/22 3:49, Waiman Long wrote:
>> +	do {
>> +		if (ret > key_data_len) {
>> +			if (unlikely(key_data))
>> +				__kvzfree(key_data, key_data_len);
>> +			key_data_len = ret;
>> +			continue;	/* Allocate buffer */
> Excuse me, but "continue;" inside "do { ... } while (0);" means "break;"
> because "while (0)" is evaluated before continuing the loop.

You are right. My mistake. Will send out a new one for patch 2.

-Longman
diff mbox series

Patch

diff --git a/security/keys/internal.h b/security/keys/internal.h
index ba3e2da14cef..6d0ca48ae9a5 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,14 @@  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 (addr) {
+		memset((void *)addr, 0, len);
+		kvfree(addr);
+	}
+}
 #endif /* _INTERNAL_H */
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 434ed9defd3a..2f72bbe2962b 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;
 }
@@ -827,7 +827,8 @@  long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
 	struct key *key;
 	key_ref_t key_ref;
 	long ret;
-	char *key_data;
+	char *key_data = NULL;
+	size_t key_data_len;
 
 	/* find the key first */
 	key_ref = lookup_user_key(keyid, 0, 0);
@@ -878,24 +879,50 @@  long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
 	 * Allocating a temporary buffer to hold the keys before
 	 * transferring them to user buffer to avoid potential
 	 * deadlock involving page fault and mmap_sem.
+	 *
+	 * key_data_len = (buflen <= PAGE_SIZE)
+	 *		? buflen : actual length of key data
+	 *
+	 * This prevents allocating arbitrary large buffer which can
+	 * be much larger than the actual key length. In the latter case,
+	 * at least 2 passes of this loop is required.
 	 */
-	key_data = kmalloc(buflen, GFP_KERNEL);
+	key_data_len = (buflen <= PAGE_SIZE) ? buflen : 0;
+	do {
+		if (key_data_len) {
+			key_data = kvmalloc(key_data_len, GFP_KERNEL);
+			if (!key_data) {
+				ret = -ENOMEM;
+				goto key_put_out;
+			}
+		}
 
-	if (!key_data) {
-		ret = -ENOMEM;
-		goto key_put_out;
-	}
-	ret = __keyctl_read_key(key, key_data, buflen);
+		ret = __keyctl_read_key(key, key_data, key_data_len);
+
+		/*
+		 * Read methods will just return the required length without
+		 * any copying if the provided length isn't large enough.
+		 */
+		if (ret <= 0 || ret > buflen)
+			break;
+
+		/*
+		 * The key may change (unlikely) in between 2 consecutive
+		 * __keyctl_read_key() calls. In this case, we reallocate
+		 * a larger buffer and redo the key read when
+		 * key_data_len < ret <= buflen.
+		 */
+		if (ret > key_data_len) {
+			if (unlikely(key_data))
+				__kvzfree(key_data, key_data_len);
+			key_data_len = ret;
+			continue;	/* Allocate buffer */
+		}
 
-	/*
-	 * Read methods will just return the required length without
-	 * any copying if the provided length isn't large enough.
-	 */
-	if (ret > 0 && ret <= buflen) {
 		if (copy_to_user(buffer, key_data, ret))
 			ret = -EFAULT;
-	}
-	kzfree(key_data);
+	} while (0);
+	__kvzfree(key_data, key_data_len);
 
 key_put_out:
 	key_put(key);