* [PATCH] vfio/pci: Fix integer overflow/heap memory disclosure
@ 2016-10-11 11:02 Vlad Tsyrklevich
2016-10-11 16:10 ` Alex Williamson
0 siblings, 1 reply; 10+ messages in thread
From: Vlad Tsyrklevich @ 2016-10-11 11:02 UTC (permalink / raw)
To: kvm; +Cc: alex.williamson, Vlad Tsyrklevich
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@tsyrklevich.net>
---
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;
@@ -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.
--
2.7.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] vfio/pci: Fix integer overflow/heap memory disclosure
2016-10-11 11:02 [PATCH] vfio/pci: Fix integer overflow/heap memory disclosure Vlad Tsyrklevich
@ 2016-10-11 16:10 ` Alex Williamson
2016-10-11 20:04 ` Vlad Tsyrklevich
0 siblings, 1 reply; 10+ messages in thread
From: Alex Williamson @ 2016-10-11 16:10 UTC (permalink / raw)
To: Vlad Tsyrklevich; +Cc: kvm
On Tue, 11 Oct 2016 13:02:19 +0200
Vlad Tsyrklevich <vlad@tsyrklevich.net> 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@tsyrklevich.net>
>
> ---
> 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.
> @@ -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.
I'm not seeing that there's an issue on either path here, please
elaborate if you still disagree. Thanks,
Alex
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] vfio/pci: Fix integer overflow/heap memory disclosure
2016-10-11 16:10 ` Alex Williamson
@ 2016-10-11 20:04 ` Vlad Tsyrklevich
2016-10-11 22:39 ` Alex Williamson
0 siblings, 1 reply; 10+ messages in thread
From: Vlad Tsyrklevich @ 2016-10-11 20:04 UTC (permalink / raw)
To: Alex Williamson; +Cc: kvm
On Tue, Oct 11, 2016 at 6:10 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Tue, 11 Oct 2016 13:02:19 +0200
> Vlad Tsyrklevich <vlad@tsyrklevich.net> 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@tsyrklevich.net>
>>
>> ---
>> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] vfio/pci: Fix integer overflow/heap memory disclosure
2016-10-11 20:04 ` Vlad Tsyrklevich
@ 2016-10-11 22:39 ` Alex Williamson
2016-10-12 7:53 ` [PATCH v2] vfio/pci: Fix integer overflows, bitmask check Vlad Tsyrklevich
0 siblings, 1 reply; 10+ messages in thread
From: Alex Williamson @ 2016-10-11 22:39 UTC (permalink / raw)
To: Vlad Tsyrklevich; +Cc: kvm
On Tue, 11 Oct 2016 22:04:08 +0200
Vlad Tsyrklevich <vlad@tsyrklevich.net> wrote:
> On Tue, Oct 11, 2016 at 6:10 PM, Alex Williamson
> <alex.williamson@redhat.com> wrote:
> > On Tue, 11 Oct 2016 13:02:19 +0200
> > Vlad Tsyrklevich <vlad@tsyrklevich.net> 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@tsyrklevich.net>
> >>
> >> ---
> >> 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.
Yes, let's sanitize regardless of the data type/presence flags. We can
still use vfio_pci_get_irq_count() to be much more precise about it.
> 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?
Seems plausible, we should make sure that both
VFIO_IRQ_SET_DATA_TYPE_MASK and VFIO_IRQ_SET_ACTION_TYPE_MASK each only
have a single bit set.
> >> @@ -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.
They do, patch away. Thanks!
Alex
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2] vfio/pci: Fix integer overflows, bitmask check
2016-10-11 22:39 ` Alex Williamson
@ 2016-10-12 7:53 ` Vlad Tsyrklevich
2016-10-12 14:51 ` Alex Williamson
0 siblings, 1 reply; 10+ messages in thread
From: Vlad Tsyrklevich @ 2016-10-12 7:53 UTC (permalink / raw)
To: kvm; +Cc: alex.williamson, Vlad Tsyrklevich
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@tsyrklevich.net>
---
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);
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;
--
2.7.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] vfio/pci: Fix integer overflows, bitmask check
2016-10-12 7:53 ` [PATCH v2] vfio/pci: Fix integer overflows, bitmask check Vlad Tsyrklevich
@ 2016-10-12 14:51 ` Alex Williamson
2016-10-12 15:06 ` Vlad Tsyrklevich
0 siblings, 1 reply; 10+ messages in thread
From: Alex Williamson @ 2016-10-12 14:51 UTC (permalink / raw)
To: Vlad Tsyrklevich; +Cc: kvm
On Wed, 12 Oct 2016 09:53:49 +0200
Vlad Tsyrklevich <vlad@tsyrklevich.net> 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@tsyrklevich.net>
> ---
> 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?
>
> 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?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] vfio/pci: Fix integer overflows, bitmask check
2016-10-12 14:51 ` Alex Williamson
@ 2016-10-12 15:06 ` Vlad Tsyrklevich
2016-10-12 15:39 ` Alex Williamson
0 siblings, 1 reply; 10+ messages in thread
From: Vlad Tsyrklevich @ 2016-10-12 15:06 UTC (permalink / raw)
To: Alex Williamson; +Cc: kvm
On Wed, Oct 12, 2016 at 4:51 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> On Wed, 12 Oct 2016 09:53:49 +0200
> Vlad Tsyrklevich <vlad@tsyrklevich.net> 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@tsyrklevich.net>
>> ---
>> 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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] vfio/pci: Fix integer overflows, bitmask check
2016-10-12 15:06 ` Vlad Tsyrklevich
@ 2016-10-12 15:39 ` Alex Williamson
2016-10-12 16:51 ` [PATCH v3] " Vlad Tsyrklevich
0 siblings, 1 reply; 10+ messages in thread
From: Alex Williamson @ 2016-10-12 15:39 UTC (permalink / raw)
To: Vlad Tsyrklevich; +Cc: kvm
On Wed, 12 Oct 2016 17:06:15 +0200
Vlad Tsyrklevich <vlad@tsyrklevich.net> wrote:
> On Wed, Oct 12, 2016 at 4:51 PM, Alex Williamson
> <alex.williamson@redhat.com> wrote:
> > On Wed, 12 Oct 2016 09:53:49 +0200
> > Vlad Tsyrklevich <vlad@tsyrklevich.net> 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@tsyrklevich.net>
> >> ---
> >> 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.
It feels like pulling vfio_pci_get_irq_count() up allows us to more
strictly validate the range, not just sanitize it for overflow. Maybe
that leads to some redundancy later, but we can fix that with future
patches if it seems necessary.
> >>
> >> 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.
Ah, I see. Yes, please note this in the commit log. Thanks,
Alex
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3] vfio/pci: Fix integer overflows, bitmask check
2016-10-12 15:39 ` Alex Williamson
@ 2016-10-12 16:51 ` Vlad Tsyrklevich
2016-10-26 21:19 ` Alex Williamson
0 siblings, 1 reply; 10+ messages in thread
From: Vlad Tsyrklevich @ 2016-10-12 16:51 UTC (permalink / raw)
To: kvm; +Cc: alex.williamson, Vlad Tsyrklevich
The VFIO_DEVICE_SET_IRQS ioctl did not sufficiently sanitize
user-supplied integers, potentially allowing memory corruption. This
patch adds appropriate integer overflow checks, checks the range bounds
for VFIO_IRQ_SET_DATA_NONE, and also verifies that only 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().
Furthermore, a kzalloc is changed to a kcalloc because the use of a
kzalloc with an integer multiplication allowed an integer overflow
condition to be reached without this patch. kcalloc checks for overflow
and should prevent a similar occurrence.
Signed-off-by: Vlad Tsyrklevich <vlad@tsyrklevich.net>
---
drivers/vfio/pci/vfio_pci.c | 33 +++++++++++++++++++++------------
drivers/vfio/pci/vfio_pci_intrs.c | 2 +-
2 files changed, 22 insertions(+), 13 deletions(-)
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index d624a52..031bc08 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -829,8 +829,9 @@ 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;
+ int max, ret = 0;
minsz = offsetofend(struct vfio_irq_set, count);
@@ -838,23 +839,31 @@ 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);
+ max = vfio_pci_get_irq_count(vdev, hdr.index);
+ if (hdr.start >= max || hdr.start + hdr.count > max)
+ 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;
+ 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.argsz - minsz < hdr.count * size ||
- hdr.start >= max || hdr.start + hdr.count > max)
+ if (size) {
+ if (hdr.argsz - minsz < hdr.count * size)
return -EINVAL;
data = memdup_user((void __user *)(arg + minsz),
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;
--
2.7.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3] vfio/pci: Fix integer overflows, bitmask check
2016-10-12 16:51 ` [PATCH v3] " Vlad Tsyrklevich
@ 2016-10-26 21:19 ` Alex Williamson
0 siblings, 0 replies; 10+ messages in thread
From: Alex Williamson @ 2016-10-26 21:19 UTC (permalink / raw)
To: Vlad Tsyrklevich; +Cc: kvm
On Wed, 12 Oct 2016 18:51:24 +0200
Vlad Tsyrklevich <vlad@tsyrklevich.net> wrote:
> The VFIO_DEVICE_SET_IRQS ioctl did not sufficiently sanitize
> user-supplied integers, potentially allowing memory corruption. This
> patch adds appropriate integer overflow checks, checks the range bounds
> for VFIO_IRQ_SET_DATA_NONE, and also verifies that only 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().
>
> Furthermore, a kzalloc is changed to a kcalloc because the use of a
> kzalloc with an integer multiplication allowed an integer overflow
> condition to be reached without this patch. kcalloc checks for overflow
> and should prevent a similar occurrence.
>
> Signed-off-by: Vlad Tsyrklevich <vlad@tsyrklevich.net>
> ---
> drivers/vfio/pci/vfio_pci.c | 33 +++++++++++++++++++++------------
> drivers/vfio/pci/vfio_pci_intrs.c | 2 +-
> 2 files changed, 22 insertions(+), 13 deletions(-)
I've applied this to my for-linus branch for v4.9, thanks,
Alex
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index d624a52..031bc08 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -829,8 +829,9 @@ 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;
> + int max, ret = 0;
>
> minsz = offsetofend(struct vfio_irq_set, count);
>
> @@ -838,23 +839,31 @@ 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);
> + max = vfio_pci_get_irq_count(vdev, hdr.index);
> + if (hdr.start >= max || hdr.start + hdr.count > max)
> + 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;
> + 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.argsz - minsz < hdr.count * size ||
> - hdr.start >= max || hdr.start + hdr.count > max)
> + if (size) {
> + if (hdr.argsz - minsz < hdr.count * size)
> return -EINVAL;
>
> data = memdup_user((void __user *)(arg + minsz),
> 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;
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-10-26 21:19 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-11 11:02 [PATCH] vfio/pci: Fix integer overflow/heap memory disclosure Vlad Tsyrklevich
2016-10-11 16:10 ` Alex Williamson
2016-10-11 20:04 ` Vlad Tsyrklevich
2016-10-11 22:39 ` Alex Williamson
2016-10-12 7:53 ` [PATCH v2] vfio/pci: Fix integer overflows, bitmask check Vlad Tsyrklevich
2016-10-12 14:51 ` Alex Williamson
2016-10-12 15:06 ` Vlad Tsyrklevich
2016-10-12 15:39 ` Alex Williamson
2016-10-12 16:51 ` [PATCH v3] " Vlad Tsyrklevich
2016-10-26 21:19 ` Alex Williamson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.