diff mbox

vfio/pci: Fix NULL pointer oops in error interrupt setup handling

Message ID 20160802195924.18621.26365.stgit@gimli.home (mailing list archive)
State New, archived
Headers show

Commit Message

Alex Williamson Aug. 2, 2016, 8 p.m. UTC 
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@redhat.com>
Reported-by: Chris Thompson <the_cartographer@hotmail.com>
Cc: stable@vger.kernel.org
---
 drivers/vfio/pci/vfio_pci_intrs.c |   85 +++++++++++++++++++++----------------
 1 file changed, 49 insertions(+), 36 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Eric Auger Aug. 3, 2016, 1:06 p.m. UTC | #1 
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@redhat.com>
> Reported-by: Chris Thompson <the_cartographer@hotmail.com>
> Cc: stable@vger.kernel.org
> ---
>  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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Looks good to me.

Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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,