Skip to contentSkip to conversation

fsl_hypervisor: dereferencing error pointers in ioctl

PATCH Applied Dan Carpenter 2 participants 5 messages Dec 18, 2018 – Apr 4, 2019

Subsystems (3)
Memory management Kernel janitors MM core
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/2 fsl_hypervisor: dereferencing error pointers in ioctl

The strndup_user() function returns error pointers on error, and then in the error handling we pass the error pointers to kfree(). It will cause an Oops.

Fixes 6db7199407ca ("drivers/virt: introduce Freescale hypervisor management driver")
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/virt/fsl_hypervisor.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)
drivers/virt/fsl_hypervisor.c +13 -13
index 8ba726e600e9..7b7f8e9a2801 100644
@@ -331,8 +331,8 @@ 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. */

@@ -344,32 +344,30 @@ static long ioctl_dtprop(struct fsl_hv_ioctl_prop __user *p, int set)
 	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;
+		goto err_free_path;
 	}
 
 	if (param.proplen > FH_DTPROP_MAX_PROPLEN) {
 		ret = -EINVAL;
-		goto out;
+		goto err_free_propname;
 	}
 
 	propval = kmalloc(param.proplen, GFP_KERNEL);
 	if (!propval) {
 		ret = -ENOMEM;
-		goto out;
+		goto err_free_propname;
 	}
 
 	if (set) {
 		if (copy_from_user(propval, upropval, param.proplen)) {
 			ret = -EFAULT;
-			goto out;
+			goto err_free_propval;
 		}
 
 		param.ret = fh_partition_set_dtprop(param.handle,

@@ -388,7 +386,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 err_free_propval;
 			}
 		}
 	}

@@ -396,10 +394,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);
+err_free_propval:
 	kfree(propval);
+err_free_propname:
 	kfree(propname);
+err_free_path:
+	kfree(path);
 
 	return ret;
 }
Signature · 1 line
2.17.1
author
#1 lore raw

PATCH 2/2 fsl_hypervisor: prevent integer overflow in ioctl

The "param.count" value is a u64 thatcomes from the user. The code later in the function assumes that param.count is at least one and if it's not then it leads to an Oops when we dereference the ZERO_SIZE_PTR.

Also the addition can have an integer overflow which would lead us to allocate a smaller "pages" array than required. I can't immediately tell what the possible run times implications are, but it's safest to prevent the overflow.

Fixes 6db7199407ca ("drivers/virt: introduce Freescale hypervisor management driver")
Signed-off-by Dan Carpenter <[email protected]>
---
 drivers/virt/fsl_hypervisor.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/virt/fsl_hypervisor.c +3 -0
index 7b7f8e9a2801..1bbd910d4ddb 100644
@@ -215,6 +215,9 @@ static long ioctl_memcpy(struct fsl_hv_ioctl_memcpy __user *p)
 	 * hypervisor.
 	 */
 	lb_offset = param.local_vaddr & (PAGE_SIZE - 1);
+	if (param.count = 0 ||
+	    param.count > U64_MAX - lb_offset - PAGE_SIZE + 1)
+		return -EINVAL;
 	num_pages = (param.count + lb_offset + PAGE_SIZE - 1) >> PAGE_SHIFT;
 
 	/* Allocate the buffers we need */
Signature · 1 line
2.17.1
maintainer
#3 lore raw
In reply to Dan Carpenter
Quoted 4 lines · On Tue, 18 Dec 2018 11:20:03 +0300 Dan Carpenter <[email protected]>…

The strndup_user() function returns error pointers on error, and then in the error handling we pass the error pointers to kfree(). It will cause an Oops.

Looks good to me.

I guess we should fix this too?

From: Andrew Morton <[email protected]>
Subject: mm/util.c: fix strndup_user() comment

The kerneldoc misdescribes strndup_user()'s return value.

Cc Dan Carpenter <[email protected]>
Cc Timur Tabi <[email protected]>
Cc Mihai Caraman <[email protected]>
Cc Kumar Gala <[email protected]>
Signed-off-by Andrew Morton <[email protected]>

---

 mm/util.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/util.c renamed from mm/util.c~mm-utilc-fix-strndup_user-comment +1 -1
@@ -204,7 +204,7 @@ EXPORT_SYMBOL(vmemdup_user);
  * @s: The string to duplicate
  * @n: Maximum number of bytes to copy, including the trailing NUL.
  *
- * Return: newly allocated copy of @s or %NULL in case of error
+ * Return: newly allocated copy of @s or an ERR_PTR() in case of error
  */
 char *strndup_user(const char __user *s, long n)
 {

_

author
#4 lore raw
Quoted 11 lines · Andrew Morton
Quoted 4 lines · On Tue, 18 Dec 2018 11:20:03 +0300 Dan Carpenter <[email protected]>…

The strndup_user() function returns error pointers on error, and then in the error handling we pass the error pointers to kfree(). It will cause an Oops.

Looks good to me.

I guess we should fix this too?

I didn't notice that. Thanks!

regards,
dan carpenter