On Mar 15 12:19, Philippe Mathieu-Daudé wrote: > 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> >
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]; > > >
signature.asc
Description: PGP signature