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

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

[Klaus Jensen]

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

Fix three issues reported by coverity (CID 1450756, CID 1450757 and CID
1450758).

Klaus Jensen (2):
  hw/block/nvme: fix potential overflow
  hw/block/nvme: assert namespaces array indices

 hw/block/nvme-subsys.h |  2 ++
 hw/block/nvme.h        | 10 ++++++++--
 hw/block/nvme-subsys.c |  7 +++++--
 hw/block/nvme.c        |  2 +-
 4 files changed, 16 insertions(+), 5 deletions(-)

-- 
2.30.1

[PATCH 1/2] hw/block/nvme: fix potential overflow

[Klaus Jensen]

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

page_size is a uint32_t, and zasl is a uint8_t, so the expression
`page_size << zasl` is done using 32-bit arithmetic and might overflow.
Since we then compare this against a 64 bit data_size value, Coverity
complains that we might overflow unintentionally. An MDTS/ZASL value in
excess of 4GiB is probably impractical, but it is not entirely
unrealistic, so add a cast such that we handle that case properly.

Fixes: 578d914b263c ("hw/block/nvme: align zoned.zasl with mdts")
Fixes: CID 1450756
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/block/nvme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index d439e44db839..f8ad34077000 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -2188,7 +2188,7 @@ static uint16_t nvme_do_write(NvmeCtrl *n, NvmeRequest *req, bool append,
                 goto invalid;
             }
 
-            if (n->params.zasl && data_size > n->page_size << n->params.zasl) {
+            if (n->params.zasl && data_size > (uint64_t)n->page_size << n->params.zasl) {
                 trace_pci_nvme_err_zasl(data_size);
                 return NVME_INVALID_FIELD | NVME_DNR;
             }
-- 
2.30.1

[PATCH 2/2] hw/block/nvme: assert namespaces array indices

[Klaus Jensen]

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

Coverity complains about a possible memory corruption in the
nvme_ns_attach and _detach functions. While we should not (famous last
words) be able to reach this function without nsid having previously
been validated, this is still an open door for future misuse.

Make Coverity and maintainers happy by asserting that the index into the
array is valid. Also, while not detected by Coverity (yet), add an
assert in nvme_subsys_ns and nvme_subsys_register_ns as well since a
similar issue is exists there.

Fixes: 037953b5b299 ("hw/block/nvme: support namespace detach")
Fixes: CID 1450757
Fixes: CID 1450758
Cc: Minwoo Im <minwoo.im.dev@gmail.com>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/block/nvme-subsys.h |  2 ++
 hw/block/nvme.h        | 10 ++++++++--
 hw/block/nvme-subsys.c |  7 +++++--
 3 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/hw/block/nvme-subsys.h b/hw/block/nvme-subsys.h
index fb66ae752ad5..aafa04b84829 100644
--- a/hw/block/nvme-subsys.h
+++ b/hw/block/nvme-subsys.h
@@ -54,6 +54,8 @@ static inline NvmeNamespace *nvme_subsys_ns(NvmeSubsystem *subsys,
         return NULL;
     }
 
+    assert(nsid && nsid <= NVME_SUBSYS_MAX_NAMESPACES);
+
     return subsys->namespaces[nsid];
 }
 
diff --git a/hw/block/nvme.h b/hw/block/nvme.h
index 4955d649c7d4..45ba9dbc2131 100644
--- a/hw/block/nvme.h
+++ b/hw/block/nvme.h
@@ -236,12 +236,18 @@ static inline bool nvme_ns_is_attached(NvmeCtrl *n, NvmeNamespace *ns)
 
 static inline void nvme_ns_attach(NvmeCtrl *n, NvmeNamespace *ns)
 {
-    n->namespaces[nvme_nsid(ns) - 1] = ns;
+    uint32_t nsid = ns->params.nsid;
+    assert(nsid && nsid <= NVME_MAX_NAMESPACES);
+
+    n->namespaces[nsid - 1] = ns;
 }
 
 static inline void nvme_ns_detach(NvmeCtrl *n, NvmeNamespace *ns)
 {
-    n->namespaces[nvme_nsid(ns) - 1] = NULL;
+    uint32_t nsid = ns->params.nsid;
+    assert(nsid && nsid <= NVME_MAX_NAMESPACES);
+
+    n->namespaces[nsid - 1] = NULL;
 }
 
 static inline NvmeCQueue *nvme_cq(NvmeRequest *req)
diff --git a/hw/block/nvme-subsys.c b/hw/block/nvme-subsys.c
index af4804a819ee..2f6d3b47bacf 100644
--- a/hw/block/nvme-subsys.c
+++ b/hw/block/nvme-subsys.c
@@ -47,15 +47,18 @@ int nvme_subsys_register_ns(NvmeNamespace *ns, Error **errp)
 {
     NvmeSubsystem *subsys = ns->subsys;
     NvmeCtrl *n;
+    uint32_t nsid = ns->params.nsid;
     int i;
 
-    if (subsys->namespaces[nvme_nsid(ns)]) {
+    assert(nsid && nsid <= NVME_SUBSYS_MAX_NAMESPACES);
+
+    if (subsys->namespaces[nsid]) {
         error_setg(errp, "namespace %d already registerd to subsy %s",
                    nvme_nsid(ns), subsys->parent_obj.id);
         return -1;
     }
 
-    subsys->namespaces[nvme_nsid(ns)] = ns;
+    subsys->namespaces[nsid] = ns;
 
     for (i = 0; i < ARRAY_SIZE(subsys->ctrls); i++) {
         n = subsys->ctrls[i];
-- 
2.30.1

Re: [PATCH 2/2] hw/block/nvme: assert namespaces array indices

[Philippe Mathieu-Daudé]

On 3/15/21 12:03 PM, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Coverity complains about a possible memory corruption in the
> nvme_ns_attach and _detach functions. While we should not (famous last
> words) be able to reach this function without nsid having previously
> been validated, this is still an open door for future misuse.
> 
> Make Coverity and maintainers happy by asserting that the index into the
> array is valid. Also, while not detected by Coverity (yet), add an
> assert in nvme_subsys_ns and nvme_subsys_register_ns as well since a
> similar issue is exists there.
> 
> Fixes: 037953b5b299 ("hw/block/nvme: support namespace detach")
> Fixes: CID 1450757
> Fixes: CID 1450758
> Cc: Minwoo Im <minwoo.im.dev@gmail.com>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>  hw/block/nvme-subsys.h |  2 ++
>  hw/block/nvme.h        | 10 ++++++++--
>  hw/block/nvme-subsys.c |  7 +++++--
>  3 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/block/nvme-subsys.h b/hw/block/nvme-subsys.h
> index fb66ae752ad5..aafa04b84829 100644
> --- a/hw/block/nvme-subsys.h
> +++ b/hw/block/nvme-subsys.h
> @@ -54,6 +54,8 @@ static inline NvmeNamespace *nvme_subsys_ns(NvmeSubsystem *subsys,
>          return NULL;
>      }
>  
> +    assert(nsid && nsid <= NVME_SUBSYS_MAX_NAMESPACES);
> +
>      return subsys->namespaces[nsid];
>  }
>  
> diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> index 4955d649c7d4..45ba9dbc2131 100644
> --- a/hw/block/nvme.h
> +++ b/hw/block/nvme.h
> @@ -236,12 +236,18 @@ static inline bool nvme_ns_is_attached(NvmeCtrl *n, NvmeNamespace *ns)
>  
>  static inline void nvme_ns_attach(NvmeCtrl *n, NvmeNamespace *ns)
>  {
> -    n->namespaces[nvme_nsid(ns) - 1] = ns;
> +    uint32_t nsid = ns->params.nsid;

Why not keep using nvme_nsid(ns)?

> +    assert(nsid && nsid <= NVME_MAX_NAMESPACES);
> +
> +    n->namespaces[nsid - 1] = ns;
>  }
>  
>  static inline void nvme_ns_detach(NvmeCtrl *n, NvmeNamespace *ns)
>  {
> -    n->namespaces[nvme_nsid(ns) - 1] = NULL;
> +    uint32_t nsid = ns->params.nsid;

Ditto.

> +    assert(nsid && nsid <= NVME_MAX_NAMESPACES);
> +
> +    n->namespaces[nsid - 1] = NULL;
>  }
>  
>  static inline NvmeCQueue *nvme_cq(NvmeRequest *req)
> diff --git a/hw/block/nvme-subsys.c b/hw/block/nvme-subsys.c
> index af4804a819ee..2f6d3b47bacf 100644
> --- a/hw/block/nvme-subsys.c
> +++ b/hw/block/nvme-subsys.c
> @@ -47,15 +47,18 @@ int nvme_subsys_register_ns(NvmeNamespace *ns, Error **errp)
>  {
>      NvmeSubsystem *subsys = ns->subsys;
>      NvmeCtrl *n;
> +    uint32_t nsid = ns->params.nsid;

Ditto.

Preferably using nvme_nsid():
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>      int i;
>  
> -    if (subsys->namespaces[nvme_nsid(ns)]) {
> +    assert(nsid && nsid <= NVME_SUBSYS_MAX_NAMESPACES);
> +
> +    if (subsys->namespaces[nsid]) {
>          error_setg(errp, "namespace %d already registerd to subsy %s",
>                     nvme_nsid(ns), subsys->parent_obj.id);
>          return -1;
>      }
>  
> -    subsys->namespaces[nvme_nsid(ns)] = ns;
> +    subsys->namespaces[nsid] = ns;
>  
>      for (i = 0; i < ARRAY_SIZE(subsys->ctrls); i++) {
>          n = subsys->ctrls[i];
> 

Re: [PATCH 2/2] hw/block/nvme: assert namespaces array indices

[Klaus Jensen]

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

On Mar 15 12:19, Philippe Mathieu-Daudé wrote:
> On 3/15/21 12:03 PM, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > Coverity complains about a possible memory corruption in the
> > nvme_ns_attach and _detach functions. While we should not (famous last
> > words) be able to reach this function without nsid having previously
> > been validated, this is still an open door for future misuse.
> > 
> > Make Coverity and maintainers happy by asserting that the index into the
> > array is valid. Also, while not detected by Coverity (yet), add an
> > assert in nvme_subsys_ns and nvme_subsys_register_ns as well since a
> > similar issue is exists there.
> > 
> > Fixes: 037953b5b299 ("hw/block/nvme: support namespace detach")
> > Fixes: CID 1450757
> > Fixes: CID 1450758
> > Cc: Minwoo Im <minwoo.im.dev@gmail.com>
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >  hw/block/nvme-subsys.h |  2 ++
> >  hw/block/nvme.h        | 10 ++++++++--
> >  hw/block/nvme-subsys.c |  7 +++++--
> >  3 files changed, 15 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/block/nvme-subsys.h b/hw/block/nvme-subsys.h
> > index fb66ae752ad5..aafa04b84829 100644
> > --- a/hw/block/nvme-subsys.h
> > +++ b/hw/block/nvme-subsys.h
> > @@ -54,6 +54,8 @@ static inline NvmeNamespace *nvme_subsys_ns(NvmeSubsystem *subsys,
> >          return NULL;
> >      }
> >  
> > +    assert(nsid && nsid <= NVME_SUBSYS_MAX_NAMESPACES);
> > +
> >      return subsys->namespaces[nsid];
> >  }
> >  
> > diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> > index 4955d649c7d4..45ba9dbc2131 100644
> > --- a/hw/block/nvme.h
> > +++ b/hw/block/nvme.h
> > @@ -236,12 +236,18 @@ static inline bool nvme_ns_is_attached(NvmeCtrl *n, NvmeNamespace *ns)
> >  
> >  static inline void nvme_ns_attach(NvmeCtrl *n, NvmeNamespace *ns)
> >  {
> > -    n->namespaces[nvme_nsid(ns) - 1] = ns;
> > +    uint32_t nsid = ns->params.nsid;
> 
> Why not keep using nvme_nsid(ns)?
> 
> > +    assert(nsid && nsid <= NVME_MAX_NAMESPACES);
> > +
> > +    n->namespaces[nsid - 1] = ns;
> >  }
> >  
> >  static inline void nvme_ns_detach(NvmeCtrl *n, NvmeNamespace *ns)
> >  {
> > -    n->namespaces[nvme_nsid(ns) - 1] = NULL;
> > +    uint32_t nsid = ns->params.nsid;
> 
> Ditto.
> 
> > +    assert(nsid && nsid <= NVME_MAX_NAMESPACES);
> > +
> > +    n->namespaces[nsid - 1] = NULL;
> >  }
> >  
> >  static inline NvmeCQueue *nvme_cq(NvmeRequest *req)
> > diff --git a/hw/block/nvme-subsys.c b/hw/block/nvme-subsys.c
> > index af4804a819ee..2f6d3b47bacf 100644
> > --- a/hw/block/nvme-subsys.c
> > +++ b/hw/block/nvme-subsys.c
> > @@ -47,15 +47,18 @@ int nvme_subsys_register_ns(NvmeNamespace *ns, Error **errp)
> >  {
> >      NvmeSubsystem *subsys = ns->subsys;
> >      NvmeCtrl *n;
> > +    uint32_t nsid = ns->params.nsid;
> 
> Ditto.
> 
> Preferably using nvme_nsid():
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 

You are right. I'll switch it back. Thanks!

> >      int i;
> >  
> > -    if (subsys->namespaces[nvme_nsid(ns)]) {
> > +    assert(nsid && nsid <= NVME_SUBSYS_MAX_NAMESPACES);
> > +
> > +    if (subsys->namespaces[nsid]) {
> >          error_setg(errp, "namespace %d already registerd to subsy %s",
> >                     nvme_nsid(ns), subsys->parent_obj.id);
> >          return -1;
> >      }
> >  
> > -    subsys->namespaces[nvme_nsid(ns)] = ns;
> > +    subsys->namespaces[nsid] = ns;
> >  
> >      for (i = 0; i < ARRAY_SIZE(subsys->ctrls); i++) {
> >          n = subsys->ctrls[i];
> > 
> 

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

[PATCH 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).

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

 hw/block/nvme-dif.c | 2 +-
 hw/block/nvme.c     | 5 +++++
 2 files changed, 6 insertions(+), 1 deletion(-)

-- 
2.31.0