Re: [PATCH] vfio/pci: Fix NULL pointer oops in error interrupt setup handling

[Auger Eric]

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