Linux-Security-Module Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v6 0/2] KEYS: Read keys to internal buffer & then copy to userspace
@ 2020-03-20 19:19 Waiman Long
  2020-03-20 19:19 ` [PATCH v6 1/2] KEYS: Don't write out to userspace while holding key semaphore Waiman Long
  2020-03-20 19:19 ` [PATCH v6 2/2] KEYS: Avoid false positive ENOMEM error on key read Waiman Long
  0 siblings, 2 replies; 6+ messages in thread
From: Waiman Long @ 2020-03-20 19:19 UTC (permalink / raw)
  To: David Howells, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
	Mimi Zohar, David S. Miller, Jakub Kicinski
  Cc: keyrings, linux-kernel, linux-security-module, linux-integrity,
	netdev, linux-afs, Sumit Garg, Jerry Snitselaar, Roberto Sassu,
	Eric Biggers, Chris von Recklinghausen, Waiman Long

v6:
 - Make some variable name changes and revise comments as suggested by
   Jarkko. No functional change from v5.

v5:
 - Merge v4 patches 2 and 3 into 1 to avoid sparse warning. Merge some of 
   commit logs into patch 1 as well. There is no further change.

v4:
 - Remove the __user annotation from big_key_read() and user_read() in
   patch 1.
 - Add a new patch 2 to remove __user annotation from rxrpc_read().
 - Add a new patch 3 to remove __user annotation from dns_resolver_read().
 - Merge the original patches 2 and 3 into a single patch 4 and refactor
   it as suggested by Jarkko and Eric.

v3:
 - Reorganize the keyctl_read_key() code to make it more readable as
   suggested by Jarkko Sakkinen.
 - Add patch 3 to use kvmalloc() for safer large buffer allocation as
   suggested by David Howells.

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/keys/big_key-type.h               |  2 +-
 include/keys/user-type.h                  |  3 +-
 include/linux/key-type.h                  |  2 +-
 net/dns_resolver/dns_key.c                |  2 +-
 net/rxrpc/key.c                           | 27 +++-----
 security/keys/big_key.c                   | 11 ++-
 security/keys/encrypted-keys/encrypted.c  |  7 +-
 security/keys/internal.h                  | 12 ++++
 security/keys/keyctl.c                    | 84 ++++++++++++++++++++---
 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 +-
 13 files changed, 114 insertions(+), 68 deletions(-)

-- 
2.18.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v6 1/2] KEYS: Don't write out to userspace while holding key semaphore
  2020-03-20 19:19 [PATCH v6 0/2] KEYS: Read keys to internal buffer & then copy to userspace Waiman Long
@ 2020-03-20 19:19 ` Waiman Long
  2020-03-20 19:19 ` [PATCH v6 2/2] KEYS: Avoid false positive ENOMEM error on key read Waiman Long
  1 sibling, 0 replies; 6+ messages in thread
From: Waiman Long @ 2020-03-20 19:19 UTC (permalink / raw)
  To: David Howells, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
	Mimi Zohar, David S. Miller, Jakub Kicinski
  Cc: keyrings, linux-kernel, linux-security-module, linux-integrity,
	netdev, linux-afs, Sumit Garg, Jerry Snitselaar, Roberto Sassu,
	Eric Biggers, Chris von Recklinghausen, 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 all the relevant
read methods as well as additional changes to not use any userspace
write helpers. That is,

  1) The put_user() call is replaced by a direct copy.
  2) The copy_to_user() call is replaced by memcpy().
  3) All the fault handling code is removed.

Compiling on a x86-64 system, the size of the rxrpc_read() function is
reduced from 3795 bytes to 2384 bytes with this patch.

Fixes: ^1da177e4c3f4 ("Linux-2.6.12-rc2")
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: Waiman Long <longman@redhat.com>
---
 include/keys/big_key-type.h               |  2 +-
 include/keys/user-type.h                  |  3 +-
 include/linux/key-type.h                  |  2 +-
 net/dns_resolver/dns_key.c                |  2 +-
 net/rxrpc/key.c                           | 27 ++++-------
 security/keys/big_key.c                   | 11 ++---
 security/keys/encrypted-keys/encrypted.c  |  7 ++-
 security/keys/keyctl.c                    | 57 +++++++++++++++++++----
 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 +-
 12 files changed, 77 insertions(+), 66 deletions(-)

diff --git a/include/keys/big_key-type.h b/include/keys/big_key-type.h
index f6a7ba4dccd4..3fee04f81439 100644
--- a/include/keys/big_key-type.h
+++ b/include/keys/big_key-type.h
@@ -17,6 +17,6 @@ extern void big_key_free_preparse(struct key_preparsed_payload *prep);
 extern void big_key_revoke(struct key *key);
 extern void big_key_destroy(struct key *key);
 extern void big_key_describe(const struct key *big_key, struct seq_file *m);
-extern long big_key_read(const struct key *key, char __user *buffer, size_t buflen);
+extern long big_key_read(const struct key *key, char *buffer, size_t buflen);
 
 #endif /* _KEYS_BIG_KEY_TYPE_H */
diff --git a/include/keys/user-type.h b/include/keys/user-type.h
index d5e73266a81a..be61fcddc02a 100644
--- a/include/keys/user-type.h
+++ b/include/keys/user-type.h
@@ -41,8 +41,7 @@ extern int user_update(struct key *key, struct key_preparsed_payload *prep);
 extern void user_revoke(struct key *key);
 extern void user_destroy(struct key *key);
 extern void user_describe(const struct key *user, struct seq_file *m);
-extern long user_read(const struct key *key,
-		      char __user *buffer, size_t buflen);
+extern long user_read(const struct key *key, char *buffer, size_t buflen);
 
 static inline const struct user_key_payload *user_key_payload_rcu(const struct key *key)
 {
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/net/dns_resolver/dns_key.c b/net/dns_resolver/dns_key.c
index 3e1a90669006..ad53eb31d40f 100644
--- a/net/dns_resolver/dns_key.c
+++ b/net/dns_resolver/dns_key.c
@@ -302,7 +302,7 @@ static void dns_resolver_describe(const struct key *key, struct seq_file *m)
  * - the key's semaphore is read-locked
  */
 static long dns_resolver_read(const struct key *key,
-			      char __user *buffer, size_t buflen)
+			      char *buffer, size_t buflen)
 {
 	int err = PTR_ERR(key->payload.data[dns_key_error]);
 
diff --git a/net/rxrpc/key.c b/net/rxrpc/key.c
index 6c3f35fac42d..0c98313dd7a8 100644
--- a/net/rxrpc/key.c
+++ b/net/rxrpc/key.c
@@ -31,7 +31,7 @@ static void rxrpc_free_preparse_s(struct key_preparsed_payload *);
 static void rxrpc_destroy(struct key *);
 static void rxrpc_destroy_s(struct key *);
 static void rxrpc_describe(const struct key *, struct seq_file *);
-static long rxrpc_read(const struct key *, char __user *, size_t);
+static long rxrpc_read(const struct key *, char *, size_t);
 
 /*
  * rxrpc defined keys take an arbitrary string as the description and an
@@ -1042,12 +1042,12 @@ EXPORT_SYMBOL(rxrpc_get_null_key);
  * - this returns the result in XDR form
  */
 static long rxrpc_read(const struct key *key,
-		       char __user *buffer, size_t buflen)
+		       char *buffer, size_t buflen)
 {
 	const struct rxrpc_key_token *token;
 	const struct krb5_principal *princ;
 	size_t size;
-	__be32 __user *xdr, *oldxdr;
+	__be32 *xdr, *oldxdr;
 	u32 cnlen, toksize, ntoks, tok, zero;
 	u16 toksizes[AFSTOKEN_MAX];
 	int loop;
@@ -1124,30 +1124,25 @@ static long rxrpc_read(const struct key *key,
 	if (!buffer || buflen < size)
 		return size;
 
-	xdr = (__be32 __user *) buffer;
+	xdr = (__be32 *)buffer;
 	zero = 0;
 #define ENCODE(x)				\
 	do {					\
-		__be32 y = htonl(x);		\
-		if (put_user(y, xdr++) < 0)	\
-			goto fault;		\
+		*xdr++ = htonl(x);		\
 	} while(0)
 #define ENCODE_DATA(l, s)						\
 	do {								\
 		u32 _l = (l);						\
 		ENCODE(l);						\
-		if (copy_to_user(xdr, (s), _l) != 0)			\
-			goto fault;					\
-		if (_l & 3 &&						\
-		    copy_to_user((u8 __user *)xdr + _l, &zero, 4 - (_l & 3)) != 0) \
-			goto fault;					\
+		memcpy(xdr, (s), _l);					\
+		if (_l & 3)						\
+			memcpy((u8 *)xdr + _l, &zero, 4 - (_l & 3));	\
 		xdr += (_l + 3) >> 2;					\
 	} while(0)
 #define ENCODE64(x)					\
 	do {						\
 		__be64 y = cpu_to_be64(x);		\
-		if (copy_to_user(xdr, &y, 8) != 0)	\
-			goto fault;			\
+		memcpy(xdr, &y, 8);			\
 		xdr += 8 >> 2;				\
 	} while(0)
 #define ENCODE_STR(s)				\
@@ -1238,8 +1233,4 @@ static long rxrpc_read(const struct key *key,
 	ASSERTCMP((char __user *) xdr - buffer, ==, size);
 	_leave(" = %zu", size);
 	return size;
-
-fault:
-	_leave(" = -EFAULT");
-	return -EFAULT;
 }
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..5a0794cb8815 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -797,6 +797,21 @@ long keyctl_keyring_search(key_serial_t ringid,
 	return ret;
 }
 
+/*
+ * Call the read method
+ */
+static long __keyctl_read_key(struct key *key, char *buffer, size_t buflen)
+{
+	long ret;
+
+	down_read(&key->sem);
+	ret = key_validate(key);
+	if (ret == 0)
+		ret = key->type->read(key, buffer, buflen);
+	up_read(&key->sem);
+	return ret;
+}
+
 /*
  * Read a key's payload.
  *
@@ -844,16 +859,42 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
 
 	/* the key is probably readable - now try to read it */
 can_read_key:
-	ret = -EOPNOTSUPP;
-	if (key->type->read) {
-		/* Read the data with the semaphore held (since we might sleep)
+	if (!key->type->read) {
+		ret = -EOPNOTSUPP;
+		goto error2;
+	}
+
+	if (!buffer || !buflen) {
+		/* Get the key length from the read method */
+		ret = __keyctl_read_key(key, NULL, 0);
+	} else {
+
+		/*
+		 * 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 *key_data = kmalloc(buflen, GFP_KERNEL);
+
+		if (!key_data) {
+			ret = -ENOMEM;
+			goto error2;
+		}
+		ret = __keyctl_read_key(key, key_data, buflen);
+
+		/*
+		 * Read methods will just return the required length
+		 * without any copying if the provided length isn't big
+		 * enough.
 		 */
-		down_read(&key->sem);
-		ret = key_validate(key);
-		if (ret == 0)
-			ret = key->type->read(key, buffer, buflen);
-		up_read(&key->sem);
+		if (ret > 0 && ret <= buflen) {
+			if (copy_to_user(buffer, key_data, ret))
+				ret = -EFAULT;
+		}
+		kzfree(key_data);
 	}
 
 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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v6 2/2] KEYS: Avoid false positive ENOMEM error on key read
  2020-03-20 19:19 [PATCH v6 0/2] KEYS: Read keys to internal buffer & then copy to userspace Waiman Long
  2020-03-20 19:19 ` [PATCH v6 1/2] KEYS: Don't write out to userspace while holding key semaphore Waiman Long
@ 2020-03-20 19:19 ` Waiman Long
  2020-03-20 22:19   ` Jarkko Sakkinen
  1 sibling, 1 reply; 6+ messages in thread
From: Waiman Long @ 2020-03-20 19:19 UTC (permalink / raw)
  To: David Howells, Jarkko Sakkinen, James Morris, Serge E. Hallyn,
	Mimi Zohar, David S. Miller, Jakub Kicinski
  Cc: keyrings, linux-kernel, linux-security-module, linux-integrity,
	netdev, linux-afs, Sumit Garg, Jerry Snitselaar, Roberto Sassu,
	Eric Biggers, Chris von Recklinghausen, Waiman Long

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.

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

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 5a0794cb8815..ded69108db0d 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;
 }
@@ -877,13 +877,23 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
 		 * transferring them to user buffer to avoid potential
 		 * deadlock involving page fault and mmap_sem.
 		 */
-		char *key_data = kmalloc(buflen, GFP_KERNEL);
+		char *key_data = NULL;
+		size_t key_data_len = buflen;
 
-		if (!key_data) {
-			ret = -ENOMEM;
-			goto error2;
+		/*
+		 * When the user-supplied key length is larger than
+		 * PAGE_SIZE, we get the actual key length first before
+		 * allocating a right-sized key data buffer.
+		 */
+		if (buflen <= PAGE_SIZE) {
+allocbuf:
+			key_data = kvmalloc(key_data_len, GFP_KERNEL);
+			if (!key_data) {
+				ret = -ENOMEM;
+				goto error2;
+			}
 		}
-		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
@@ -891,10 +901,23 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
 		 * enough.
 		 */
 		if (ret > 0 && ret <= buflen) {
+			/*
+			 * The key may change (unlikely) in between 2
+			 * consecutive __keyctl_read_key() calls. We will
+			 * need to allocate a larger buffer and redo the key
+			 * read when key_data_len < ret <= buflen.
+			 */
+			if (!key_data || unlikely(ret > key_data_len)) {
+				if (unlikely(key_data))
+					__kvzfree(key_data, key_data_len);
+				key_data_len = ret;
+				goto allocbuf;
+			}
+
 			if (copy_to_user(buffer, key_data, ret))
 				ret = -EFAULT;
 		}
-		kzfree(key_data);
+		__kvzfree(key_data, key_data_len);
 	}
 
 error2:
-- 
2.18.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v6 2/2] KEYS: Avoid false positive ENOMEM error on key read
  2020-03-20 19:19 ` [PATCH v6 2/2] KEYS: Avoid false positive ENOMEM error on key read Waiman Long
@ 2020-03-20 22:19   ` Jarkko Sakkinen
  2020-03-20 22:21     ` Jarkko Sakkinen
  2020-03-21  1:51     ` Waiman Long
  0 siblings, 2 replies; 6+ messages in thread
From: Jarkko Sakkinen @ 2020-03-20 22:19 UTC (permalink / raw)
  To: Waiman Long
  Cc: David Howells, James Morris, Serge E. Hallyn, Mimi Zohar,
	David S. Miller, Jakub Kicinski, keyrings, linux-kernel,
	linux-security-module, linux-integrity, netdev, linux-afs,
	Sumit Garg, Jerry Snitselaar, Roberto Sassu, Eric Biggers,
	Chris von Recklinghausen

On Fri, Mar 20, 2020 at 03:19:03PM -0400, Waiman Long wrote:
> 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.
> 
> Signed-off-by: Waiman Long <longman@redhat.com>
> ---
>  security/keys/internal.h | 12 ++++++++++++
>  security/keys/keyctl.c   | 39 +++++++++++++++++++++++++++++++--------
>  2 files changed, 43 insertions(+), 8 deletions(-)
> 
> 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 5a0794cb8815..ded69108db0d 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;
>  }
> @@ -877,13 +877,23 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
>  		 * transferring them to user buffer to avoid potential
>  		 * deadlock involving page fault and mmap_sem.
>  		 */
> -		char *key_data = kmalloc(buflen, GFP_KERNEL);
> +		char *key_data = NULL;
> +		size_t key_data_len = buflen;
>  
> -		if (!key_data) {
> -			ret = -ENOMEM;
> -			goto error2;
> +		/*
> +		 * When the user-supplied key length is larger than
> +		 * PAGE_SIZE, we get the actual key length first before
> +		 * allocating a right-sized key data buffer.
> +		 */
> +		if (buflen <= PAGE_SIZE) {
> +allocbuf:

Would move this label before condition instead of jumping inside the
nested block since it will always evaluate correctly.

To this version haven't really gotten why you don't use a legit loop
construct but instead jump from one random nested location to another
random nested location? This construct will be somewhat nasty to
maintain. The construct is weird enough that you should have rather
good explanation in the long description why such a mess.


> +			key_data = kvmalloc(key_data_len, GFP_KERNEL);
> +			if (!key_data) {
> +				ret = -ENOMEM;
> +				goto error2;
> +			}
>  		}
> -		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
> @@ -891,10 +901,23 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
>  		 * enough.
>  		 */
>  		if (ret > 0 && ret <= buflen) {
> +			/*
> +			 * The key may change (unlikely) in between 2
> +			 * consecutive __keyctl_read_key() calls. We will
> +			 * need to allocate a larger buffer and redo the key
> +			 * read when key_data_len < ret <= buflen.
> +			 */
> +			if (!key_data || unlikely(ret > key_data_len)) {
> +				if (unlikely(key_data))
> +					__kvzfree(key_data, key_data_len);
> +				key_data_len = ret;
> +				goto allocbuf;
> +			}
> +
>  			if (copy_to_user(buffer, key_data, ret))
>  				ret = -EFAULT;
>  		}
> -		kzfree(key_data);
> +		__kvzfree(key_data, key_data_len);
>  	}
>  
>  error2:
> -- 
> 2.18.1
> 

Doesn't this go to infinite loop if actual key size is at least
PAGE_SIZE + 1? Where is the guarantee that this cannot happen?

/Jarkko



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v6 2/2] KEYS: Avoid false positive ENOMEM error on key read
  2020-03-20 22:19   ` Jarkko Sakkinen
@ 2020-03-20 22:21     ` Jarkko Sakkinen
  2020-03-21  1:51     ` Waiman Long
  1 sibling, 0 replies; 6+ messages in thread
From: Jarkko Sakkinen @ 2020-03-20 22:21 UTC (permalink / raw)
  To: Waiman Long
  Cc: David Howells, James Morris, Serge E. Hallyn, Mimi Zohar,
	David S. Miller, Jakub Kicinski, keyrings, linux-kernel,
	linux-security-module, linux-integrity, netdev, linux-afs,
	Sumit Garg, Jerry Snitselaar, Roberto Sassu, Eric Biggers,
	Chris von Recklinghausen

On Sat, Mar 21, 2020 at 12:19:27AM +0200, Jarkko Sakkinen wrote:
> Would move this label before condition instead of jumping inside the
> nested block since it will always evaluate correctly.
> 
> To this version haven't really gotten why you don't use a legit loop
> construct but instead jump from one random nested location to another
> random nested location? This construct will be somewhat nasty to
> maintain. The construct is weird enough that you should have rather
> good explanation in the long description why such a mess.

What I'm saying that if I fix a bug, the first version of the fix
would probably look something like this is right now. They I think
how to write it right. We don't want fixes that just happen to work.
Right now I'm worried to take this in since I'm not confident that
I haven't some possible corner case, or might still have gotten
something just plain wrong.

/Jarkko

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v6 2/2] KEYS: Avoid false positive ENOMEM error on key read
  2020-03-20 22:19   ` Jarkko Sakkinen
  2020-03-20 22:21     ` Jarkko Sakkinen
@ 2020-03-21  1:51     ` Waiman Long
  1 sibling, 0 replies; 6+ messages in thread
From: Waiman Long @ 2020-03-21  1:51 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: David Howells, James Morris, Serge E. Hallyn, Mimi Zohar,
	David S. Miller, Jakub Kicinski, keyrings, linux-kernel,
	linux-security-module, linux-integrity, netdev, linux-afs,
	Sumit Garg, Jerry Snitselaar, Roberto Sassu, Eric Biggers,
	Chris von Recklinghausen

On 3/20/20 6:19 PM, Jarkko Sakkinen wrote:
> On Fri, Mar 20, 2020 at 03:19:03PM -0400, Waiman Long wrote:
>> 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.
>>
>> Signed-off-by: Waiman Long <longman@redhat.com>
>> ---
>>  security/keys/internal.h | 12 ++++++++++++
>>  security/keys/keyctl.c   | 39 +++++++++++++++++++++++++++++++--------
>>  2 files changed, 43 insertions(+), 8 deletions(-)
>>
>> 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 5a0794cb8815..ded69108db0d 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;
>>  }
>> @@ -877,13 +877,23 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
>>  		 * transferring them to user buffer to avoid potential
>>  		 * deadlock involving page fault and mmap_sem.
>>  		 */
>> -		char *key_data = kmalloc(buflen, GFP_KERNEL);
>> +		char *key_data = NULL;
>> +		size_t key_data_len = buflen;
>>  
>> -		if (!key_data) {
>> -			ret = -ENOMEM;
>> -			goto error2;
>> +		/*
>> +		 * When the user-supplied key length is larger than
>> +		 * PAGE_SIZE, we get the actual key length first before
>> +		 * allocating a right-sized key data buffer.
>> +		 */
>> +		if (buflen <= PAGE_SIZE) {
>> +allocbuf:
> Would move this label before condition instead of jumping inside the
> nested block since it will always evaluate correctly.

Yes, you are right. That was not the case for initial version and I
didn't recheck it.


> To this version haven't really gotten why you don't use a legit loop
> construct but instead jump from one random nested location to another
> random nested location? This construct will be somewhat nasty to
> maintain. The construct is weird enough that you should have rather
> good explanation in the long description why such a mess.

I did that to avoid deep nesting. I can rewrite it to remove the the
goto statement.


>
>> +			key_data = kvmalloc(key_data_len, GFP_KERNEL);
>> +			if (!key_data) {
>> +				ret = -ENOMEM;
>> +				goto error2;
>> +			}
>>  		}
>> -		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
>> @@ -891,10 +901,23 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
>>  		 * enough.
>>  		 */
>>  		if (ret > 0 && ret <= buflen) {
>> +			/*
>> +			 * The key may change (unlikely) in between 2
>> +			 * consecutive __keyctl_read_key() calls. We will
>> +			 * need to allocate a larger buffer and redo the key
>> +			 * read when key_data_len < ret <= buflen.
>> +			 */
>> +			if (!key_data || unlikely(ret > key_data_len)) {
>> +				if (unlikely(key_data))
>> +					__kvzfree(key_data, key_data_len);
>> +				key_data_len = ret;
>> +				goto allocbuf;
>> +			}
>> +
>>  			if (copy_to_user(buffer, key_data, ret))
>>  				ret = -EFAULT;
>>  		}
>> -		kzfree(key_data);
>> +		__kvzfree(key_data, key_data_len);
>>  	}
>>  
>>  error2:
>> -- 
>> 2.18.1
>>
> Doesn't this go to infinite loop if actual key size is at least
> PAGE_SIZE + 1? Where is the guarantee that this cannot happen?

I think you may have the wrong impression that it caps the buffer length
to PAGE_SIZE. That is not true. key_data_len can be greater than
PAGE_SIZE. I run tests that include one that creates a big key of almost
1Mb. So for buflen <= PAGE_SIZE, key_data_len = buflen. For buflen >
PAGE_SIZE, key_data_len = the actual key length which can be >
PAGE_SIZE. This patch just tries to avoid allocating arbitrary large
buffer with a length much larger than the actual key length.

I will try to make the code easier to read.

Cheers,
Longman


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-20 19:19 [PATCH v6 0/2] KEYS: Read keys to internal buffer & then copy to userspace Waiman Long
2020-03-20 19:19 ` [PATCH v6 1/2] KEYS: Don't write out to userspace while holding key semaphore Waiman Long
2020-03-20 19:19 ` [PATCH v6 2/2] KEYS: Avoid false positive ENOMEM error on key read Waiman Long
2020-03-20 22:19   ` Jarkko Sakkinen
2020-03-20 22:21     ` Jarkko Sakkinen
2020-03-21  1:51     ` Waiman Long

Linux-Security-Module Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-security-module/0 linux-security-module/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-security-module linux-security-module/ https://lore.kernel.org/linux-security-module \
		linux-security-module@vger.kernel.org
	public-inbox-index linux-security-module

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-security-module


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git