On Wed, Oct 12, 2016 at 4:51 PM, Alex Williamson
<alex.williamson@xxxxxxxxxx> wrote:
> On Wed, 12 Oct 2016 09:53:49 +0200
> Vlad Tsyrklevich <vlad@xxxxxxxxxxxxxxx> wrote:
>
>> The VFIO_DEVICE_SET_IRQS ioctl did not sufficiently sanitize
>> user-supplied integers, potentially allowing arbitrary memory writes.
>> This patch adds appropriate integer checks, and also verifies that only
>> a single element in the VFIO_IRQ_SET_DATA_TYPE_MASK bitmask is set.
>> VFIO_IRQ_SET_ACTION_TYPE_MASK is already correctly checked later in
>> vfio_pci_set_irqs_ioctl().
>>
>> Signed-off-by: Vlad Tsyrklevich <vlad@xxxxxxxxxxxxxxx>
>> ---
>> drivers/vfio/pci/vfio_pci.c | 26 +++++++++++++++++---------
>> drivers/vfio/pci/vfio_pci_intrs.c | 2 +-
>> 2 files changed, 18 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
>> index d624a52..44de13c 100644
>> --- a/drivers/vfio/pci/vfio_pci.c
>> +++ b/drivers/vfio/pci/vfio_pci.c
>> @@ -829,6 +829,7 @@ static long vfio_pci_ioctl(void *device_data,
>>
>> } else if (cmd == VFIO_DEVICE_SET_IRQS) {
>> struct vfio_irq_set hdr;
>> + size_t size;
>> u8 *data = NULL;
>> int ret = 0;
>>
>> @@ -838,20 +839,27 @@ 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;
>>
>> - if (!(hdr.flags & VFIO_IRQ_SET_DATA_NONE)) {
>> - size_t size;
>> - int max = vfio_pci_get_irq_count(vdev, hdr.index);
>> + switch (hdr.flags & VFIO_IRQ_SET_DATA_TYPE_MASK) {
>> + case VFIO_IRQ_SET_DATA_NONE:
>> + size = 0;
>> + break;
>> + case VFIO_IRQ_SET_DATA_BOOL:
>> + size = sizeof(uint8_t);
>> + break;
>> + case VFIO_IRQ_SET_DATA_EVENTFD:
>> + size = sizeof(int32_t);
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>>
>> - 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 (size) {
>> + int max = vfio_pci_get_irq_count(vdev, hdr.index);
>
>
> Why not pull this out too so we can fully validate start/count for the
> DATA_NONE case?
I'm not familiar with the vfio subsystem so I wasn't sure if the check
against vfio_pci_get_irq_count() was also valid for the DATA_NONE
case. It turns out that the ioctls also all check start/count as well
so it's not strictly necessary, but I'm happy to also break out that
case here.
>>
>> if (hdr.argsz - minsz < hdr.count * size ||
>> hdr.start >= max || hdr.start + hdr.count > max)
>> diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
>> index c2e6089..1c46045 100644
>> --- a/drivers/vfio/pci/vfio_pci_intrs.c
>> +++ b/drivers/vfio/pci/vfio_pci_intrs.c
>> @@ -256,7 +256,7 @@ static int vfio_msi_enable(struct vfio_pci_device *vdev, int nvec, bool msix)
>> if (!is_irq_none(vdev))
>> return -EINVAL;
>>
>> - vdev->ctx = kzalloc(nvec * sizeof(struct vfio_pci_irq_ctx), GFP_KERNEL);
>> + vdev->ctx = kcalloc(nvec, sizeof(struct vfio_pci_irq_ctx), GFP_KERNEL);
>> if (!vdev->ctx)
>> return -ENOMEM;
>
> Unrelated change?
I put it in this patch because kcalloc checks against integer overflow
and part of the cause of the issue I identified last time was caused
by the fact that nvec * sizeof(struct vfio_pci_irq_ctx) could be made
to overflow. That integer overflow is no longer possible with the
other changes I've made in this patch, but I figured it would be a
good protection from other integer issues as well. Should have called
that out more clearly.
--
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
