diff mbox series

[PULL,06/23] hw/nvme: namespace parameter for EUI-64

Message ID 20210629184743.230173-7-its@irrelevant.dk
State New
Headers show
Series [PULL,01/23] hw/nvme: fix style | expand

Commit Message

Klaus Jensen June 29, 2021, 6:47 p.m. UTC 
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.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
Acked-by: Klaus Jensen <k.jensen@samsung.com>
Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
---
 docs/system/nvme.rst |  4 ++++
 hw/nvme/nvme.h       |  1 +
 hw/nvme/ctrl.c       | 56 +++++++++++++++++++++++++++-----------------
 hw/nvme/ns.c         |  2 ++
 4 files changed, 41 insertions(+), 22 deletions(-)

Comments

Peter Maydell Aug. 9, 2021, 10:18 a.m. UTC | #1 
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
Klaus Jensen Aug. 9, 2021, 10:44 a.m. UTC | #2 
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 mbox series

Patch

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),