On Tue, 29 Jun 2021 at 19:48, Klaus Jensen <i...@irrelevant.dk> wrote:
>
> From: Heinrich Schuchardt <xypron.g...@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
