| Message ID | 20170905110744.32537-1-kleber.souza@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [Trusty,SRU,CVE-2016-9083/4] vfio/pci: Fix integer overflows, bitmask check | expand |
On 05/09/17 12:07, Kleber Sacilotto de Souza wrote: > From: Vlad Tsyrklevich <vlad@tsyrklevich.net> > > CVE-2016-9083 > CVE-2016-9084 > > 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> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > (cherry picked from commit 05692d7005a364add85c6e25a6c4447ce08f913a) > Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> > --- > 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 55fe2ee771c3..05b14be1c45a 100644 > --- a/drivers/vfio/pci/vfio_pci.c > +++ b/drivers/vfio/pci/vfio_pci.c > @@ -471,8 +471,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); > > @@ -480,23 +481,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 641bc87bdb96..05b0834e26e0 100644 > --- a/drivers/vfio/pci/vfio_pci_intrs.c > +++ b/drivers/vfio/pci/vfio_pci_intrs.c > @@ -465,7 +465,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; > > Clean cherry pick, looks good to me. Acked-by: Colin Ian King <colin.king@canonical.com>
On 05.09.2017 13:07, Kleber Sacilotto de Souza wrote: > From: Vlad Tsyrklevich <vlad@tsyrklevich.net> > > CVE-2016-9083 > CVE-2016-9084 > > 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> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > (cherry picked from commit 05692d7005a364add85c6e25a6c4447ce08f913a) > Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- > 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 55fe2ee771c3..05b14be1c45a 100644 > --- a/drivers/vfio/pci/vfio_pci.c > +++ b/drivers/vfio/pci/vfio_pci.c > @@ -471,8 +471,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); > > @@ -480,23 +481,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 641bc87bdb96..05b0834e26e0 100644 > --- a/drivers/vfio/pci/vfio_pci_intrs.c > +++ b/drivers/vfio/pci/vfio_pci_intrs.c > @@ -465,7 +465,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; > >
Applied to trusty/master-next branch. Thanks.
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c index 55fe2ee771c3..05b14be1c45a 100644 --- a/drivers/vfio/pci/vfio_pci.c +++ b/drivers/vfio/pci/vfio_pci.c @@ -471,8 +471,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); @@ -480,23 +481,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 641bc87bdb96..05b0834e26e0 100644 --- a/drivers/vfio/pci/vfio_pci_intrs.c +++ b/drivers/vfio/pci/vfio_pci_intrs.c @@ -465,7 +465,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;