On Mar 12 13:12, Peter Maydell wrote: > On Tue, 9 Mar 2021 at 11:46, Klaus Jensen <i...@irrelevant.dk> wrote: > > > > From: Minwoo Im <minwoo.im....@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