On 3/15/21 12:03 PM, Klaus Jensen wrote:
> From: Klaus Jensen <k.jen...@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....@gmail.com>
> Signed-off-by: Klaus Jensen <k.jen...@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é <phi...@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];
>
