[PATCH v2 0/2] hw/block/nvme: coverity fixes

[PATCH v2 0/2] hw/block/nvme: coverity fixes

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

Fix two issues reported by coverity (CID 1451080 and 1451082).

v2:
  - replace [2/2] with a fix for the bad reference counting noticed by
    Max

Klaus Jensen (2):
  hw/block/nvme: fix resource leak in nvme_dif_rw
  hw/block/nvme: fix ref counting in nvme_format_ns

 hw/block/nvme-dif.c |  2 +-
 hw/block/nvme.c     | 10 ++++++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

-- 
2.31.0

[PATCH v2 1/2] hw/block/nvme: fix resource leak in nvme_dif_rw

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

If nvme_map_dptr() fails, nvme_dif_rw() will leak the bounce context.
Fix this by using the same error handling as everywhere else in the
function.

Reported-by: Coverity (CID 1451080)
Fixes: 146f720c5563 ("hw/block/nvme: end-to-end data protection")
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/block/nvme-dif.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/nvme-dif.c b/hw/block/nvme-dif.c
index 2038d724bda5..e6f04faafb5f 100644
--- a/hw/block/nvme-dif.c
+++ b/hw/block/nvme-dif.c
@@ -432,7 +432,7 @@ uint16_t nvme_dif_rw(NvmeCtrl *n, NvmeRequest *req)
 
     status = nvme_map_dptr(n, &req->sg, mapped_len, &req->cmd);
     if (status) {
-        return status;
+        goto err;
     }
 
     ctx->data.bounce = g_malloc(len);
-- 
2.31.0

[PATCH v2 2/2] hw/block/nvme: fix ref counting in nvme_format_ns

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

Max noticed that since blk_aio_pwrite_zeroes() may invoke the callback
before returning, the callbacks will never see *count == 0 and thus
never free the count variable or decrement num_formats causing a CQE to
never be posted.

Coverity (CID 1451082) also picked up on the fact that count would not
be free'ed if the namespace was of zero size.

Fix both of these issues by explicitly checking *count and finalize for
the given namespace if --(*count) is zero. Enqueing a CQE if there are
no AIOs outstanding after this case is already handled by nvme_format()
by inspecting *num_formats.

Reported-by: Max Reitz <mreitz@redhat.com>
Reported-by: Coverity (CID 1451082)
Fixes: dc04d25e2f3f ("hw/block/nvme: add support for the format nvm command")
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/block/nvme.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 6842b01ab58b..c54ec3c9523c 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -5009,9 +5009,15 @@ static uint16_t nvme_format_ns(NvmeCtrl *n, NvmeNamespace *ns, uint8_t lbaf,
 
     }
 
-    (*count)--;
+    if (--(*count)) {
+        return NVME_NO_COMPLETE;
+    }
 
-    return NVME_NO_COMPLETE;
+    g_free(count);
+    ns->status = 0x0;
+    (*num_formats)--;
+
+    return NVME_SUCCESS;
 }
 
 static uint16_t nvme_format(NvmeCtrl *n, NvmeRequest *req)
-- 
2.31.0

Re: [PATCH v2 0/2] hw/block/nvme: coverity fixes

[Klaus Jensen]

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

On Mar 22 13:09, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Fix two issues reported by coverity (CID 1451080 and 1451082).
> 
> v2:
>   - replace [2/2] with a fix for the bad reference counting noticed by
>     Max
> 
> Klaus Jensen (2):
>   hw/block/nvme: fix resource leak in nvme_dif_rw
>   hw/block/nvme: fix ref counting in nvme_format_ns
> 
>  hw/block/nvme-dif.c |  2 +-
>  hw/block/nvme.c     | 10 ++++++++--
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 

Gentle ping on this so Peter can get the coverity issues off the list ;)

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

Re: [PATCH v2 1/2] hw/block/nvme: fix resource leak in nvme_dif_rw

[Gollu Appalanaidu]

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

On Mon, Mar 22, 2021 at 01:09:43PM +0100, Klaus Jensen wrote:
>From: Klaus Jensen <k.jensen@samsung.com>
>
>If nvme_map_dptr() fails, nvme_dif_rw() will leak the bounce context.
>Fix this by using the same error handling as everywhere else in the
>function.
>
>Reported-by: Coverity (CID 1451080)
>Fixes: 146f720c5563 ("hw/block/nvme: end-to-end data protection")
>Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>---
> hw/block/nvme-dif.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/hw/block/nvme-dif.c b/hw/block/nvme-dif.c
>index 2038d724bda5..e6f04faafb5f 100644
>--- a/hw/block/nvme-dif.c
>+++ b/hw/block/nvme-dif.c
>@@ -432,7 +432,7 @@ uint16_t nvme_dif_rw(NvmeCtrl *n, NvmeRequest *req)
>
>     status = nvme_map_dptr(n, &req->sg, mapped_len, &req->cmd);
>     if (status) {
>-        return status;
>+        goto err;
Looks good to me.
>     }
>
>     ctx->data.bounce = g_malloc(len);
>-- 
Reviewed-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
>2.31.0
>
>

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 2/2] hw/block/nvme: fix ref counting in nvme_format_ns

[Gollu Appalanaidu]

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

On Mon, Mar 22, 2021 at 01:09:44PM +0100, Klaus Jensen wrote:
>From: Klaus Jensen <k.jensen@samsung.com>
>
>Max noticed that since blk_aio_pwrite_zeroes() may invoke the callback
>before returning, the callbacks will never see *count == 0 and thus
>never free the count variable or decrement num_formats causing a CQE to
>never be posted.
>
>Coverity (CID 1451082) also picked up on the fact that count would not
>be free'ed if the namespace was of zero size.
>
>Fix both of these issues by explicitly checking *count and finalize for
>the given namespace if --(*count) is zero. Enqueing a CQE if there are
>no AIOs outstanding after this case is already handled by nvme_format()
>by inspecting *num_formats.
>
>Reported-by: Max Reitz <mreitz@redhat.com>
>Reported-by: Coverity (CID 1451082)
>Fixes: dc04d25e2f3f ("hw/block/nvme: add support for the format nvm command")
>Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
>---
> hw/block/nvme.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
>diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>index 6842b01ab58b..c54ec3c9523c 100644
>--- a/hw/block/nvme.c
>+++ b/hw/block/nvme.c
>@@ -5009,9 +5009,15 @@ static uint16_t nvme_format_ns(NvmeCtrl *n, NvmeNamespace *ns, uint8_t lbaf,
>
>     }
>
>-    (*count)--;
>+    if (--(*count)) {
>+        return NVME_NO_COMPLETE;
>+    }
>
>-    return NVME_NO_COMPLETE;
>+    g_free(count);
>+    ns->status = 0x0;
>+    (*num_formats)--;
>+
>+    return NVME_SUCCESS;
> }
Reviewed-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
>
> static uint16_t nvme_format(NvmeCtrl *n, NvmeRequest *req)
>-- 
>2.31.0
>
>

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]