[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/2] hw/block/nvme: assert namespaces array indices
From: |
Klaus Jensen |
Subject: |
Re: [PATCH 2/2] hw/block/nvme: assert namespaces array indices |
Date: |
Mon, 15 Mar 2021 12:24:39 +0100 |
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];
> >
>
signature.asc
Description: PGP signature