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

Message ID 20200313152102.1707-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 13, 2020, 3:21 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 | 48 ++++++++++++++++++++++++++++++++----------
 1 file changed, 37 insertions(+), 11 deletions(-)

Comments

Jarkko Sakkinen March 15, 2020, 9:32 p.m. UTC | #1
On Fri, Mar 13, 2020 at 11:21:01AM -0400, Waiman Long wrote:
> -		 * Read methods will just return the required length
> -		 * without any copying if the provided length isn't big
> -		 * enough.
> +		 * 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 <= 0x400) {

1. The overwhelmingly long comment. Will be destined to rotten.
2. Magic number.
3. The cap must be updated both in comment and code, and not only
   that, but the numbers use a different base (dec and hex).

/Jarkko
Waiman Long March 17, 2020, 6:36 p.m. UTC | #2
On 3/15/20 5:32 PM, Jarkko Sakkinen wrote:
> On Fri, Mar 13, 2020 at 11:21:01AM -0400, Waiman Long wrote:
>> -		 * Read methods will just return the required length
>> -		 * without any copying if the provided length isn't big
>> -		 * enough.
>> +		 * 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 <= 0x400) {
> 1. The overwhelmingly long comment. Will be destined to rotten.
> 2. Magic number.
> 3. The cap must be updated both in comment and code, and not only
>    that, but the numbers use a different base (dec and hex).
>
> /Jarkko
>
Thank for the comment. I will make the necessary change.

Cheers,
Longman

Patch
diff mbox series

diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 81f68e434b9f..a05a4dd2f9ce 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -877,24 +877,50 @@  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 *tmpbuf = kmalloc(buflen, GFP_KERNEL);
-
-		if (!tmpbuf) {
-			ret = -ENOMEM;
-			goto error2;
-		}
-		ret = __keyctl_read_key(key, tmpbuf, buflen);
+		char *tmpbuf = NULL;
+		size_t tmpbuflen = buflen;
 
 		/*
-		 * Read methods will just return the required length
-		 * without any copying if the provided length isn't big
-		 * enough.
+		 * 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 <= 0x400) {
+allocbuf:
+			tmpbuf = kmalloc(tmpbuflen, GFP_KERNEL);
+			if (!tmpbuf) {
+				ret = -ENOMEM;
+				goto error2;
+			}
+		}
+
+		ret = __keyctl_read_key(key, tmpbuf, tmpbuflen);
 		if ((ret > 0) && (ret <= buflen)) {
+			/*
+			 * 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 > tmpbuflen)) {
+				if (unlikely(tmpbuf))
+					kzfree(tmpbuf);
+				tmpbuflen = ret;
+				goto allocbuf;
+			}
+
 			if (copy_to_user(buffer, tmpbuf, ret))
 				ret = -EFAULT;
 		}
-		kzfree(tmpbuf);
+		if (tmpbuf)
+			kzfree(tmpbuf);
 	}
 
 error2: