svn commit: r337887 - head/usr.sbin/bhyve

[Rodney W. Grimes]

> On Thu, Aug 16, 2018 at 8:03 AM, Rodney W. Grimes <
> freebsd at pdx.rh.cn85.dnsmgr.net> wrote:
> 
> > > Author: araujo
> > > Date: Thu Aug 16 06:31:54 2018
> > > New Revision: 337887
> > > URL: https://svnweb.freebsd.org/changeset/base/337887
> > >
> > > Log:
> > >   Add a comment explaining how the PSN works and why there is no need for
> > >   a null terminator. Also mark CID 1394825 as intentional.
> > >
> > >   Reported by:        Coverity
> > >   CID:                1394825
> > >   MFC after:  1 week
> > >   Sponsored by:       iXsystems Inc.
> > >
> > > Modified:
> > >   head/usr.sbin/bhyve/pci_nvme.c
> > >
> > > Modified: head/usr.sbin/bhyve/pci_nvme.c
> > > ============================================================
> > ==================
> > > --- head/usr.sbin/bhyve/pci_nvme.c    Thu Aug 16 06:20:25 2018
> > (r337886)
> > > +++ head/usr.sbin/bhyve/pci_nvme.c    Thu Aug 16 06:31:54 2018
> > (r337887)
> > > @@ -1714,6 +1714,11 @@ pci_nvme_parse_opts(struct pci_nvme_softc *sc,
> > char *o
> > >               } else if (!strcmp("sectsz", xopts)) {
> > >                       sectsz = atoi(config);
> > >               } else if (!strcmp("ser", xopts)) {
> > > +                     /*
> > > +                      * This field indicates the Product Serial Number
> > in
> > > +                      * 8-bit ASCII, unused bytes should be NULL
> > characters.
> > > +                      * Ref: NVM Express Management Interface 1.0a.
> > > +                      */
> >
> > I have seen this before on ATA devices,
> > if the vendor fills all bytes of PSN,
> > there well be no unused bytes,
> > so no null byte at the end,
> > and you end up with an unterminated string.
> >
> > Can you please verify that this edge case is handled correctly?
> > Thanks,
> > Rod
> >
> > >                       memset(sc->ctrldata.sn, 0, sizeof(sc->ctrldata.sn
> > ));
> > >                       strncpy(sc->ctrldata.sn, config,
> > >                               sizeof(sc->ctrldata.sn));
> > >
> >
> 
> strncpy will not NUL terminate when there's exactly sizeof(ctrldata.sn)
> bytes in the 'config' string. Thus that case where all characters are
> non-NUL is handled properly (the standard says the string need not be NUL
> terminated).

I get that, are we certain that all consumers of ctrldata.sn
obey this, ie it is never attempted to print this string
with a %s?

> Keep in mind, though, that ATA is 100% irrelevant to NVMe,
> since the NVMe standard specifies everything.

I was using that as a case that has been seen where
an assumption about there always being a null in the SN
would be certain that strings are null terminated,
not saying that ATA applied to NVMe.

> 
> I've sent a followup to marcelo though about the 8-bit and NUL details,
> however, since I have conflicting info about that.
> 
> Warner

-- 
Rod Grimes                                                 rgrimes at freebsd.org