Hi Alex,
On 02/08/2016 22:00, Alex Williamson wrote:
> There are multiple cases in vfio_pci_set_ctx_trigger_single() where
> we assume we can safely read from our data pointer without actually
> checking whether the user has passed any data via the count field.
> VFIO_IRQ_SET_DATA_NONE in particular is entirely broken since we
> attempt to pull an int32_t file descriptor out before even checking
> the data type. The other data types assume the data pointer contains
> one element of their type as well.
>
> In part this is good news because we were previously restricted from
> doing much sanitization of parameters because it was missed in the
> past and we didn't want to break existing users. Clearly DATA_NONE
> is completely broken, so it must not have any users and we can fix
> it up completely. For DATA_BOOL and DATA_EVENTFD, we'll just
> protect ourselves, returning error when count is zero since we
> previously would have oopsed.
>
> Signed-off-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
> Reported-by: Chris Thompson <the_cartographer@xxxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> ---
> drivers/vfio/pci/vfio_pci_intrs.c | 85 +++++++++++++++++++++----------------
> 1 file changed, 49 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
> index 15ecfc9..152b438 100644
> --- a/drivers/vfio/pci/vfio_pci_intrs.c
> +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> @@ -564,67 +564,80 @@ static int vfio_pci_set_msi_trigger(struct vfio_pci_device *vdev,
> }
>
> static int vfio_pci_set_ctx_trigger_single(struct eventfd_ctx **ctx,
> - uint32_t flags, void *data)
> + unsigned int count, uint32_t flags,
> + void *data)
> {
> - int32_t fd = *(int32_t *)data;
> -
> - if (!(flags & VFIO_IRQ_SET_DATA_TYPE_MASK))
> - return -EINVAL;
> -
> /* DATA_NONE/DATA_BOOL enables loopback testing */
> if (flags & VFIO_IRQ_SET_DATA_NONE) {
> - if (*ctx)
> - eventfd_signal(*ctx, 1);
> - return 0;
> + if (*ctx) {
> + if (count) {
> + eventfd_signal(*ctx, 1);
> + } else {
> + eventfd_ctx_put(*ctx);
> + *ctx = NULL;
> + }
> + return 0;
> + }
> } else if (flags & VFIO_IRQ_SET_DATA_BOOL) {
> - uint8_t trigger = *(uint8_t *)data;
> + uint8_t trigger;
> +
> + if (!count)
> + return -EINVAL;
> +
> + trigger = *(uint8_t *)data;
> if (trigger && *ctx)
> eventfd_signal(*ctx, 1);
> - return 0;
> - }
>
> - /* Handle SET_DATA_EVENTFD */
> - if (fd == -1) {
> - if (*ctx)
> - eventfd_ctx_put(*ctx);
> - *ctx = NULL;
> return 0;
> - } else if (fd >= 0) {
> - struct eventfd_ctx *efdctx;
> - efdctx = eventfd_ctx_fdget(fd);
> - if (IS_ERR(efdctx))
> - return PTR_ERR(efdctx);
> - if (*ctx)
> - eventfd_ctx_put(*ctx);
> - *ctx = efdctx;
> + } else if (flags & VFIO_IRQ_SET_DATA_EVENTFD) {
> + int32_t fd;
> +
> + if (!count)
> + return -EINVAL;
> +
> + fd = *(int32_t *)data;
> + if (fd == -1) {
> + if (*ctx)
> + eventfd_ctx_put(*ctx);
> + *ctx = NULL;
> + } else if (fd >= 0) {
> + struct eventfd_ctx *efdctx;
> +
> + efdctx = eventfd_ctx_fdget(fd);
> + if (IS_ERR(efdctx))
> + return PTR_ERR(efdctx);
> +
> + if (*ctx)
> + eventfd_ctx_put(*ctx);
> +
> + *ctx = efdctx;
> + }
> return 0;
> - } else
> - return -EINVAL;
> + }
> +
> + return -EINVAL;
> }
>
> static int vfio_pci_set_err_trigger(struct vfio_pci_device *vdev,
> unsigned index, unsigned start,
> unsigned count, uint32_t flags, void *data)
> {
> - if (index != VFIO_PCI_ERR_IRQ_INDEX)
> + if (index != VFIO_PCI_ERR_IRQ_INDEX || start != 0 || count > 1)
> return -EINVAL;
>
> - /*
> - * We should sanitize start & count, but that wasn't caught
> - * originally, so this IRQ index must forever ignore them :-(
> - */
> -
> - return vfio_pci_set_ctx_trigger_single(&vdev->err_trigger, flags, data);
> + return vfio_pci_set_ctx_trigger_single(&vdev->err_trigger,
> + count, flags, data);
> }
>
> static int vfio_pci_set_req_trigger(struct vfio_pci_device *vdev,
> unsigned index, unsigned start,
> unsigned count, uint32_t flags, void *data)
> {
> - if (index != VFIO_PCI_REQ_IRQ_INDEX || start != 0 || count != 1)
> + if (index != VFIO_PCI_REQ_IRQ_INDEX || start != 0 || count > 1)
> return -EINVAL;
>
> - return vfio_pci_set_ctx_trigger_single(&vdev->req_trigger, flags, data);
> + return vfio_pci_set_ctx_trigger_single(&vdev->req_trigger,
> + count, flags, data);
> }
>
> int vfio_pci_set_irqs_ioctl(struct vfio_pci_device *vdev, uint32_t flags,
>
> --
> 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
>
Looks good to me.
Reviewed-by: Eric Auger <eric.auger@xxxxxxxxxx>
Thanks
Eric
--
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
