Message ID | 20210629184743.230173-7-its@irrelevant.dk |
---|---|
State | New |
Headers | show |
Series | [PULL,01/23] hw/nvme: fix style | expand |
On Tue, 29 Jun 2021 at 19:48, Klaus Jensen <its@irrelevant.dk> wrote: > > From: Heinrich Schuchardt <xypron.glpk@gmx.de> > > The EUI-64 field is the only identifier for NVMe namespaces in UEFI device > paths. Add a new namespace property "eui64", that provides the user the > option to specify the EUI-64. Hi; Coverity complains about some uses of uninitialized data in this code (CID 1458835 1459295 1459580). I think the bug was present in the previous version of this function, but this was the last change to touch it... > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c > index 7dea64b72e6a..762bb82e3cac 100644 > --- a/hw/nvme/ctrl.c > +++ b/hw/nvme/ctrl.c > @@ -4426,19 +4426,19 @@ static uint16_t nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeRequest *req) > NvmeIdentify *c = (NvmeIdentify *)&req->cmd; > uint32_t nsid = le32_to_cpu(c->nsid); > uint8_t list[NVME_IDENTIFY_DATA_SIZE] = {}; > - > - struct data { > - struct { > - NvmeIdNsDescr hdr; > - uint8_t v[NVME_NIDL_UUID]; > - } uuid; > - struct { > - NvmeIdNsDescr hdr; > - uint8_t v; > - } csi; > - }; > - > - struct data *ns_descrs = (struct data *)list; > + uint8_t *pos = list; > + struct { > + NvmeIdNsDescr hdr; > + uint8_t v[NVME_NIDL_UUID]; > + } QEMU_PACKED uuid; > + struct { > + NvmeIdNsDescr hdr; > + uint64_t v; > + } QEMU_PACKED eui64; > + struct { > + NvmeIdNsDescr hdr; > + uint8_t v; > + } QEMU_PACKED csi; Here we define locals 'uuid', 'eui64', 'csi', without an initializer. > trace_pci_nvme_identify_ns_descr_list(nsid); > > @@ -4452,17 +4452,29 @@ static uint16_t nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeRequest *req) > } > > /* > - * Because the NGUID and EUI64 fields are 0 in the Identify Namespace data > - * structure, a Namespace UUID (nidt = 3h) must be reported in the > - * Namespace Identification Descriptor. Add the namespace UUID here. > + * If the EUI-64 field is 0 and the NGUID field is 0, the namespace must > + * provide a valid Namespace UUID in the Namespace Identification Descriptor > + * data structure. QEMU does not yet support setting NGUID. > */ > - ns_descrs->uuid.hdr.nidt = NVME_NIDT_UUID; > - ns_descrs->uuid.hdr.nidl = NVME_NIDL_UUID; > - memcpy(&ns_descrs->uuid.v, ns->params.uuid.data, NVME_NIDL_UUID); > + uuid.hdr.nidt = NVME_NIDT_UUID; > + uuid.hdr.nidl = NVME_NIDL_UUID; > + memcpy(uuid.v, ns->params.uuid.data, NVME_NIDL_UUID); Here we fill in some fields of uuid, but we don't touch uuid.hdr.rsvd2[], which remains thus 2 bytes of uninitialized junk from our stack. > + memcpy(pos, &uuid, sizeof(uuid)); Here we copy all of uuid to a buffer which we're going to hand to the guest, so we've just given it two bytes of QEMU stack data that it shouldn't really be able to look at. > + pos += sizeof(uuid); > > - ns_descrs->csi.hdr.nidt = NVME_NIDT_CSI; > - ns_descrs->csi.hdr.nidl = NVME_NIDL_CSI; > - ns_descrs->csi.v = ns->csi; > + if (ns->params.eui64) { > + eui64.hdr.nidt = NVME_NIDT_EUI64; > + eui64.hdr.nidl = NVME_NIDL_EUI64; > + eui64.v = cpu_to_be64(ns->params.eui64); > + memcpy(pos, &eui64, sizeof(eui64)); > + pos += sizeof(eui64); > + } > + > + csi.hdr.nidt = NVME_NIDT_CSI; > + csi.hdr.nidl = NVME_NIDL_CSI; > + csi.v = ns->csi; > + memcpy(pos, &csi, sizeof(csi)); > + pos += sizeof(csi); We do the same thing for the rsvd2[] bytes in csi.hdr and eui64.hdr. > return nvme_c2h(n, list, sizeof(list), req); > } Explicitly zero-initializing uuid, csi, eui64 with "= { }" would be the most robust fix. If you think it's worth avoiding "zero init and then overwrite 90% of the fields anyway" then you could explicitly zero the .hdr.rsvd2 bytes. thanks -- PMM
On Aug 9 11:18, Peter Maydell wrote: > On Tue, 29 Jun 2021 at 19:48, Klaus Jensen <its@irrelevant.dk> wrote: > > > > From: Heinrich Schuchardt <xypron.glpk@gmx.de> > > > > The EUI-64 field is the only identifier for NVMe namespaces in UEFI device > > paths. Add a new namespace property "eui64", that provides the user the > > option to specify the EUI-64. > > Hi; Coverity complains about some uses of uninitialized data in this > code (CID 1458835 1459295 1459580). I think the bug was present in > the previous version of this function, but this was the last change > to touch it... > > > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c > > index 7dea64b72e6a..762bb82e3cac 100644 > > --- a/hw/nvme/ctrl.c > > +++ b/hw/nvme/ctrl.c > > @@ -4426,19 +4426,19 @@ static uint16_t nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeRequest *req) > > NvmeIdentify *c = (NvmeIdentify *)&req->cmd; > > uint32_t nsid = le32_to_cpu(c->nsid); > > uint8_t list[NVME_IDENTIFY_DATA_SIZE] = {}; > > - > > - struct data { > > - struct { > > - NvmeIdNsDescr hdr; > > - uint8_t v[NVME_NIDL_UUID]; > > - } uuid; > > - struct { > > - NvmeIdNsDescr hdr; > > - uint8_t v; > > - } csi; > > - }; > > - > > - struct data *ns_descrs = (struct data *)list; > > + uint8_t *pos = list; > > + struct { > > + NvmeIdNsDescr hdr; > > + uint8_t v[NVME_NIDL_UUID]; > > + } QEMU_PACKED uuid; > > + struct { > > + NvmeIdNsDescr hdr; > > + uint64_t v; > > + } QEMU_PACKED eui64; > > + struct { > > + NvmeIdNsDescr hdr; > > + uint8_t v; > > + } QEMU_PACKED csi; > > Here we define locals 'uuid', 'eui64', 'csi', without an initializer. > > > trace_pci_nvme_identify_ns_descr_list(nsid); > > > > @@ -4452,17 +4452,29 @@ static uint16_t nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeRequest *req) > > } > > > > /* > > - * Because the NGUID and EUI64 fields are 0 in the Identify Namespace data > > - * structure, a Namespace UUID (nidt = 3h) must be reported in the > > - * Namespace Identification Descriptor. Add the namespace UUID here. > > + * If the EUI-64 field is 0 and the NGUID field is 0, the namespace must > > + * provide a valid Namespace UUID in the Namespace Identification Descriptor > > + * data structure. QEMU does not yet support setting NGUID. > > */ > > - ns_descrs->uuid.hdr.nidt = NVME_NIDT_UUID; > > - ns_descrs->uuid.hdr.nidl = NVME_NIDL_UUID; > > - memcpy(&ns_descrs->uuid.v, ns->params.uuid.data, NVME_NIDL_UUID); > > + uuid.hdr.nidt = NVME_NIDT_UUID; > > + uuid.hdr.nidl = NVME_NIDL_UUID; > > + memcpy(uuid.v, ns->params.uuid.data, NVME_NIDL_UUID); > > Here we fill in some fields of uuid, but we don't touch uuid.hdr.rsvd2[], > which remains thus 2 bytes of uninitialized junk from our stack. > > > + memcpy(pos, &uuid, sizeof(uuid)); > > Here we copy all of uuid to a buffer which we're going to hand > to the guest, so we've just given it two bytes of QEMU stack data > that it shouldn't really be able to look at. > > > + pos += sizeof(uuid); > > > > > - ns_descrs->csi.hdr.nidt = NVME_NIDT_CSI; > > - ns_descrs->csi.hdr.nidl = NVME_NIDL_CSI; > > - ns_descrs->csi.v = ns->csi; > > + if (ns->params.eui64) { > > + eui64.hdr.nidt = NVME_NIDT_EUI64; > > + eui64.hdr.nidl = NVME_NIDL_EUI64; > > + eui64.v = cpu_to_be64(ns->params.eui64); > > + memcpy(pos, &eui64, sizeof(eui64)); > > + pos += sizeof(eui64); > > + } > > + > > + csi.hdr.nidt = NVME_NIDT_CSI; > > + csi.hdr.nidl = NVME_NIDL_CSI; > > + csi.v = ns->csi; > > + memcpy(pos, &csi, sizeof(csi)); > > + pos += sizeof(csi); > > We do the same thing for the rsvd2[] bytes in csi.hdr and eui64.hdr. > > > return nvme_c2h(n, list, sizeof(list), req); > > } > > Explicitly zero-initializing uuid, csi, eui64 with "= { }" would > be the most robust fix. If you think it's worth avoiding "zero > init and then overwrite 90% of the fields anyway" then you could > explicitly zero the .hdr.rsvd2 bytes. > Thanks Peter, Fix posted!
diff --git a/docs/system/nvme.rst b/docs/system/nvme.rst index f7f63d6bf615..b5f8288d7c85 100644 --- a/docs/system/nvme.rst +++ b/docs/system/nvme.rst @@ -81,6 +81,10 @@ There are a number of parameters available: Set the UUID of the namespace. This will be reported as a "Namespace UUID" descriptor in the Namespace Identification Descriptor List. +``eui64`` + Set the EUI-64 of the namespace. This will be reported as a "IEEE Extended + Unique Identifier" descriptor in the Namespace Identification Descriptor List. + ``bus`` If there are more ``nvme`` devices defined, this parameter may be used to attach the namespace to a specific ``nvme`` device (identified by an ``id`` diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h index 93a7e0e5380e..ac90e13d7b3f 100644 --- a/hw/nvme/nvme.h +++ b/hw/nvme/nvme.h @@ -83,6 +83,7 @@ typedef struct NvmeNamespaceParams { bool shared; uint32_t nsid; QemuUUID uuid; + uint64_t eui64; uint16_t ms; uint8_t mset; diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c index 7dea64b72e6a..762bb82e3cac 100644 --- a/hw/nvme/ctrl.c +++ b/hw/nvme/ctrl.c @@ -4426,19 +4426,19 @@ static uint16_t nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeRequest *req) NvmeIdentify *c = (NvmeIdentify *)&req->cmd; uint32_t nsid = le32_to_cpu(c->nsid); uint8_t list[NVME_IDENTIFY_DATA_SIZE] = {}; - - struct data { - struct { - NvmeIdNsDescr hdr; - uint8_t v[NVME_NIDL_UUID]; - } uuid; - struct { - NvmeIdNsDescr hdr; - uint8_t v; - } csi; - }; - - struct data *ns_descrs = (struct data *)list; + uint8_t *pos = list; + struct { + NvmeIdNsDescr hdr; + uint8_t v[NVME_NIDL_UUID]; + } QEMU_PACKED uuid; + struct { + NvmeIdNsDescr hdr; + uint64_t v; + } QEMU_PACKED eui64; + struct { + NvmeIdNsDescr hdr; + uint8_t v; + } QEMU_PACKED csi; trace_pci_nvme_identify_ns_descr_list(nsid); @@ -4452,17 +4452,29 @@ static uint16_t nvme_identify_ns_descr_list(NvmeCtrl *n, NvmeRequest *req) } /* - * Because the NGUID and EUI64 fields are 0 in the Identify Namespace data - * structure, a Namespace UUID (nidt = 3h) must be reported in the - * Namespace Identification Descriptor. Add the namespace UUID here. + * If the EUI-64 field is 0 and the NGUID field is 0, the namespace must + * provide a valid Namespace UUID in the Namespace Identification Descriptor + * data structure. QEMU does not yet support setting NGUID. */ - ns_descrs->uuid.hdr.nidt = NVME_NIDT_UUID; - ns_descrs->uuid.hdr.nidl = NVME_NIDL_UUID; - memcpy(&ns_descrs->uuid.v, ns->params.uuid.data, NVME_NIDL_UUID); + uuid.hdr.nidt = NVME_NIDT_UUID; + uuid.hdr.nidl = NVME_NIDL_UUID; + memcpy(uuid.v, ns->params.uuid.data, NVME_NIDL_UUID); + memcpy(pos, &uuid, sizeof(uuid)); + pos += sizeof(uuid); - ns_descrs->csi.hdr.nidt = NVME_NIDT_CSI; - ns_descrs->csi.hdr.nidl = NVME_NIDL_CSI; - ns_descrs->csi.v = ns->csi; + if (ns->params.eui64) { + eui64.hdr.nidt = NVME_NIDT_EUI64; + eui64.hdr.nidl = NVME_NIDL_EUI64; + eui64.v = cpu_to_be64(ns->params.eui64); + memcpy(pos, &eui64, sizeof(eui64)); + pos += sizeof(eui64); + } + + csi.hdr.nidt = NVME_NIDT_CSI; + csi.hdr.nidl = NVME_NIDL_CSI; + csi.v = ns->csi; + memcpy(pos, &csi, sizeof(csi)); + pos += sizeof(csi); return nvme_c2h(n, list, sizeof(list), req); } diff --git a/hw/nvme/ns.c b/hw/nvme/ns.c index 3fec9c627321..45e457de6ae1 100644 --- a/hw/nvme/ns.c +++ b/hw/nvme/ns.c @@ -77,6 +77,7 @@ static int nvme_ns_init(NvmeNamespace *ns, Error **errp) id_ns->mssrl = cpu_to_le16(ns->params.mssrl); id_ns->mcl = cpu_to_le32(ns->params.mcl); id_ns->msrc = ns->params.msrc; + id_ns->eui64 = cpu_to_be64(ns->params.eui64); ds = 31 - clz32(ns->blkconf.logical_block_size); ms = ns->params.ms; @@ -511,6 +512,7 @@ static Property nvme_ns_props[] = { DEFINE_PROP_BOOL("shared", NvmeNamespace, params.shared, false), DEFINE_PROP_UINT32("nsid", NvmeNamespace, params.nsid, 0), DEFINE_PROP_UUID("uuid", NvmeNamespace, params.uuid), + DEFINE_PROP_UINT64("eui64", NvmeNamespace, params.eui64, 0), DEFINE_PROP_UINT16("ms", NvmeNamespace, params.ms, 0), DEFINE_PROP_UINT8("mset", NvmeNamespace, params.mset, 0), DEFINE_PROP_UINT8("pi", NvmeNamespace, params.pi, 0),