Re: [PATCH 2/2] hw/block/nvme: assert namespaces array indices

[Klaus Jensen <its@irrelevant.dk>]

[-- Attachment #1: Type: text/plain, Size: 3639 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]