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

Message ID 20200320191903.19494-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 20, 2020, 7:19 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.

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

Comments

Jarkko Sakkinen March 20, 2020, 10:19 p.m. UTC | #1
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
Jarkko Sakkinen March 20, 2020, 10:21 p.m. UTC | #2
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
Waiman Long March 21, 2020, 1:51 a.m. UTC | #3
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

Patch
diff mbox series

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: