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 <danielhb...@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);
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
