Re: [PULL v2 36/38] hw/block/nvme: support namespace attachment command

[Klaus Jensen]

On Mar 12 13:12, Peter Maydell wrote:
> On Tue, 9 Mar 2021 at 11:46, Klaus Jensen <its@irrelevant.dk> wrote:
> >
> > From: Minwoo Im <minwoo.im.dev@gmail.com>
> >
> > This patch supports Namespace Attachment command for the pre-defined
> > nvme-ns device nodes.  Of course, attach/detach namespace should only be
> > supported in case 'subsys' is given.  This is because if we detach a
> > namespace from a controller, somebody needs to manage the detached, but
> > allocated namespace in the NVMe subsystem.
> >
> > As command effect for the namespace attachment command is registered,
> > the host will be notified that namespace inventory is changed so that
> > host will rescan the namespace inventory after this command.  For
> > example, kernel driver manages this command effect via passthru IOCTL.
> 
> > diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> > index 85a7b5a14f4e..1287bc2cd17a 100644
> > --- a/hw/block/nvme.h
> > +++ b/hw/block/nvme.h
> > @@ -235,6 +235,11 @@ static inline void nvme_ns_attach(NvmeCtrl *n, 
> > NvmeNamespace *ns)
> >      n->namespaces[nvme_nsid(ns) - 1] = ns;
> >  }
> >
> > +static inline void nvme_ns_detach(NvmeCtrl *n, NvmeNamespace *ns)
> > +{
> > +    n->namespaces[nvme_nsid(ns) - 1] = NULL;
> > +}
> 
> Hi; Coverity complains about a possible array overflow both here
> in nvme_ns_detach() (CID 1450757) and in nvme_ns_attach() (CID 1450758):
> nvme_nsid() can return -1, but this code does not check for that.
> 
> If these functions both assume that their ns argument is non-NULL,
> then adding an "assert(ns)" would probably placate Coverity and also
> would mean that any bugs elsewhere resulting in accidentally passing
> a NULL pointer would result in a clean assertion failure rather than
> memory corruption. (Or you could directly assert that the array index
> is in-bounds, I guess.)
> 

Hi Peter,

Thanks!

As far as I can tell, we never reach this without nsid being a valid
value or ns being NULL, but as you say, future misuse would be bad. I
will post a fix to make sure such misuse does not happen in the future.

signature.asc
Description: PGP signature