[PATCH v2 0/1] Coverity fixes in hw/ppc/spapr_nvdimm.c

[PATCH v2 0/1] Coverity fixes in hw/ppc/spapr_nvdimm.c

[Daniel Henrique Barboza]

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

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

[Daniel Henrique Barboza]

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

Re: [PATCH v2 0/1] Coverity fixes in hw/ppc/spapr_nvdimm.c

[Daniel Henrique Barboza]

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

Re: [PATCH v2 0/1] Coverity fixes in hw/ppc/spapr_nvdimm.c

[Peter Maydell]

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

Re: [PATCH v2 0/1] Coverity fixes in hw/ppc/spapr_nvdimm.c

[Daniel Henrique Barboza]

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

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

[Greg Kurz]

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

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