All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] fsl_hypervisor: dereferencing error pointers in ioctl
@ 2018-12-18  8:20 ` Dan Carpenter
  0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2018-12-18  8:20 UTC (permalink / raw)
  To: Andrew Morton, Timur Tabi
  Cc: linux-kernel, kernel-janitors, Mihai Caraman, Kumar Gala

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 <dan.carpenter@oracle.com>
---
 drivers/virt/fsl_hypervisor.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/virt/fsl_hypervisor.c b/drivers/virt/fsl_hypervisor.c
index 8ba726e600e9..7b7f8e9a2801 100644
--- a/drivers/virt/fsl_hypervisor.c
+++ b/drivers/virt/fsl_hypervisor.c
@@ -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;
 }
-- 
2.17.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/2] fsl_hypervisor: dereferencing error pointers in ioctl
@ 2018-12-18  8:20 ` Dan Carpenter
  0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2018-12-18  8:20 UTC (permalink / raw)
  To: Andrew Morton, Timur Tabi
  Cc: linux-kernel, kernel-janitors, Mihai Caraman, Kumar Gala

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 <dan.carpenter@oracle.com>
---
 drivers/virt/fsl_hypervisor.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/virt/fsl_hypervisor.c b/drivers/virt/fsl_hypervisor.c
index 8ba726e600e9..7b7f8e9a2801 100644
--- a/drivers/virt/fsl_hypervisor.c
+++ b/drivers/virt/fsl_hypervisor.c
@@ -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;
 }
-- 
2.17.1

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/2] fsl_hypervisor: prevent integer overflow in ioctl
  2018-12-18  8:20 ` Dan Carpenter
@ 2018-12-18  8:21   ` Dan Carpenter
  -1 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2018-12-18  8:21 UTC (permalink / raw)
  To: Andrew Morton, Timur Tabi
  Cc: linux-kernel, kernel-janitors, Mihai Caraman, Kumar Gala

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 <dan.carpenter@oracle.com>
---
 drivers/virt/fsl_hypervisor.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/virt/fsl_hypervisor.c b/drivers/virt/fsl_hypervisor.c
index 7b7f8e9a2801..1bbd910d4ddb 100644
--- a/drivers/virt/fsl_hypervisor.c
+++ b/drivers/virt/fsl_hypervisor.c
@@ -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 */
-- 
2.17.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/2] fsl_hypervisor: prevent integer overflow in ioctl
@ 2018-12-18  8:21   ` Dan Carpenter
  0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2018-12-18  8:21 UTC (permalink / raw)
  To: Andrew Morton, Timur Tabi
  Cc: linux-kernel, kernel-janitors, Mihai Caraman, Kumar Gala

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 <dan.carpenter@oracle.com>
---
 drivers/virt/fsl_hypervisor.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/virt/fsl_hypervisor.c b/drivers/virt/fsl_hypervisor.c
index 7b7f8e9a2801..1bbd910d4ddb 100644
--- a/drivers/virt/fsl_hypervisor.c
+++ b/drivers/virt/fsl_hypervisor.c
@@ -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 */
-- 
2.17.1

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] fsl_hypervisor: dereferencing error pointers in ioctl
  2018-12-18  8:20 ` Dan Carpenter
@ 2019-04-04 10:13   ` Dan Carpenter
  -1 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2019-04-04 10:13 UTC (permalink / raw)
  To: Andrew Morton, Timur Tabi
  Cc: linux-kernel, kernel-janitors, Mihai Caraman, Kumar Gala

I sent this patch series right before Christmas so everyone was on
holiday.  It still applies fine.  Could you take a look at it?

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] fsl_hypervisor: dereferencing error pointers in ioctl
@ 2019-04-04 10:13   ` Dan Carpenter
  0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2019-04-04 10:13 UTC (permalink / raw)
  To: Andrew Morton, Timur Tabi
  Cc: linux-kernel, kernel-janitors, Mihai Caraman, Kumar Gala

I sent this patch series right before Christmas so everyone was on
holiday.  It still applies fine.  Could you take a look at it?

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] fsl_hypervisor: dereferencing error pointers in ioctl
  2018-12-18  8:20 ` Dan Carpenter
@ 2019-04-04 19:10   ` Andrew Morton
  -1 siblings, 0 replies; 10+ messages in thread
From: Andrew Morton @ 2019-04-04 19:10 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Timur Tabi, linux-kernel, kernel-janitors, Mihai Caraman, Kumar Gala

On Tue, 18 Dec 2018 11:20:03 +0300 Dan Carpenter <dan.carpenter@oracle.com> wrote:

> 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 <akpm@linux-foundation.org>
Subject: mm/util.c: fix strndup_user() comment

The kerneldoc misdescribes strndup_user()'s return value.

Cc: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Timur Tabi <timur@freescale.com>
Cc: Mihai Caraman <mihai.caraman@freescale.com>
Cc: Kumar Gala <galak@kernel.crashing.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 mm/util.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/mm/util.c~mm-utilc-fix-strndup_user-comment
+++ a/mm/util.c
@@ -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)
 {
_


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] fsl_hypervisor: dereferencing error pointers in ioctl
@ 2019-04-04 19:10   ` Andrew Morton
  0 siblings, 0 replies; 10+ messages in thread
From: Andrew Morton @ 2019-04-04 19:10 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Timur Tabi, linux-kernel, kernel-janitors, Mihai Caraman, Kumar Gala

On Tue, 18 Dec 2018 11:20:03 +0300 Dan Carpenter <dan.carpenter@oracle.com> wrote:

> 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 <akpm@linux-foundation.org>
Subject: mm/util.c: fix strndup_user() comment

The kerneldoc misdescribes strndup_user()'s return value.

Cc: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Timur Tabi <timur@freescale.com>
Cc: Mihai Caraman <mihai.caraman@freescale.com>
Cc: Kumar Gala <galak@kernel.crashing.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 mm/util.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/mm/util.c~mm-utilc-fix-strndup_user-comment
+++ a/mm/util.c
@@ -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)
 {
_

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] fsl_hypervisor: dereferencing error pointers in ioctl
  2019-04-04 19:10   ` Andrew Morton
@ 2019-04-04 19:14     ` Dan Carpenter
  -1 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2019-04-04 19:14 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Timur Tabi, linux-kernel, kernel-janitors, Mihai Caraman, Kumar Gala

On Thu, Apr 04, 2019 at 12:10:44PM -0700, Andrew Morton wrote:
> On Tue, 18 Dec 2018 11:20:03 +0300 Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> > 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] fsl_hypervisor: dereferencing error pointers in ioctl
@ 2019-04-04 19:14     ` Dan Carpenter
  0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2019-04-04 19:14 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Timur Tabi, linux-kernel, kernel-janitors, Mihai Caraman, Kumar Gala

On Thu, Apr 04, 2019 at 12:10:44PM -0700, Andrew Morton wrote:
> On Tue, 18 Dec 2018 11:20:03 +0300 Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> > 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-04-04 19:14 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-18  8:20 [PATCH 1/2] fsl_hypervisor: dereferencing error pointers in ioctl Dan Carpenter
2018-12-18  8:20 ` Dan Carpenter
2018-12-18  8:21 ` [PATCH 2/2] fsl_hypervisor: prevent integer overflow " Dan Carpenter
2018-12-18  8:21   ` Dan Carpenter
2019-04-04 10:13 ` [PATCH 1/2] fsl_hypervisor: dereferencing error pointers " Dan Carpenter
2019-04-04 10:13   ` Dan Carpenter
2019-04-04 19:10 ` Andrew Morton
2019-04-04 19:10   ` Andrew Morton
2019-04-04 19:14   ` Dan Carpenter
2019-04-04 19:14     ` Dan Carpenter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.