Skip to contentSkip to conversation

xen/privcmd: check for integer overflow in ioctl

PATCH NAK Dan Carpenter 3 participants 6 messages Sep 8, 2012 – Sep 10, 2012

Subsystems (3)
Kernel janitors virtio core Xen hypervisor interface
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/3 xen/privcmd: check for integer overflow in ioctl

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 <[email protected]>

---
Only needed in linux-next.

drivers/xen/privcmd.c +4 -0
index 215a3c0..fdff8f9 100644
@@ -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;
Acked-by on v1 1/3
#1 lore raw
Quoted 4 lines · Dan Carpenter

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 <[email protected]>
Acked-by Andres Lagar-Cavilla <[email protected]>
Quoted 25 lines

---
Only needed in linux-next.

drivers/xen/privcmd.c +4 -0
index 215a3c0..fdff8f9 100644
@@ -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;
Acked-by on v1 1/3
#2 lore raw
In reply to Dan Carpenter
Quoted 4 lines · Dan Carpenter

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 <[email protected]>
Acked-by Andres Lagar-Cavilla <[email protected]>
Quoted 25 lines

---
Only needed in linux-next.

drivers/xen/privcmd.c +4 -0
index 215a3c0..fdff8f9 100644
@@ -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;
#3 lore raw
In reply to Dan Carpenter
Quoted 2 lines · Dan Carpenter

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

Quoted 31 lines
Signed-off-by Dan Carpenter <[email protected]>

---
Only needed in linux-next.

drivers/xen/privcmd.c +4 -0
index 215a3c0..fdff8f9 100644
@@ -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
[email protected]
http://lists.xen.org/xen-devel

author
#4 lore raw
Quoted 7 lines · David Vrabel
Quoted 2 lines · Dan Carpenter

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

#5 lore raw
In reply to Dan Carpenter
Quoted 2 lines · Dan Carpenter

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

Quoted 31 lines
Signed-off-by Dan Carpenter <[email protected]>

---
Only needed in linux-next.

drivers/xen/privcmd.c +4 -0
index 215a3c0..fdff8f9 100644
@@ -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
[email protected]
http://lists.xen.org/xen-devel