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