Update the linux secure_key patch to match the keyctl.c api changes

cancel
Showing results for 
Show  only  | Search instead for 
Did you mean: 

Update the linux secure_key patch to match the keyctl.c api changes

252 Views
charles_hardin
Contributor I

The keyctl api used to call into the read for the key types and expect them to copy the key data to user space, but that was changed to avoid false ENOMEM

commit 419d8fb1630cbb04883fc73e08f37400a1e8ce86
Author: Waiman Long <longman@redhat.com>
Date: Sat Mar 21 21:11:25 2020 -0400

KEYS: Avoid false positive ENOMEM error on key read

[ Upstream commit 4f0882491a148059a52480e753b7f07fc550e188 ]

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>
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>


So, this means that the secure key code would get a kernel buffer and return EFAULT instead of just a pack into the kernel memory. The diff below fixes this problem on the lf-5.10.y branch - not sure if this is already fixed in another branch.

diff --git a/security/keys/secure_key.c b/security/keys/secure_key.c
index ec8ad4394549..623513bd3fd1 100644
--- a/security/keys/secure_key.c
+++ b/security/keys/secure_key.c
@@ -228,17 +228,16 @@ static int secure_instantiate(struct key *key,
 }
 
 /*
- * secure_read - copy the  blob data to userspace in hex.
+ * secure_read - copy the blob data eventually to userspace in hex.
  * param[in]: key pointer to key struct
- * param[in]: buffer pointer to user data for creating key
+ * param[in]: buffer pointer to kernel memory for storing key
  * param[in]: buflen is the length of the buffer
- * On success, return to userspace the secure key data size.
+ * On success, return the secure key data size.
  */
-static long secure_read(const struct key *key, char __user *buffer,
+static long secure_read(const struct key *key, char *buffer,
 			 size_t buflen)
 {
-	const struct secure_key_payload *p = NULL;
-	char *ascii_buf;
+	const struct secure_key_payload *p;
 	char *bufp;
 	int i;
 
@@ -247,18 +246,9 @@ static long secure_read(const struct key *key, char __user *buffer,
 		return -EINVAL;
 
 	if (buffer && buflen >= 2 * p->blob_len) {
-		ascii_buf = kmalloc(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;
 }

 

0 Kudos
Reply
0 Replies