linux-kernel - [PATCH v2 2/2] KEYS: Avoid false positive ENOMEM error on key read

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20200308170410.14166-3-longman@redhat.com>
Date:   Sun,  8 Mar 2020 13:04:10 -0400
From:   Waiman Long <longman@...hat.com>
To:     David Howells <dhowells@...hat.com>,
        Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>,
        James Morris <jmorris@...ei.org>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        Mimi Zohar <zohar@...ux.ibm.com>
Cc:     keyrings@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-security-module@...r.kernel.org,
        linux-integrity@...r.kernel.org,
        Sumit Garg <sumit.garg@...aro.org>,
        Jerry Snitselaar <jsnitsel@...hat.com>,
        Roberto Sassu <roberto.sassu@...wei.com>,
        Eric Biggers <ebiggers@...gle.com>,
        Chris von Recklinghausen <crecklin@...hat.com>,
        Waiman Long <longman@...hat.com>
Subject: [PATCH v2 2/2] KEYS: Avoid false positive ENOMEM error on key read

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@...hat.com>
---
 security/keys/keyctl.c | 46 ++++++++++++++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 11 deletions(-)

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