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@xxxxxxxxxx>
>> ---
>> 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
