On Tue, Oct 11, 2016 at 6:10 PM, Alex Williamson
<alex.williamson@xxxxxxxxxx> wrote:
> On Tue, 11 Oct 2016 13:02:19 +0200
> Vlad Tsyrklevich <vlad@xxxxxxxxxxxxxxx> wrote:
>
>> The VFIO_DEVICE_SET_IRQS and VFIO_DEVICE_GET_PCI_HOT_RESET_INFO ioctls
>> do not sufficiently sanitize user-supplied integers, allowing users to
>> read arbitrary amounts of kernel heap memory or cause a crash.
>>
>> Signed-off-by: Vlad Tsyrklevich <vlad@xxxxxxxxxxxxxxx>
>>
>> ---
>> drivers/vfio/pci/vfio_pci.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
>> index d624a52..c3fbfb8 100644
>> --- a/drivers/vfio/pci/vfio_pci.c
>> +++ b/drivers/vfio/pci/vfio_pci.c
>> @@ -838,6 +838,7 @@ static long vfio_pci_ioctl(void *device_data,
>> return -EFAULT;
>>
>> if (hdr.argsz < minsz || hdr.index >= VFIO_PCI_NUM_IRQS ||
>> + hdr.count >= (U32_MAX - hdr.start) ||
>> hdr.flags & ~(VFIO_IRQ_SET_DATA_TYPE_MASK |
>> VFIO_IRQ_SET_ACTION_TYPE_MASK))
>> return -EINVAL;
>
>
> Isn't the issue you're trying to solve here caught in the code block
> below this? ie.
>
> 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);
> else if (hdr.flags & VFIO_IRQ_SET_DATA_EVENTFD)
> size = sizeof(int32_t);
> else
> return -EINVAL;
>
> if (hdr.argsz - minsz < hdr.count * size ||
> hdr.start >= max || hdr.start + hdr.count > max)
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> return -EINVAL;
>
> data = memdup_user((void __user *)(arg + minsz),
> hdr.count * size);
> if (IS_ERR(data))
> return PTR_ERR(data);
> }
>
> In fact we're doing better than allowing U32_MAX, we're testing the
> number of indexes for a given IRQ and making sure the user hasn't
> exceeded that.
That's true, though strictly speaking integer overflows are still
possible. For example, if hdr.count is set to U32_MAX, hdr.start +
hdr.count will overflow and wrap around to hdr.start - 1. That won't
really matter if VFIO_IRQ_SET_DATA_NONE is cleared since memdup_user
should fail early; however, if DATA_NONE is set you could reach the
following code in vfio_pci_set_msi_trigger():
if (!irq_is(vdev, index) || start + count > vdev->num_ctx)
return -EINVAL;
for (i = start; i < start + count; i++) {
if (!vdev->ctx[i].trigger)
continue;
if (flags & VFIO_IRQ_SET_DATA_NONE) {
eventfd_signal(vdev->ctx[i].trigger, 1);
} else if (flags & VFIO_IRQ_SET_DATA_BOOL) {
uint8_t *bools = data;
if (bools[i - start])
eventfd_signal(vdev->ctx[i].trigger, 1);
}
}
In this case you could hand pick values of start and count such that
start + count overflow to be less than vdev->num_ctx but still allow
you to access/write to arbitrary offsets from vdev->ctx. Disallowing
integer overflows for any code path reachable through
vfio_pci_set_irqs_ioctl() is a stronger solution than allowing some
overflows by adding the start >= max comparison.
While I was writing this out I think I also found another potential
issue. I don't believe anything checks that only one of DATA_NONE,
DATA_BOOL, DATA_EVENTFD is set, so if you set flags to DATA_NONE |
DATA_EVENTFD, you can get around the start and count checks you noted
above. You could reach the following in vfio_pci_set_msi_trigger():
if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
int32_t *fds = data;
int ret;
if (vdev->irq_type == index)
return vfio_msi_set_block(vdev, start, count,
fds, msix);
ret = vfio_msi_enable(vdev, start + count, msix);
if (ret)
return ret;
At this point start and count have not been validated at all. Even if
you add the patch I sent above you could still reach an integer
overflow in vfio_msi_enable():
static int vfio_msi_enable(struct vfio_pci_device *vdev, int nvec, bool msix)
{
...
vdev->ctx = kzalloc(nvec * sizeof(struct vfio_pci_irq_ctx), GFP_KERNEL);
if (!vdev->ctx)
return -ENOMEM;
/* return the number of supported vectors if we can't get all: */
ret = pci_alloc_irq_vectors(pdev, 1, nvec, flag);
if (ret < nvec) {
if (ret > 0)
pci_free_irq_vectors(pdev);
kfree(vdev->ctx);
return ret;
}
On 32-bit systems you could pick values of start and count such that
nvec [start + count] * sizeof(struct vfio_pci_irq_ctx) overflows. The
kzalloc allocation would succeed, but pci_alloc_irq_vectors() would
fail. In the error handler, it would free vdev->ctx but not set the
pointer to NULL. Future calls to the vfio ioctls could lead to a
use-after-free of vdev->ctx. I just found this and haven't had time to
convince myself if it's possible, does that look reasonable?
>> @@ -909,6 +910,9 @@ static long vfio_pci_ioctl(void *device_data,
>>
>> WARN_ON(!fill.max); /* Should always be at least one */
>>
>> + if (hdr.count > fill.max)
>> + hdr.count = fill.max;
>> +
>> /*
>> * If there's enough space, fill it now, otherwise return
>> * -ENOSPC and the number of devices affected.
>
> hdr.count is an output for this ioctl, not an input. hdr.count is set
> on all success paths and on the -ENOSPC path, the user cannot rely on a
> value for any other error cases.
You're completely right. I had an error in my static analysis and
misunderstood the control-flow on manual inspection. Sorry for the
noise.
> I'm not seeing that there's an issue on either path here, please
> elaborate if you still disagree. Thanks,
If the two issues I noted above look legitimate to you, I'm happy to
submit an updated patch.
> Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
