Skip to contentSkip to conversation

vfio-pci: integer overflow in vfio_pci_ioctl()

PATCH In review Dan Carpenter 2 participants 4 messages Mar 26, 2013

Subsystems (3)
KVM Kernel janitors VFIO driver
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 vfio-pci: integer overflow in vfio_pci_ioctl()

The worry here is that a large value of hdr.start would cause a read before the start of the array and a crash in vfio_msi_set_vector_signal().

The check in vfio_msi_set_block() is not enough:

	if (start + count > vdev->num_ctx)
		return -EINVAL;

A large value of "start" would lead to an integer overflow.

The check in vfio_msi_set_vector_signal() doesn't work either:

	if (vector >= vdev->num_ctx)
		return -EINVAL;

Here "vector" is "count" casted to a signed int so it would be negative and thus smaller than "vdev->num_ctx" which is also a signed int.

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

---
Static analysis stuff. Untested.

This patch is not beautiful. There is probably a better limit to use if I knew the code.

drivers/vfio/pci/vfio_pci.c +3 -0
index acfcb1a..de54f69 100644
@@ -371,6 +371,9 @@ static long vfio_pci_ioctl(void *device_data,
 			    hdr.count > vfio_pci_get_irq_count(vdev, hdr.index))
 				return -EINVAL;
 
+			if (hdr.start > INT_MAX - hdr.count)
+				return -EINVAL;
+
 			data = memdup_user((void __user *)(arg + minsz),
 					   hdr.count * size);
 			if (IS_ERR(data))
author
#1 lore raw
Quoted 9 lines · Dan Carpenter

The worry here is that a large value of hdr.start would cause a read before the start of the array and a crash in vfio_msi_set_vector_signal().

The check in vfio_msi_set_block() is not enough:

	if (start + count > vdev->num_ctx)
		return -EINVAL;
Quoted 8 lines

A large value of "start" would lead to an integer overflow.

The check in vfio_msi_set_vector_signal() doesn't work either:

	if (vector >= vdev->num_ctx)
		return -EINVAL;

Here "vector" is "count" casted to a signed int so it would be negative

                    ^^^^^

I meant "start" not "count".

Quoted 2 lines

and thus smaller than "vdev->num_ctx" which is also a signed int.

Gar... I hate this patch already. I really should make vdev->num_ctx an unsigned int. I'll send that in a v2 along with whatever other suggestions people send.

regards,
dan carpenter

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

The worry here is that a large value of hdr.start would cause a read before the start of the array and a crash in vfio_msi_set_vector_signal().

The check in vfio_msi_set_block() is not enough:

	if (start + count > vdev->num_ctx)
		return -EINVAL;

A large value of "start" would lead to an integer overflow.

The check in vfio_msi_set_vector_signal() doesn't work either:

	if (vector >= vdev->num_ctx)
		return -EINVAL;

Here "vector" is "count" casted to a signed int so it would be negative and thus smaller than "vdev->num_ctx" which is also a signed int.

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

---
Static analysis stuff. Untested.

This patch is not beautiful. There is probably a better limit to use if I knew the code.

drivers/vfio/pci/vfio_pci.c +0 -0
index acfcb1a..de54f69 100644

drivers/vfio/pci/vfio_pci.c static long vfio_pci_ioctl(void *device_data, · line 379

Quoted 9 lines
drivers/vfio/pci/vfio_pci.c +3 -0
@@ -371,6 +371,9 @@ static long vfio_pci_ioctl(void *device_data,
 			    hdr.count > vfio_pci_get_irq_count(vdev, hdr.index))
 				return -EINVAL;
 
+			if (hdr.start > INT_MAX - hdr.count)
+				return -EINVAL;
+
 			data = memdup_user((void __user *)(arg + minsz),
 					   hdr.count * size);
 			if (IS_ERR(data))

Thanks Dan. Is this more like what you're looking for?

commit 16deadb9d424c0af2162a5be2935fbc4e0f09b98

Author: Alex Williamson <[email protected]>
Date:   Tue Mar 26 08:59:22 2013 -0600

vfio-pci: Fix possible integer overflow

The VFIO_DEVICE_SET_IRQS ioctl takes a start and count parameter, both of which are unsigned. We attempt to bounds check these, but fail to account for the case where start is a very large number, allowing start + count to wrap back into the valid range. Bounds check both start and start + count.

Reported-by Dan Carpenter <[email protected]>
Signed-off-by Alex Williamson <[email protected]>
drivers/vfio/pci/vfio_pci.c +2 -1
index acfcb1a..866bde1 100644
@@ -359,6 +359,7 @@ static long vfio_pci_ioctl(void *device_data,
 
 		if (!(hdr.flags & VFIO_IRQ_SET_DATA_NONE)) {
 			size_t size;
+			int max = vfio_pci_get_irq_count(vdev, hdr.index);
 
 			if (hdr.flags & VFIO_IRQ_SET_DATA_BOOL)
 				size = sizeof(uint8_t);

@@ -368,7 +369,7 @@ static long vfio_pci_ioctl(void *device_data,
 				return -EINVAL;
 
 			if (hdr.argsz - minsz < hdr.count * size ||
-			    hdr.count > vfio_pci_get_irq_count(vdev, hdr.index))
+			    hdr.start >= max || hdr.start + hdr.count > max)
 				return -EINVAL;
 
 			data = memdup_user((void __user *)(arg + minsz),
author
#3 lore raw
Quoted 2 lines · Alex Williamson

Thanks Dan. Is this more like what you're looking for?

Yes. That looks nice. :)

regards,
dan carpenter