* [patch 1/3] xen/privcmd: check for integer overflow in ioctl
@ 2012-09-08 9:52 ` Dan Carpenter
0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2012-09-08 9:52 UTC (permalink / raw)
To: Andres Lagar-Cavilla
Cc: kernel-janitors, Jeremy Fitzhardinge, xen-devel, virtualization,
Konrad Rzeszutek Wilk
If m.num is too large then the "m.num * sizeof(*m.arr)" multiplication
could overflow and the access_ok() check wouldn't test the right size.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Only needed in linux-next.
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 215a3c0..fdff8f9 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -325,6 +325,8 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
return -EFAULT;
/* Returns per-frame error in m.arr. */
m.err = NULL;
+ if (m.num > SIZE_MAX / sizeof(*m.arr))
+ return -EINVAL;
if (!access_ok(VERIFY_WRITE, m.arr, m.num * sizeof(*m.arr)))
return -EFAULT;
break;
@@ -332,6 +334,8 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
if (copy_from_user(&m, udata, sizeof(struct privcmd_mmapbatch_v2)))
return -EFAULT;
/* Returns per-frame error code in m.err. */
+ if (m.num > SIZE_MAX / sizeof(*m.err))
+ return -EINVAL;
if (!access_ok(VERIFY_WRITE, m.err, m.num * (sizeof(*m.err))))
return -EFAULT;
break;
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [patch 1/3] xen/privcmd: check for integer overflow in ioctl
2012-09-08 9:52 ` Dan Carpenter
@ 2012-09-09 19:49 ` Andres Lagar-Cavilla
-1 siblings, 0 replies; 10+ messages in thread
From: Andres Lagar-Cavilla @ 2012-09-09 19:49 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jeremy Fitzhardinge, xen-devel, Konrad Rzeszutek Wilk,
Andres Lagar-Cavilla, kernel-janitors, virtualization
On Sep 8, 2012, at 5:52 AM, Dan Carpenter wrote:
> If m.num is too large then the "m.num * sizeof(*m.arr)" multiplication
> could overflow and the access_ok() check wouldn't test the right size.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Acked-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> ---
> Only needed in linux-next.
>
> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> index 215a3c0..fdff8f9 100644
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -325,6 +325,8 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
> return -EFAULT;
> /* Returns per-frame error in m.arr. */
> m.err = NULL;
> + if (m.num > SIZE_MAX / sizeof(*m.arr))
> + return -EINVAL;
> if (!access_ok(VERIFY_WRITE, m.arr, m.num * sizeof(*m.arr)))
> return -EFAULT;
> break;
> @@ -332,6 +334,8 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
> if (copy_from_user(&m, udata, sizeof(struct privcmd_mmapbatch_v2)))
> return -EFAULT;
> /* Returns per-frame error code in m.err. */
> + if (m.num > SIZE_MAX / sizeof(*m.err))
> + return -EINVAL;
> if (!access_ok(VERIFY_WRITE, m.err, m.num * (sizeof(*m.err))))
> return -EFAULT;
> break;
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch 1/3] xen/privcmd: check for integer overflow in ioctl
@ 2012-09-09 19:49 ` Andres Lagar-Cavilla
0 siblings, 0 replies; 10+ messages in thread
From: Andres Lagar-Cavilla @ 2012-09-09 19:49 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jeremy Fitzhardinge, xen-devel, Konrad Rzeszutek Wilk,
Andres Lagar-Cavilla, kernel-janitors, virtualization
On Sep 8, 2012, at 5:52 AM, Dan Carpenter wrote:
> If m.num is too large then the "m.num * sizeof(*m.arr)" multiplication
> could overflow and the access_ok() check wouldn't test the right size.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Acked-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> ---
> Only needed in linux-next.
>
> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> index 215a3c0..fdff8f9 100644
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -325,6 +325,8 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
> return -EFAULT;
> /* Returns per-frame error in m.arr. */
> m.err = NULL;
> + if (m.num > SIZE_MAX / sizeof(*m.arr))
> + return -EINVAL;
> if (!access_ok(VERIFY_WRITE, m.arr, m.num * sizeof(*m.arr)))
> return -EFAULT;
> break;
> @@ -332,6 +334,8 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
> if (copy_from_user(&m, udata, sizeof(struct privcmd_mmapbatch_v2)))
> return -EFAULT;
> /* Returns per-frame error code in m.err. */
> + if (m.num > SIZE_MAX / sizeof(*m.err))
> + return -EINVAL;
> if (!access_ok(VERIFY_WRITE, m.err, m.num * (sizeof(*m.err))))
> return -EFAULT;
> break;
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch 1/3] xen/privcmd: check for integer overflow in ioctl
2012-09-08 9:52 ` Dan Carpenter
(?)
(?)
@ 2012-09-09 19:49 ` Andres Lagar-Cavilla
-1 siblings, 0 replies; 10+ messages in thread
From: Andres Lagar-Cavilla @ 2012-09-09 19:49 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jeremy Fitzhardinge, xen-devel, Konrad Rzeszutek Wilk,
Andres Lagar-Cavilla, kernel-janitors, virtualization
On Sep 8, 2012, at 5:52 AM, Dan Carpenter wrote:
> If m.num is too large then the "m.num * sizeof(*m.arr)" multiplication
> could overflow and the access_ok() check wouldn't test the right size.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Acked-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> ---
> Only needed in linux-next.
>
> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> index 215a3c0..fdff8f9 100644
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -325,6 +325,8 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
> return -EFAULT;
> /* Returns per-frame error in m.arr. */
> m.err = NULL;
> + if (m.num > SIZE_MAX / sizeof(*m.arr))
> + return -EINVAL;
> if (!access_ok(VERIFY_WRITE, m.arr, m.num * sizeof(*m.arr)))
> return -EFAULT;
> break;
> @@ -332,6 +334,8 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
> if (copy_from_user(&m, udata, sizeof(struct privcmd_mmapbatch_v2)))
> return -EFAULT;
> /* Returns per-frame error code in m.err. */
> + if (m.num > SIZE_MAX / sizeof(*m.err))
> + return -EINVAL;
> if (!access_ok(VERIFY_WRITE, m.err, m.num * (sizeof(*m.err))))
> return -EFAULT;
> break;
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Xen-devel] [patch 1/3] xen/privcmd: check for integer overflow in ioctl
2012-09-08 9:52 ` Dan Carpenter
` (2 preceding siblings ...)
(?)
@ 2012-09-10 10:35 ` David Vrabel
-1 siblings, 0 replies; 10+ messages in thread
From: David Vrabel @ 2012-09-10 10:35 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jeremy Fitzhardinge, xen-devel, Konrad Rzeszutek Wilk,
Andres Lagar-Cavilla, kernel-janitors, virtualization
On 08/09/12 10:52, Dan Carpenter wrote:
> If m.num is too large then the "m.num * sizeof(*m.arr)" multiplication
> could overflow and the access_ok() check wouldn't test the right size.
m.num is range checked later on so it doesn't matter that the
access_ok() checks might be wrong. A bit subtle, perhaps.
David
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Only needed in linux-next.
>
> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> index 215a3c0..fdff8f9 100644
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -325,6 +325,8 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
> return -EFAULT;
> /* Returns per-frame error in m.arr. */
> m.err = NULL;
> + if (m.num > SIZE_MAX / sizeof(*m.arr))
> + return -EINVAL;
> if (!access_ok(VERIFY_WRITE, m.arr, m.num * sizeof(*m.arr)))
> return -EFAULT;
> break;
> @@ -332,6 +334,8 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
> if (copy_from_user(&m, udata, sizeof(struct privcmd_mmapbatch_v2)))
> return -EFAULT;
> /* Returns per-frame error code in m.err. */
> + if (m.num > SIZE_MAX / sizeof(*m.err))
> + return -EINVAL;
> if (!access_ok(VERIFY_WRITE, m.err, m.num * (sizeof(*m.err))))
> return -EFAULT;
> break;
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Xen-devel] [patch 1/3] xen/privcmd: check for integer overflow in ioctl
2012-09-08 9:52 ` Dan Carpenter
@ 2012-09-10 10:35 ` David Vrabel
-1 siblings, 0 replies; 10+ messages in thread
From: David Vrabel @ 2012-09-10 10:35 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jeremy Fitzhardinge, xen-devel, Konrad Rzeszutek Wilk,
Andres Lagar-Cavilla, kernel-janitors, virtualization
On 08/09/12 10:52, Dan Carpenter wrote:
> If m.num is too large then the "m.num * sizeof(*m.arr)" multiplication
> could overflow and the access_ok() check wouldn't test the right size.
m.num is range checked later on so it doesn't matter that the
access_ok() checks might be wrong. A bit subtle, perhaps.
David
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Only needed in linux-next.
>
> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> index 215a3c0..fdff8f9 100644
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -325,6 +325,8 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
> return -EFAULT;
> /* Returns per-frame error in m.arr. */
> m.err = NULL;
> + if (m.num > SIZE_MAX / sizeof(*m.arr))
> + return -EINVAL;
> if (!access_ok(VERIFY_WRITE, m.arr, m.num * sizeof(*m.arr)))
> return -EFAULT;
> break;
> @@ -332,6 +334,8 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
> if (copy_from_user(&m, udata, sizeof(struct privcmd_mmapbatch_v2)))
> return -EFAULT;
> /* Returns per-frame error code in m.err. */
> + if (m.num > SIZE_MAX / sizeof(*m.err))
> + return -EINVAL;
> if (!access_ok(VERIFY_WRITE, m.err, m.num * (sizeof(*m.err))))
> return -EFAULT;
> break;
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [patch 1/3] xen/privcmd: check for integer overflow in ioctl
@ 2012-09-10 10:35 ` David Vrabel
0 siblings, 0 replies; 10+ messages in thread
From: David Vrabel @ 2012-09-10 10:35 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jeremy Fitzhardinge, xen-devel, Konrad Rzeszutek Wilk,
Andres Lagar-Cavilla, kernel-janitors, virtualization
On 08/09/12 10:52, Dan Carpenter wrote:
> If m.num is too large then the "m.num * sizeof(*m.arr)" multiplication
> could overflow and the access_ok() check wouldn't test the right size.
m.num is range checked later on so it doesn't matter that the
access_ok() checks might be wrong. A bit subtle, perhaps.
David
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Only needed in linux-next.
>
> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
> index 215a3c0..fdff8f9 100644
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -325,6 +325,8 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
> return -EFAULT;
> /* Returns per-frame error in m.arr. */
> m.err = NULL;
> + if (m.num > SIZE_MAX / sizeof(*m.arr))
> + return -EINVAL;
> if (!access_ok(VERIFY_WRITE, m.arr, m.num * sizeof(*m.arr)))
> return -EFAULT;
> break;
> @@ -332,6 +334,8 @@ static long privcmd_ioctl_mmap_batch(void __user *udata, int version)
> if (copy_from_user(&m, udata, sizeof(struct privcmd_mmapbatch_v2)))
> return -EFAULT;
> /* Returns per-frame error code in m.err. */
> + if (m.num > SIZE_MAX / sizeof(*m.err))
> + return -EINVAL;
> if (!access_ok(VERIFY_WRITE, m.err, m.num * (sizeof(*m.err))))
> return -EFAULT;
> break;
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Xen-devel] [patch 1/3] xen/privcmd: check for integer overflow in ioctl
2012-09-10 10:35 ` David Vrabel
@ 2012-09-10 11:20 ` Dan Carpenter
-1 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2012-09-10 11:20 UTC (permalink / raw)
To: David Vrabel
Cc: Jeremy Fitzhardinge, xen-devel, Konrad Rzeszutek Wilk,
Andres Lagar-Cavilla, kernel-janitors, virtualization
On Mon, Sep 10, 2012 at 11:35:11AM +0100, David Vrabel wrote:
> On 08/09/12 10:52, Dan Carpenter wrote:
> > If m.num is too large then the "m.num * sizeof(*m.arr)" multiplication
> > could overflow and the access_ok() check wouldn't test the right size.
>
> m.num is range checked later on so it doesn't matter that the
> access_ok() checks might be wrong. A bit subtle, perhaps.
>
Yeah. It's too subtle for my static checker but not so subtle for
a human being. Laziness on my part.
Please drop this patch.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Xen-devel] [patch 1/3] xen/privcmd: check for integer overflow in ioctl
@ 2012-09-10 11:20 ` Dan Carpenter
0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2012-09-10 11:20 UTC (permalink / raw)
To: David Vrabel
Cc: Jeremy Fitzhardinge, xen-devel, Konrad Rzeszutek Wilk,
Andres Lagar-Cavilla, kernel-janitors, virtualization
On Mon, Sep 10, 2012 at 11:35:11AM +0100, David Vrabel wrote:
> On 08/09/12 10:52, Dan Carpenter wrote:
> > If m.num is too large then the "m.num * sizeof(*m.arr)" multiplication
> > could overflow and the access_ok() check wouldn't test the right size.
>
> m.num is range checked later on so it doesn't matter that the
> access_ok() checks might be wrong. A bit subtle, perhaps.
>
Yeah. It's too subtle for my static checker but not so subtle for
a human being. Laziness on my part.
Please drop this patch.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 10+ messages in thread