* [PATCH v2 0/1] Coverity fixes in hw/ppc/spapr_nvdimm.c @ 2022-04-09 20:08 Daniel Henrique Barboza 2022-04-09 20:08 ` [PATCH v2 1/1] hw/ppc: check if spapr_drc_index() returns NULL in spapr_nvdimm.c Daniel Henrique Barboza ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Daniel Henrique Barboza @ 2022-04-09 20:08 UTC (permalink / raw) To: qemu-devel; +Cc: Daniel Henrique Barboza, qemu-ppc, clg, david Changes from v1: - clarified in the commit message which kind of errors we aim to prevent - changed the H_HARDWARE return to g_assert() exit - v1 link: https://lists.gnu.org/archive/html/qemu-devel/2022-04/msg00569.html Daniel Henrique Barboza (1): hw/ppc: check if spapr_drc_index() returns NULL in spapr_nvdimm.c hw/ppc/spapr_nvdimm.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) -- 2.35.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/1] hw/ppc: check if spapr_drc_index() returns NULL in spapr_nvdimm.c 2022-04-09 20:08 [PATCH v2 0/1] Coverity fixes in hw/ppc/spapr_nvdimm.c Daniel Henrique Barboza @ 2022-04-09 20:08 ` Daniel Henrique Barboza 2022-07-20 13:16 ` Greg Kurz 2022-05-04 18:51 ` [PATCH v2 0/1] Coverity fixes in hw/ppc/spapr_nvdimm.c Daniel Henrique Barboza 2022-07-20 12:34 ` Peter Maydell 2 siblings, 1 reply; 7+ messages in thread From: Daniel Henrique Barboza @ 2022-04-09 20:08 UTC (permalink / raw) To: qemu-devel; +Cc: Daniel Henrique Barboza, qemu-ppc, clg, david 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(-) 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); -- 2.35.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/1] hw/ppc: check if spapr_drc_index() returns NULL in spapr_nvdimm.c 2022-04-09 20:08 ` [PATCH v2 1/1] hw/ppc: check if spapr_drc_index() returns NULL in spapr_nvdimm.c Daniel Henrique Barboza @ 2022-07-20 13:16 ` Greg Kurz 2022-07-20 13:58 ` Daniel Henrique Barboza 0 siblings, 1 reply; 7+ messages in thread From: Greg Kurz @ 2022-07-20 13:16 UTC (permalink / raw) To: Daniel Henrique Barboza; +Cc: qemu-devel, qemu-ppc, clg, david 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> > 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); ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/1] hw/ppc: check if spapr_drc_index() returns NULL in spapr_nvdimm.c 2022-07-20 13:16 ` Greg Kurz @ 2022-07-20 13:58 ` Daniel Henrique Barboza 0 siblings, 0 replies; 7+ messages in thread From: Daniel Henrique Barboza @ 2022-07-20 13:58 UTC (permalink / raw) To: Greg Kurz; +Cc: qemu-devel, qemu-ppc, clg, david 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); > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 0/1] Coverity fixes in hw/ppc/spapr_nvdimm.c 2022-04-09 20:08 [PATCH v2 0/1] Coverity fixes in hw/ppc/spapr_nvdimm.c Daniel Henrique Barboza 2022-04-09 20:08 ` [PATCH v2 1/1] hw/ppc: check if spapr_drc_index() returns NULL in spapr_nvdimm.c Daniel Henrique Barboza @ 2022-05-04 18:51 ` Daniel Henrique Barboza 2022-07-20 12:34 ` Peter Maydell 2 siblings, 0 replies; 7+ messages in thread From: Daniel Henrique Barboza @ 2022-05-04 18:51 UTC (permalink / raw) To: qemu-devel; +Cc: qemu-ppc, david, clg Ping On 4/9/22 17:08, Daniel Henrique Barboza wrote: > Changes from v1: > - clarified in the commit message which kind of errors we aim to prevent > - changed the H_HARDWARE return to g_assert() exit > - v1 link: https://lists.gnu.org/archive/html/qemu-devel/2022-04/msg00569.html > > Daniel Henrique Barboza (1): > hw/ppc: check if spapr_drc_index() returns NULL in spapr_nvdimm.c > > hw/ppc/spapr_nvdimm.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 0/1] Coverity fixes in hw/ppc/spapr_nvdimm.c 2022-04-09 20:08 [PATCH v2 0/1] Coverity fixes in hw/ppc/spapr_nvdimm.c Daniel Henrique Barboza 2022-04-09 20:08 ` [PATCH v2 1/1] hw/ppc: check if spapr_drc_index() returns NULL in spapr_nvdimm.c Daniel Henrique Barboza 2022-05-04 18:51 ` [PATCH v2 0/1] Coverity fixes in hw/ppc/spapr_nvdimm.c Daniel Henrique Barboza @ 2022-07-20 12:34 ` Peter Maydell 2022-07-20 12:55 ` Daniel Henrique Barboza 2 siblings, 1 reply; 7+ messages in thread From: Peter Maydell @ 2022-07-20 12:34 UTC (permalink / raw) To: Daniel Henrique Barboza; +Cc: qemu-devel, qemu-ppc, clg, david, Greg Kurz On Sat, 9 Apr 2022 at 21:11, Daniel Henrique Barboza <danielhb413@gmail.com> wrote: > > Changes from v1: > - clarified in the commit message which kind of errors we aim to prevent > - changed the H_HARDWARE return to g_assert() exit > - v1 link: https://lists.gnu.org/archive/html/qemu-devel/2022-04/msg00569.html > > Daniel Henrique Barboza (1): > hw/ppc: check if spapr_drc_index() returns NULL in spapr_nvdimm.c > > hw/ppc/spapr_nvdimm.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) Hi -- I was just going through old-but-unclosed Coverity issues, and it looks like this patch fixing one got lost somewhere along the line. Could somebody pick it up for the ppc tree, please? thanks -- PMM ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 0/1] Coverity fixes in hw/ppc/spapr_nvdimm.c 2022-07-20 12:34 ` Peter Maydell @ 2022-07-20 12:55 ` Daniel Henrique Barboza 0 siblings, 0 replies; 7+ messages in thread From: Daniel Henrique Barboza @ 2022-07-20 12:55 UTC (permalink / raw) To: Peter Maydell; +Cc: qemu-devel, qemu-ppc, clg, david, Greg Kurz On 7/20/22 09:34, Peter Maydell wrote: > On Sat, 9 Apr 2022 at 21:11, Daniel Henrique Barboza > <danielhb413@gmail.com> wrote: >> >> Changes from v1: >> - clarified in the commit message which kind of errors we aim to prevent >> - changed the H_HARDWARE return to g_assert() exit >> - v1 link: https://lists.gnu.org/archive/html/qemu-devel/2022-04/msg00569.html >> >> Daniel Henrique Barboza (1): >> hw/ppc: check if spapr_drc_index() returns NULL in spapr_nvdimm.c >> >> hw/ppc/spapr_nvdimm.c | 18 ++++++++++++++---- >> 1 file changed, 14 insertions(+), 4 deletions(-) > > Hi -- I was just going through old-but-unclosed Coverity issues, and > it looks like this patch fixing one got lost somewhere along the line. > Could somebody pick it up for the ppc tree, please? I can pick it for the next PR. Just need someone else to ack it. Thanks, Daniel > > thanks > -- PMM ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-07-20 14:00 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-09 20:08 [PATCH v2 0/1] Coverity fixes in hw/ppc/spapr_nvdimm.c Daniel Henrique Barboza 2022-04-09 20:08 ` [PATCH v2 1/1] hw/ppc: check if spapr_drc_index() returns NULL in spapr_nvdimm.c Daniel Henrique Barboza 2022-07-20 13:16 ` Greg Kurz 2022-07-20 13:58 ` Daniel Henrique Barboza 2022-05-04 18:51 ` [PATCH v2 0/1] Coverity fixes in hw/ppc/spapr_nvdimm.c Daniel Henrique Barboza 2022-07-20 12:34 ` Peter Maydell 2022-07-20 12:55 ` Daniel Henrique Barboza
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.