From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756864AbcHCNOb (ORCPT ); Wed, 3 Aug 2016 09:14:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44946 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754757AbcHCNOY (ORCPT ); Wed, 3 Aug 2016 09:14:24 -0400 Subject: Re: [PATCH] vfio/pci: Fix NULL pointer oops in error interrupt setup handling To: Alex Williamson References: <20160802195924.18621.26365.stgit@gimli.home> Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org From: Auger Eric Message-ID: <59fd3001-a822-ba05-f4bb-5d5d4f826eee@redhat.com> Date: Wed, 3 Aug 2016 15:06:29 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: <20160802195924.18621.26365.stgit@gimli.home> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Wed, 03 Aug 2016 13:06:32 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > Reported-by: Chris Thompson > 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 Thanks Eric