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

From: Klaus Jensen <its@irrelevant.dk>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: Fam Zheng <fam@euphon.net>, Kevin Wolf <kwolf@redhat.com>,
	Eduardo Habkost <ehabkost@redhat.com>,
	Qemu-block <qemu-block@nongnu.org>,
	Heinrich Schuchardt <xypron.glpk@gmx.de>,
	QEMU Developers <qemu-devel@nongnu.org>,
	Max Reitz <mreitz@redhat.com>,
	Stefan Hajnoczi <stefanha@redhat.com>,
	Klaus Jensen <k.jensen@samsung.com>,
	Keith Busch <kbusch@kernel.org>
Subject: Re: [PULL 06/23] hw/nvme: namespace parameter for EUI-64
Date: Mon, 9 Aug 2021 12:44:00 +0200	[thread overview]
Message-ID: <YREG8BuwApcdqUKu@apples.localdomain> (raw)
In-Reply-To: <CAFEAcA-Xodpue0bUBbG++-CcRPV-+EJh=qw_23aGPEh3sAk0Ow@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 4289 bytes --]

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!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]