| Message ID | 20180301075428.17320-2-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,1/1] block/nvme: fix Coverity reports | expand |
On 1 March 2018 at 07:54, Fam Zheng <famz@redhat.com> wrote: > From: Paolo Bonzini <pbonzini@redhat.com> > > 1) string not null terminated in sysfs_find_group_file > > 2) NULL pointer dereference and dead local variable in nvme_init. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Fam Zheng <famz@redhat.com> > > Message-Id: <20180213015240.9352-1-famz@redhat.com> > Signed-off-by: Fam Zheng <famz@redhat.com> Hi. It looks like Coverity still doesn't like the error-exit handling in nvme_init() (CID 1385847): > --- > block/nvme.c | 10 +++++++--- > util/vfio-helpers.c | 2 +- > 2 files changed, 8 insertions(+), 4 deletions(-) > > diff --git a/block/nvme.c b/block/nvme.c > index 10bffbbf2f..75078022f6 100644 > --- a/block/nvme.c > +++ b/block/nvme.c > @@ -645,7 +645,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace, > aio_set_event_notifier(bdrv_get_aio_context(bs), &s->irq_notifier, > false, nvme_handle_event, nvme_poll_cb); > > - nvme_identify(bs, namespace, errp); > + nvme_identify(bs, namespace, &local_err); > if (local_err) { > error_propagate(errp, local_err); > ret = -EIO; > @@ -666,8 +666,12 @@ fail_queue: > nvme_free_queue_pair(bs, s->queues[0]); > fail: > g_free(s->queues); > - qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE); > - qemu_vfio_close(s->vfio); > + if (s->regs) { > + qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE); We can get here with s->vfio being NULL and s->regs being uninitialized, but qemu_vfio_pci_unmap_bar() will unconditionally dereference s->vfio. If you want to write the error path like this I think you need to initialize s->regs = NULL before we do the qemu_vfio_open_pci() call and error-check. > + } > + if (s->vfio) { > + qemu_vfio_close(s->vfio); > + } > event_notifier_cleanup(&s->irq_notifier); > return ret; > } thanks -- PMM
On Fri, 05/18 19:01, Peter Maydell wrote: > On 1 March 2018 at 07:54, Fam Zheng <famz@redhat.com> wrote: > > From: Paolo Bonzini <pbonzini@redhat.com> > > > > 1) string not null terminated in sysfs_find_group_file > > > > 2) NULL pointer dereference and dead local variable in nvme_init. > > > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > Signed-off-by: Fam Zheng <famz@redhat.com> > > > > Message-Id: <20180213015240.9352-1-famz@redhat.com> > > Signed-off-by: Fam Zheng <famz@redhat.com> > > Hi. It looks like Coverity still doesn't like the error-exit > handling in nvme_init() (CID 1385847): > > > --- > > block/nvme.c | 10 +++++++--- > > util/vfio-helpers.c | 2 +- > > 2 files changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/block/nvme.c b/block/nvme.c > > index 10bffbbf2f..75078022f6 100644 > > --- a/block/nvme.c > > +++ b/block/nvme.c > > @@ -645,7 +645,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace, > > aio_set_event_notifier(bdrv_get_aio_context(bs), &s->irq_notifier, > > false, nvme_handle_event, nvme_poll_cb); > > > > - nvme_identify(bs, namespace, errp); > > + nvme_identify(bs, namespace, &local_err); > > if (local_err) { > > error_propagate(errp, local_err); > > ret = -EIO; > > @@ -666,8 +666,12 @@ fail_queue: > > nvme_free_queue_pair(bs, s->queues[0]); > > fail: > > g_free(s->queues); > > - qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE); > > - qemu_vfio_close(s->vfio); > > + if (s->regs) { > > + qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE); > > We can get here with s->vfio being NULL and s->regs being > uninitialized, but qemu_vfio_pci_unmap_bar() will unconditionally > dereference s->vfio. > > If you want to write the error path like this I think you need > to initialize s->regs = NULL before we do the qemu_vfio_open_pci() > call and error-check. Block layer uses g_malloc0 to allocate *s, so we expect s->regs is NULL to begin with. But I can send a patch to explicitly initialize it to NULL to make the code simpler to reason. Fam > > > + } > > + if (s->vfio) { > > + qemu_vfio_close(s->vfio); > > + } > > event_notifier_cleanup(&s->irq_notifier); > > return ret; > > } > > thanks > -- PMM
diff --git a/block/nvme.c b/block/nvme.c index 10bffbbf2f..75078022f6 100644 --- a/block/nvme.c +++ b/block/nvme.c @@ -645,7 +645,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace, aio_set_event_notifier(bdrv_get_aio_context(bs), &s->irq_notifier, false, nvme_handle_event, nvme_poll_cb); - nvme_identify(bs, namespace, errp); + nvme_identify(bs, namespace, &local_err); if (local_err) { error_propagate(errp, local_err); ret = -EIO; @@ -666,8 +666,12 @@ fail_queue: nvme_free_queue_pair(bs, s->queues[0]); fail: g_free(s->queues); - qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE); - qemu_vfio_close(s->vfio); + if (s->regs) { + qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE); + } + if (s->vfio) { + qemu_vfio_close(s->vfio); + } event_notifier_cleanup(&s->irq_notifier); return ret; } diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c index f478b68400..006674c916 100644 --- a/util/vfio-helpers.c +++ b/util/vfio-helpers.c @@ -104,7 +104,7 @@ static char *sysfs_find_group_file(const char *device, Error **errp) char *path = NULL; sysfs_link = g_strdup_printf("/sys/bus/pci/devices/%s/iommu_group", device); - sysfs_group = g_malloc(PATH_MAX); + sysfs_group = g_malloc0(PATH_MAX); if (readlink(sysfs_link, sysfs_group, PATH_MAX - 1) == -1) { error_setg_errno(errp, errno, "Failed to find iommu group sysfs path"); goto out;