[v2,2/2] KEYS: Avoid false positive ENOMEM error on key read
diff mbox series

Message ID 20200308170410.14166-3-longman@redhat.com
State New
Headers show
Series
  • KEYS: Read keys to internal buffer & then copy to userspace
Related show

Commit Message

Waiman Long March 8, 2020, 5:04 p.m. UTC
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(-)

Comments

David Howells March 9, 2020, 4:32 p.m. UTC | #1
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
Waiman Long March 10, 2020, 3:45 p.m. UTC | #2
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
Waiman Long March 10, 2020, 3:58 p.m. UTC | #3
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
David Howells March 10, 2020, 5:12 p.m. UTC | #4
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
Waiman Long March 11, 2020, 3:33 p.m. UTC | #5
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

Patch
diff mbox series

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);