* [PATCH] vfio: pci: fix oops in case of vfio_msi_set_vector_signal failure @ 2016-01-29 14:43 Eric Auger 2016-01-29 21:41 ` Alex Williamson 0 siblings, 1 reply; 4+ messages in thread From: Eric Auger @ 2016-01-29 14:43 UTC (permalink / raw) To: eric.auger, eric.auger, alex.williamson, linux-arm-kernel, christoffer.dall Cc: patches, linux-kernel In case vfio_msi_set_vector_signal fails we tear down everything. In the tear down loop we compare int j against unsigned start. Given the arithmetic conversion I think it is converted into an unsigned and becomes 0xffffffff, leading to the loop being entered again and things turn bad when accessing vdev->msix[vector].vector. So let's use int parameters instead. Signed-off-by: Eric Auger <eric.auger@linaro.org> --- drivers/vfio/pci/vfio_pci_intrs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c index 3b3ba15..510c48d 100644 --- a/drivers/vfio/pci/vfio_pci_intrs.c +++ b/drivers/vfio/pci/vfio_pci_intrs.c @@ -374,8 +374,8 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev, return 0; } -static int vfio_msi_set_block(struct vfio_pci_device *vdev, unsigned start, - unsigned count, int32_t *fds, bool msix) +static int vfio_msi_set_block(struct vfio_pci_device *vdev, int start, + int count, int32_t *fds, bool msix) { int i, j, ret = 0; -- 1.9.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] vfio: pci: fix oops in case of vfio_msi_set_vector_signal failure 2016-01-29 14:43 [PATCH] vfio: pci: fix oops in case of vfio_msi_set_vector_signal failure Eric Auger @ 2016-01-29 21:41 ` Alex Williamson 2016-02-01 17:27 ` Eric Auger 0 siblings, 1 reply; 4+ messages in thread From: Alex Williamson @ 2016-01-29 21:41 UTC (permalink / raw) To: Eric Auger, eric.auger, linux-arm-kernel, christoffer.dall Cc: patches, linux-kernel On Fri, 2016-01-29 at 14:43 +0000, Eric Auger wrote: > In case vfio_msi_set_vector_signal fails we tear down everything. > In the tear down loop we compare int j against unsigned start. Given > the arithmetic conversion I think it is converted into an unsigned and > becomes 0xffffffff, leading to the loop being entered again and things > turn bad when accessing vdev->msix[vector].vector. So let's use int > parameters instead. > > Signed-off-by: Eric Auger <eric.auger@linaro.org> > --- > drivers/vfio/pci/vfio_pci_intrs.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c > index 3b3ba15..510c48d 100644 > --- a/drivers/vfio/pci/vfio_pci_intrs.c > +++ b/drivers/vfio/pci/vfio_pci_intrs.c > @@ -374,8 +374,8 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev, > return 0; > } > > -static int vfio_msi_set_block(struct vfio_pci_device *vdev, unsigned start, > - unsigned count, int32_t *fds, bool msix) > +static int vfio_msi_set_block(struct vfio_pci_device *vdev, int start, > + int count, int32_t *fds, bool msix) > { > int i, j, ret = 0; > Nice find, I don't think that's the only bug there though. If @start is -1 (UINT32_MAX) and @count is 1, then @j gets set to -1 in the setup and we hit the same index dereference problem. What if we did this instead: diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c index 3b3ba15..2ae84ad 100644 --- a/drivers/vfio/pci/vfio_pci_intrs.c +++ b/drivers/vfio/pci/vfio_pci_intrs.c @@ -309,14 +309,14 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev, int vector, int fd, bool msix) { struct pci_dev *pdev = vdev->pdev; - int irq = msix ? vdev->msix[vector].vector : pdev->irq + vector; - char *name = msix ? "vfio-msix" : "vfio-msi"; struct eventfd_ctx *trigger; - int ret; + int irq, ret; - if (vector >= vdev->num_ctx) + if (vector < 0 || vector >= vdev->num_ctx) return -EINVAL; + irq = msix ? vdev->msix[vector].vector : pdev->irq + vector; + if (vdev->ctx[vector].trigger) { free_irq(irq, vdev->ctx[vector].trigger); irq_bypass_unregister_producer(&vdev->ctx[vector].producer); @@ -328,8 +328,9 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev, if (fd < 0) return 0; - vdev->ctx[vector].name = kasprintf(GFP_KERNEL, "%s[%d](%s)", - name, vector, pci_name(pdev)); + vdev->ctx[vector].name = kasprintf(GFP_KERNEL, "vfio-msi%s[%d](%s)", + msix ? "x" : "", vector, + pci_name(pdev)); if (!vdev->ctx[vector].name) return -ENOMEM; @@ -379,7 +380,7 @@ static int vfio_msi_set_block(struct vfio_pci_device *vdev, unsigned start, { int i, j, ret = 0; - if (start + count > vdev->num_ctx) + if (start >= vdev->num_ctx || start + count > vdev->num_ctx) return -EINVAL; for (i = 0, j = start; i < count && !ret; i++, j++) { @@ -388,7 +389,7 @@ static int vfio_msi_set_block(struct vfio_pci_device *vdev, unsigned start, } if (ret) { - for (--j; j >= start; j--) + for (--j; j >= 0 && j >= start; j--) vfio_msi_set_vector_signal(vdev, j, -1, msix); } So we fix the problem with vfio_msi_set_vector_signal() dereferencing the array before it validates the index (even though it shouldn't be able to get there anymore), and then we do a better job of verifying start and count (comparing to num_ctx will use unsigned even though num_ctx itself is signed) and finally explicitly test the <0 case, which I suppose we could also do by casting start at that point (we know it's within the bounds of a signed integer given the previous tests). Thanks, Alex ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] vfio: pci: fix oops in case of vfio_msi_set_vector_signal failure 2016-01-29 21:41 ` Alex Williamson @ 2016-02-01 17:27 ` Eric Auger 2016-02-01 21:34 ` Alex Williamson 0 siblings, 1 reply; 4+ messages in thread From: Eric Auger @ 2016-02-01 17:27 UTC (permalink / raw) To: Alex Williamson, eric.auger, linux-arm-kernel, christoffer.dall Cc: patches, linux-kernel Hi Alex, On 01/29/2016 10:41 PM, Alex Williamson wrote: > On Fri, 2016-01-29 at 14:43 +0000, Eric Auger wrote: >> In case vfio_msi_set_vector_signal fails we tear down everything. >> In the tear down loop we compare int j against unsigned start. Given >> the arithmetic conversion I think it is converted into an unsigned and >> becomes 0xffffffff, leading to the loop being entered again and things >> turn bad when accessing vdev->msix[vector].vector. So let's use int >> parameters instead. >> >> Signed-off-by: Eric Auger <eric.auger@linaro.org> >> --- >> drivers/vfio/pci/vfio_pci_intrs.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c >> index 3b3ba15..510c48d 100644 >> --- a/drivers/vfio/pci/vfio_pci_intrs.c >> +++ b/drivers/vfio/pci/vfio_pci_intrs.c >> @@ -374,8 +374,8 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev, >> return 0; >> } >> >> -static int vfio_msi_set_block(struct vfio_pci_device *vdev, unsigned start, >> - unsigned count, int32_t *fds, bool msix) >> +static int vfio_msi_set_block(struct vfio_pci_device *vdev, int start, >> + int count, int32_t *fds, bool msix) >> { >> int i, j, ret = 0; >> > > Nice find, I don't think that's the only bug there though. If @start is > -1 (UINT32_MAX) and @count is 1, then @j gets set to -1 in the setup and > we hit the same index dereference problem. What if we did this instead: > > diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c > index 3b3ba15..2ae84ad 100644 > --- a/drivers/vfio/pci/vfio_pci_intrs.c > +++ b/drivers/vfio/pci/vfio_pci_intrs.c > @@ -309,14 +309,14 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev, > int vector, int fd, bool msix) > { > struct pci_dev *pdev = vdev->pdev; > - int irq = msix ? vdev->msix[vector].vector : pdev->irq + vector; > - char *name = msix ? "vfio-msix" : "vfio-msi"; > struct eventfd_ctx *trigger; > - int ret; > + int irq, ret; > > - if (vector >= vdev->num_ctx) > + if (vector < 0 || vector >= vdev->num_ctx) > return -EINVAL; > > + irq = msix ? vdev->msix[vector].vector : pdev->irq + vector; > + > if (vdev->ctx[vector].trigger) { > free_irq(irq, vdev->ctx[vector].trigger); > irq_bypass_unregister_producer(&vdev->ctx[vector].producer); > @@ -328,8 +328,9 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev, > if (fd < 0) > return 0; > > - vdev->ctx[vector].name = kasprintf(GFP_KERNEL, "%s[%d](%s)", > - name, vector, pci_name(pdev)); > + vdev->ctx[vector].name = kasprintf(GFP_KERNEL, "vfio-msi%s[%d](%s)", > + msix ? "x" : "", vector, > + pci_name(pdev)); > if (!vdev->ctx[vector].name) > return -ENOMEM; > > @@ -379,7 +380,7 @@ static int vfio_msi_set_block(struct vfio_pci_device *vdev, unsigned start, > { > int i, j, ret = 0; > > - if (start + count > vdev->num_ctx) > + if (start >= vdev->num_ctx || start + count > vdev->num_ctx) > return -EINVAL; > > for (i = 0, j = start; i < count && !ret; i++, j++) { > @@ -388,7 +389,7 @@ static int vfio_msi_set_block(struct vfio_pci_device *vdev, unsigned start, > } > > if (ret) { > - for (--j; j >= start; j--) > + for (--j; j >= 0 && j >= start; j--) > vfio_msi_set_vector_signal(vdev, j, -1, msix); > } > > > So we fix the problem with vfio_msi_set_vector_signal() dereferencing > the array before it validates the index (even though it shouldn't be > able to get there anymore), and then we do a better job of verifying > start and count (comparing to num_ctx will use unsigned even though > num_ctx itself is signed) and finally explicitly test the <0 case, which > I suppose we could also do by casting start at that point (we know it's > within the bounds of a signed integer given the previous tests). Yes it looks OK to me. I guess you submit? I will test it. Best Regards Eric > Thanks, > > Alex > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] vfio: pci: fix oops in case of vfio_msi_set_vector_signal failure 2016-02-01 17:27 ` Eric Auger @ 2016-02-01 21:34 ` Alex Williamson 0 siblings, 0 replies; 4+ messages in thread From: Alex Williamson @ 2016-02-01 21:34 UTC (permalink / raw) To: Eric Auger, eric.auger, linux-arm-kernel, christoffer.dall Cc: patches, linux-kernel On Mon, 2016-02-01 at 18:27 +0100, Eric Auger wrote: > Hi Alex, > On 01/29/2016 10:41 PM, Alex Williamson wrote: > > On Fri, 2016-01-29 at 14:43 +0000, Eric Auger wrote: > > > In case vfio_msi_set_vector_signal fails we tear down everything. > > > In the tear down loop we compare int j against unsigned start. Given > > > the arithmetic conversion I think it is converted into an unsigned and > > > becomes 0xffffffff, leading to the loop being entered again and things > > > turn bad when accessing vdev->msix[vector].vector. So let's use int > > > parameters instead. > > > > > > Signed-off-by: Eric Auger <eric.auger@linaro.org> > > > --- > > > drivers/vfio/pci/vfio_pci_intrs.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c > > > index 3b3ba15..510c48d 100644 > > > --- a/drivers/vfio/pci/vfio_pci_intrs.c > > > +++ b/drivers/vfio/pci/vfio_pci_intrs.c > > > @@ -374,8 +374,8 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev, > > > return 0; > > > } > > > > > > -static int vfio_msi_set_block(struct vfio_pci_device *vdev, unsigned start, > > > - unsigned count, int32_t *fds, bool msix) > > > +static int vfio_msi_set_block(struct vfio_pci_device *vdev, int start, > > > + int count, int32_t *fds, bool msix) > > > { > > > int i, j, ret = 0; > > > > > > > Nice find, I don't think that's the only bug there though. If @start is > > -1 (UINT32_MAX) and @count is 1, then @j gets set to -1 in the setup and > > we hit the same index dereference problem. What if we did this instead: > > > > diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c > > index 3b3ba15..2ae84ad 100644 > > --- a/drivers/vfio/pci/vfio_pci_intrs.c > > +++ b/drivers/vfio/pci/vfio_pci_intrs.c > > @@ -309,14 +309,14 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev, > > int vector, int fd, bool msix) > > { > > struct pci_dev *pdev = vdev->pdev; > > - int irq = msix ? vdev->msix[vector].vector : pdev->irq + vector; > > - char *name = msix ? "vfio-msix" : "vfio-msi"; > > struct eventfd_ctx *trigger; > > - int ret; > > + int irq, ret; > > > > - if (vector >= vdev->num_ctx) > > + if (vector < 0 || vector >= vdev->num_ctx) > > return -EINVAL; > > > > + irq = msix ? vdev->msix[vector].vector : pdev->irq + vector; > > + > > if (vdev->ctx[vector].trigger) { > > free_irq(irq, vdev->ctx[vector].trigger); > > irq_bypass_unregister_producer(&vdev->ctx[vector].producer); > > @@ -328,8 +328,9 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev, > > if (fd < 0) > > return 0; > > > > - vdev->ctx[vector].name = kasprintf(GFP_KERNEL, "%s[%d](%s)", > > - name, vector, pci_name(pdev)); > > + vdev->ctx[vector].name = kasprintf(GFP_KERNEL, "vfio-msi%s[%d](%s)", > > + msix ? "x" : "", vector, > > + pci_name(pdev)); > > if (!vdev->ctx[vector].name) > > return -ENOMEM; > > > > @@ -379,7 +380,7 @@ static int vfio_msi_set_block(struct vfio_pci_device *vdev, unsigned start, > > { > > int i, j, ret = 0; > > > > - if (start + count > vdev->num_ctx) > > + if (start >= vdev->num_ctx || start + count > vdev->num_ctx) > > return -EINVAL; > > > > for (i = 0, j = start; i < count && !ret; i++, j++) { > > @@ -388,7 +389,7 @@ static int vfio_msi_set_block(struct vfio_pci_device *vdev, unsigned start, > > } > > > > if (ret) { > > - for (--j; j >= start; j--) > > + for (--j; j >= 0 && j >= start; j--) > > vfio_msi_set_vector_signal(vdev, j, -1, msix); > > } > > > > > > So we fix the problem with vfio_msi_set_vector_signal() dereferencing > > the array before it validates the index (even though it shouldn't be > > able to get there anymore), and then we do a better job of verifying > > start and count (comparing to num_ctx will use unsigned even though > > num_ctx itself is signed) and finally explicitly test the <0 case, which > > I suppose we could also do by casting start at that point (we know it's > > within the bounds of a signed integer given the previous tests). > > Yes it looks OK to me. > > I guess you submit? I will test it. Yep, I'll post a real patch. Thanks, Alex ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-02-01 21:34 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-01-29 14:43 [PATCH] vfio: pci: fix oops in case of vfio_msi_set_vector_signal failure Eric Auger 2016-01-29 21:41 ` Alex Williamson 2016-02-01 17:27 ` Eric Auger 2016-02-01 21:34 ` Alex Williamson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).