linux-kernel - [PATCH] fs: coda: fix a double-fetch case in coda_psdev

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-next>] [day] [month] [year] [list]

Message-Id: <20181225185814.69047-1-kjlu@umn.edu>
Date:   Tue, 25 Dec 2018 12:58:14 -0600
From:   Kangjie Lu <kjlu@....edu>
To:     kjlu@....edu
Cc:     pakki001@....edu, Jan Harkes <jaharkes@...cmu.edu>,
        coda@...cmu.edu, codalist@...a.cs.cmu.edu,
        linux-kernel@...r.kernel.org
Subject: [PATCH] fs: coda: fix a double-fetch case in coda_psdev_write

"hdr" has been copied in from user space and "hdr.opcode" is checked.
The code copies it again. User space data between the two copies is
subject to modification if the user-space code is multithreaded and
malicious. The modification may invalidate the check. In the original
code, "hdr.opcode" is not used after the second copy, so it is actually
safe with the current code. However, in case future code may use
"hdr.opcode", let's avoid copying the header from user space again.

Signed-off-by: Kangjie Lu <kjlu@....edu>
---
 fs/coda/psdev.c | 41 ++++++++++++++++++++++-------------------
 1 file changed, 22 insertions(+), 19 deletions(-)

diff --git a/fs/coda/psdev.c b/fs/coda/psdev.c
index c5234c21b539..fb4d1c654773 100644
--- a/fs/coda/psdev.c
+++ b/fs/coda/psdev.c
@@ -97,35 +97,38 @@ static long coda_psdev_ioctl(struct file * filp, unsigned int cmd, unsigned long
 static ssize_t coda_psdev_write(struct file *file, const char __user *buf, 
 				size_t nbytes, loff_t *off)
 {
-        struct venus_comm *vcp = (struct venus_comm *) file->private_data;
-        struct upc_req *req = NULL;
-        struct upc_req *tmp;
+	struct venus_comm *vcp = (struct venus_comm *) file->private_data;
+	struct upc_req *req = NULL;
+	struct upc_req *tmp;
 	struct list_head *lh;
 	struct coda_in_hdr hdr;
 	ssize_t retval = 0, count = 0;
 	int error;
 
-        /* Peek at the opcode, uniquefier */
+	/* Peek at the opcode, uniquefier */
 	if (copy_from_user(&hdr, buf, 2 * sizeof(u_long)))
-	        return -EFAULT;
+		return -EFAULT;
 
-        if (DOWNCALL(hdr.opcode)) {
+	if (DOWNCALL(hdr.opcode)) {
 		union outputArgs *dcbuf;
 		int size = sizeof(*dcbuf);
 
 		if  ( nbytes < sizeof(struct coda_out_hdr) ) {
 			pr_warn("coda_downcall opc %d uniq %d, not enough!\n",
-				hdr.opcode, hdr.unique);
+					hdr.opcode, hdr.unique);
 			count = nbytes;
 			goto out;
 		}
 		if ( nbytes > size ) {
 			pr_warn("downcall opc %d, uniq %d, too much!",
-				hdr.opcode, hdr.unique);
-		        nbytes = size;
+					hdr.opcode, hdr.unique);
+			nbytes = size;
 		}
 		CODA_ALLOC(dcbuf, union outputArgs *, nbytes);
-		if (copy_from_user(dcbuf, buf, nbytes)) {
+		*((struct coda_in_hdr *)dcbuf) = hdr;
+		if (copy_from_user(dcbuf + sizeof(hdr),
+					buf + sizeof(hdr),
+					nbytes - sizeof(hdr))) {
 			CODA_FREE(dcbuf, nbytes);
 			retval = -EFAULT;
 			goto out;
@@ -137,14 +140,14 @@ static ssize_t coda_psdev_write(struct file *file, const char __user *buf,
 		CODA_FREE(dcbuf, nbytes);
 		if (error) {
 			pr_warn("%s: coda_downcall error: %d\n",
-				__func__, error);
+					__func__, error);
 			retval = error;
 			goto out;
 		}
 		count = nbytes;
 		goto out;
 	}
-        
+
 	/* Look for the message on the processing queue. */
 	mutex_lock(&vcp->vc_mutex);
 	list_for_each(lh, &vcp->vc_processing) {
@@ -159,19 +162,19 @@ static ssize_t coda_psdev_write(struct file *file, const char __user *buf,
 
 	if (!req) {
 		pr_warn("%s: msg (%d, %d) not found\n",
-			__func__, hdr.opcode, hdr.unique);
+				__func__, hdr.opcode, hdr.unique);
 		retval = -ESRCH;
 		goto out;
 	}
 
-        /* move data into response buffer. */
+	/* move data into response buffer. */
 	if (req->uc_outSize < nbytes) {
 		pr_warn("%s: too much cnt: %d, cnt: %ld, opc: %d, uniq: %d.\n",
-			__func__, req->uc_outSize, (long)nbytes,
-			hdr.opcode, hdr.unique);
+				__func__, req->uc_outSize, (long)nbytes,
+				hdr.opcode, hdr.unique);
 		nbytes = req->uc_outSize; /* don't have more space! */
 	}
-        if (copy_from_user(req->uc_data, buf, nbytes)) {
+	if (copy_from_user(req->uc_data, buf, nbytes)) {
 		req->uc_flags |= CODA_REQ_ABORT;
 		wake_up(&req->uc_sleep);
 		retval = -EFAULT;
@@ -191,9 +194,9 @@ static ssize_t coda_psdev_write(struct file *file, const char __user *buf,
 			outp->fh = fget(outp->fd);
 	}
 
-        wake_up(&req->uc_sleep);
+	wake_up(&req->uc_sleep);
 out:
-        return(count ? count : retval);  
+	return(count ? count : retval);
 }
 
 /*
-- 
2.17.2 (Apple Git-113)