vfio-pci: integer overflow in vfio_pci_ioctl()
Subsystems (3)
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.
---
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
@@ -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))
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
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
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.
drivers/vfio/pci/vfio_pci.c +2 -1
@@ -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),
Quoted 2 lines · Alex Williamson
Thanks Dan. Is this more like what you're looking for?
Yes. That looks nice. :)
regards,
dan carpenter