| Message ID | 20210315110359.51450-3-its@irrelevant.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/block/nvme: coverity fixes | expand |
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]; >
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]; > > >
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];