| Message ID | 20220405203416.75952-2-danielhb413@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Coverity fixes in hw/ppc/spapr_nvdimm.c | expand |
On Tue, Apr 05, 2022 at 05:34:16PM -0300, Daniel Henrique Barboza wrote: > spapr_nvdimm_flush_completion_cb() and flush_worker_cb() are using the > DRC object returned by spapr_drc_index() without checking it for NULL. > In this case we would be dereferencing a NULL pointer when doing > SPAPR_NVDIMM(drc->dev) and PC_DIMM(drc->dev). > > This can happen if, during a scm_flush(), the DRC object is wrongly > freed/released by another part of the code (i.e. hotunplug the device). > spapr_drc_index() would then return NULL in the callbacks. I'm not entirely clear if you're saying this would only happen due to a bug elsewhere in the code, or if there's some unusual race case or set of guest/user actions that could trigger this. > > Fixes: Coverity CID 1487108, 1487178 > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> > --- > hw/ppc/spapr_nvdimm.c | 26 ++++++++++++++++++++++---- > 1 file changed, 22 insertions(+), 4 deletions(-) > > diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c > index c4c97da5de..e92d92fdae 100644 > --- a/hw/ppc/spapr_nvdimm.c > +++ b/hw/ppc/spapr_nvdimm.c > @@ -447,9 +447,19 @@ static int flush_worker_cb(void *opaque) > { > SpaprNVDIMMDeviceFlushState *state = opaque; > SpaprDrc *drc = spapr_drc_by_index(state->drcidx); > - PCDIMMDevice *dimm = PC_DIMM(drc->dev); > - HostMemoryBackend *backend = MEMORY_BACKEND(dimm->hostmem); > - int backend_fd = memory_region_get_fd(&backend->mr); > + PCDIMMDevice *dimm; > + HostMemoryBackend *backend; > + int backend_fd; > + > + if (!drc) { > + error_report("papr_scm: Could not find nvdimm device with DRC 0x%u", > + state->drcidx); > + return H_HARDWARE; If this does indicate a bug elswhere in qemu, this should probably be an assert rather than an H_HARDWARE. > + } > + > + dimm = PC_DIMM(drc->dev); > + backend = MEMORY_BACKEND(dimm->hostmem); > + backend_fd = memory_region_get_fd(&backend->mr); > > if (object_property_get_bool(OBJECT(backend), "pmem", NULL)) { > MemoryRegion *mr = host_memory_backend_get_memory(dimm->hostmem); > @@ -475,7 +485,15 @@ static void spapr_nvdimm_flush_completion_cb(void *opaque, int hcall_ret) > { > SpaprNVDIMMDeviceFlushState *state = opaque; > SpaprDrc *drc = spapr_drc_by_index(state->drcidx); > - SpaprNVDIMMDevice *s_nvdimm = SPAPR_NVDIMM(drc->dev); > + SpaprNVDIMMDevice *s_nvdimm; > + > + if (!drc) { > + error_report("papr_scm: Could not find nvdimm device with DRC 0x%u", > + state->drcidx); > + return; > + } > + > + s_nvdimm = SPAPR_NVDIMM(drc->dev); > > state->hcall_ret = hcall_ret; > QLIST_REMOVE(state, node);
diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c index c4c97da5de..e92d92fdae 100644 --- a/hw/ppc/spapr_nvdimm.c +++ b/hw/ppc/spapr_nvdimm.c @@ -447,9 +447,19 @@ static int flush_worker_cb(void *opaque) { SpaprNVDIMMDeviceFlushState *state = opaque; SpaprDrc *drc = spapr_drc_by_index(state->drcidx); - PCDIMMDevice *dimm = PC_DIMM(drc->dev); - HostMemoryBackend *backend = MEMORY_BACKEND(dimm->hostmem); - int backend_fd = memory_region_get_fd(&backend->mr); + PCDIMMDevice *dimm; + HostMemoryBackend *backend; + int backend_fd; + + if (!drc) { + error_report("papr_scm: Could not find nvdimm device with DRC 0x%u", + state->drcidx); + return H_HARDWARE; + } + + dimm = PC_DIMM(drc->dev); + backend = MEMORY_BACKEND(dimm->hostmem); + backend_fd = memory_region_get_fd(&backend->mr); if (object_property_get_bool(OBJECT(backend), "pmem", NULL)) { MemoryRegion *mr = host_memory_backend_get_memory(dimm->hostmem); @@ -475,7 +485,15 @@ static void spapr_nvdimm_flush_completion_cb(void *opaque, int hcall_ret) { SpaprNVDIMMDeviceFlushState *state = opaque; SpaprDrc *drc = spapr_drc_by_index(state->drcidx); - SpaprNVDIMMDevice *s_nvdimm = SPAPR_NVDIMM(drc->dev); + SpaprNVDIMMDevice *s_nvdimm; + + if (!drc) { + error_report("papr_scm: Could not find nvdimm device with DRC 0x%u", + state->drcidx); + return; + } + + s_nvdimm = SPAPR_NVDIMM(drc->dev); state->hcall_ret = hcall_ret; QLIST_REMOVE(state, node);
spapr_nvdimm_flush_completion_cb() and flush_worker_cb() are using the DRC object returned by spapr_drc_index() without checking it for NULL. In this case we would be dereferencing a NULL pointer when doing SPAPR_NVDIMM(drc->dev) and PC_DIMM(drc->dev). This can happen if, during a scm_flush(), the DRC object is wrongly freed/released by another part of the code (i.e. hotunplug the device). spapr_drc_index() would then return NULL in the callbacks. Fixes: Coverity CID 1487108, 1487178 Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- hw/ppc/spapr_nvdimm.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-)