linux-kernel - [patch 1/2] drivers/virt: fix the error handling in ioctl

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: <20160714114139.GK18175@mwanda>
Date:	Thu, 14 Jul 2016 14:41:39 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Dave Hansen <dave.hansen@...ux.intel.com>,
	Timur Tabi <timur@...escale.com>
Cc:	Dave Hansen <dave.hansen@...ux.intel.com>,
	Ingo Molnar <mingo@...nel.org>, linux-kernel@...r.kernel.org,
	kernel-janitors@...r.kernel.org
Subject: [patch 1/2] drivers/virt: fix the error handling in ioctl_dtprop()

If strndup_user() user fails then it returns an error pointer and we
pass that to kfree() which causes an oops.

I've shifted this code around so that we keep only free things which
have been allocated.  We don't need to initialize the pointers at the
start any more.  We can also move the check for invalid proplen earlier
so it's before the allocations.

Fixes: 6db7199407ca ('drivers/virt: introduce Freescale hypervisor management driver')
Signed-off-by: Dan Carpenter <dan.carpenter@...cle.com>
---
I have to be honest here.  I haven't "properly" compiled this.  I hacked
up Smatch to do a best effort compile of code even if there were include
files missing etc.

diff --git a/drivers/virt/fsl_hypervisor.c b/drivers/virt/fsl_hypervisor.c
index 60bdad3..146f531 100644
--- a/drivers/virt/fsl_hypervisor.c
+++ b/drivers/virt/fsl_hypervisor.c
@@ -334,45 +334,41 @@ static long ioctl_dtprop(struct fsl_hv_ioctl_prop __user *p, int set)
 	struct fsl_hv_ioctl_prop param;
 	char __user *upath, *upropname;
 	void __user *upropval;
-	char *path = NULL, *propname = NULL;
-	void *propval = NULL;
+	char *path, *propname;
+	void *propval;
 	int ret = 0;
 
 	/* Get the parameters from the user. */
 	if (copy_from_user(&param, p, sizeof(struct fsl_hv_ioctl_prop)))
 		return -EFAULT;
 
+	if (param.proplen > FH_DTPROP_MAX_PROPLEN)
+		return -EINVAL;
+
 	upath = (char __user *)(uintptr_t)param.path;
 	upropname = (char __user *)(uintptr_t)param.propname;
 	upropval = (void __user *)(uintptr_t)param.propval;
 
 	path = strndup_user(upath, FH_DTPROP_MAX_PATHLEN);
-	if (IS_ERR(path)) {
-		ret = PTR_ERR(path);
-		goto out;
-	}
+	if (IS_ERR(path))
+		return PTR_ERR(path);
 
 	propname = strndup_user(upropname, FH_DTPROP_MAX_PATHLEN);
 	if (IS_ERR(propname)) {
 		ret = PTR_ERR(propname);
-		goto out;
-	}
-
-	if (param.proplen > FH_DTPROP_MAX_PROPLEN) {
-		ret = -EINVAL;
-		goto out;
+		goto free_path;
 	}
 
 	propval = kmalloc(param.proplen, GFP_KERNEL);
 	if (!propval) {
 		ret = -ENOMEM;
-		goto out;
+		goto free_propname;
 	}
 
 	if (set) {
 		if (copy_from_user(propval, upropval, param.proplen)) {
 			ret = -EFAULT;
-			goto out;
+			goto free_propval;
 		}
 
 		param.ret = fh_partition_set_dtprop(param.handle,
@@ -391,7 +387,7 @@ static long ioctl_dtprop(struct fsl_hv_ioctl_prop __user *p, int set)
 			if (copy_to_user(upropval, propval, param.proplen) ||
 			    put_user(param.proplen, &p->proplen)) {
 				ret = -EFAULT;
-				goto out;
+				goto free_propval;
 			}
 		}
 	}
@@ -399,10 +395,12 @@ static long ioctl_dtprop(struct fsl_hv_ioctl_prop __user *p, int set)
 	if (put_user(param.ret, &p->ret))
 		ret = -EFAULT;
 
-out:
-	kfree(path);
+free_propval:
 	kfree(propval);
+free_propname:
 	kfree(propname);
+free_path:
+	kfree(path);
 
 	return ret;
 }