Re: [PATCH for-7.1 1/1] hw/ppc: check if spapr_drc_index() returns NULL in spapr_nvdimm.c

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3005 bytes --]

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);

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]