* [PATCH 0/2] hw/block/nvme: coverity fixes @ 2021-03-15 11:03 Klaus Jensen 2021-03-15 11:03 ` [PATCH 1/2] hw/block/nvme: fix potential overflow Klaus Jensen 2021-03-15 11:03 ` [PATCH 2/2] hw/block/nvme: assert namespaces array indices Klaus Jensen 0 siblings, 2 replies; 5+ messages in thread From: Klaus Jensen @ 2021-03-15 11:03 UTC (permalink / raw) To: qemu-devel Cc: Kevin Wolf, qemu-block, Klaus Jensen, Max Reitz, Keith Busch, Klaus Jensen From: Klaus Jensen <k.jensen@samsung.com> Fix three issues reported by coverity (CID 1450756, CID 1450757 and CID 1450758). Klaus Jensen (2): hw/block/nvme: fix potential overflow hw/block/nvme: assert namespaces array indices hw/block/nvme-subsys.h | 2 ++ hw/block/nvme.h | 10 ++++++++-- hw/block/nvme-subsys.c | 7 +++++-- hw/block/nvme.c | 2 +- 4 files changed, 16 insertions(+), 5 deletions(-) -- 2.30.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] hw/block/nvme: fix potential overflow 2021-03-15 11:03 [PATCH 0/2] hw/block/nvme: coverity fixes Klaus Jensen @ 2021-03-15 11:03 ` Klaus Jensen 2021-03-15 11:03 ` [PATCH 2/2] hw/block/nvme: assert namespaces array indices Klaus Jensen 1 sibling, 0 replies; 5+ messages in thread From: Klaus Jensen @ 2021-03-15 11:03 UTC (permalink / raw) To: qemu-devel Cc: Kevin Wolf, qemu-block, Klaus Jensen, Max Reitz, Keith Busch, Klaus Jensen From: Klaus Jensen <k.jensen@samsung.com> page_size is a uint32_t, and zasl is a uint8_t, so the expression `page_size << zasl` is done using 32-bit arithmetic and might overflow. Since we then compare this against a 64 bit data_size value, Coverity complains that we might overflow unintentionally. An MDTS/ZASL value in excess of 4GiB is probably impractical, but it is not entirely unrealistic, so add a cast such that we handle that case properly. Fixes: 578d914b263c ("hw/block/nvme: align zoned.zasl with mdts") Fixes: CID 1450756 Signed-off-by: Klaus Jensen <k.jensen@samsung.com> --- hw/block/nvme.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/block/nvme.c b/hw/block/nvme.c index d439e44db839..f8ad34077000 100644 --- a/hw/block/nvme.c +++ b/hw/block/nvme.c @@ -2188,7 +2188,7 @@ static uint16_t nvme_do_write(NvmeCtrl *n, NvmeRequest *req, bool append, goto invalid; } - if (n->params.zasl && data_size > n->page_size << n->params.zasl) { + if (n->params.zasl && data_size > (uint64_t)n->page_size << n->params.zasl) { trace_pci_nvme_err_zasl(data_size); return NVME_INVALID_FIELD | NVME_DNR; } -- 2.30.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] hw/block/nvme: assert namespaces array indices 2021-03-15 11:03 [PATCH 0/2] hw/block/nvme: coverity fixes Klaus Jensen 2021-03-15 11:03 ` [PATCH 1/2] hw/block/nvme: fix potential overflow Klaus Jensen @ 2021-03-15 11:03 ` Klaus Jensen 2021-03-15 11:19 ` Philippe Mathieu-Daudé 1 sibling, 1 reply; 5+ messages in thread From: Klaus Jensen @ 2021-03-15 11:03 UTC (permalink / raw) To: qemu-devel Cc: Kevin Wolf, qemu-block, Klaus Jensen, Max Reitz, Keith Busch, Minwoo Im, Klaus Jensen 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; + 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]; -- 2.30.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] hw/block/nvme: assert namespaces array indices 2021-03-15 11:03 ` [PATCH 2/2] hw/block/nvme: assert namespaces array indices Klaus Jensen @ 2021-03-15 11:19 ` Philippe Mathieu-Daudé 2021-03-15 11:24 ` Klaus Jensen 0 siblings, 1 reply; 5+ messages in thread From: Philippe Mathieu-Daudé @ 2021-03-15 11:19 UTC (permalink / raw) To: Klaus Jensen, qemu-devel Cc: Kevin Wolf, qemu-block, Klaus Jensen, Max Reitz, Minwoo Im, Keith Busch 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]; > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] hw/block/nvme: assert namespaces array indices 2021-03-15 11:19 ` Philippe Mathieu-Daudé @ 2021-03-15 11:24 ` Klaus Jensen 0 siblings, 0 replies; 5+ messages in thread From: Klaus Jensen @ 2021-03-15 11:24 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Kevin Wolf, qemu-block, Klaus Jensen, qemu-devel, Max Reitz, Minwoo Im, Keith Busch [-- 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 --] ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-03-15 11:28 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-03-15 11:03 [PATCH 0/2] hw/block/nvme: coverity fixes Klaus Jensen 2021-03-15 11:03 ` [PATCH 1/2] hw/block/nvme: fix potential overflow Klaus Jensen 2021-03-15 11:03 ` [PATCH 2/2] hw/block/nvme: assert namespaces array indices Klaus Jensen 2021-03-15 11:19 ` Philippe Mathieu-Daudé 2021-03-15 11:24 ` Klaus Jensen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).