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

[Daniel Henrique Barboza]

On 7/20/22 10:16, Greg Kurz wrote:
> On Sat,  9 Apr 2022 17:08:56 -0300
> Daniel Henrique Barboza <danielhb413@gmail.com> 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 (e.g. a bug in another part of the code).
> > 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 | 18 ++++++++++++++----
> >   1 file changed, 14 insertions(+), 4 deletions(-)
>
> LGTM
>
> Reviewed-by: Greg Kurz <groug@kaod.org>


Thanks Greg! I'll queue this up as an eligible fix for the soft freeze.


Daniel


> > diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> > index c4c97da5de..04a64cada3 100644
> > --- a/hw/ppc/spapr_nvdimm.c
> > +++ b/hw/ppc/spapr_nvdimm.c
> > @@ -447,9 +447,15 @@ 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;
> > +
> > +    g_assert(drc != NULL);
> > +
> > +    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 +481,11 @@ 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;
> > +
> > +    g_assert(drc != NULL);
> > +
> > +    s_nvdimm = SPAPR_NVDIMM(drc->dev);
> >   
> >       state->hcall_ret = hcall_ret;
> >       QLIST_REMOVE(state, node);