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

[Marcelo Araujo]

2018-08-17 7:33 GMT+08:00 Brooks Davis :

> On Thu, Aug 16, 2018 at 05:23:26PM -0600, Warner Losh wrote:
> > On Thu, Aug 16, 2018 at 5:16 PM, Brooks Davis 
> wrote:
> >
> > > On Fri, Aug 17, 2018 at 07:04:05AM +0800, Marcelo Araujo wrote:
> > > > 2018-08-17 3:29 GMT+08:00 Rodney W. Grimes
>  > > net>:
> > > >
> > > > > > On Thu, Aug 16, 2018 at 11:06 AM, John-Mark Gurney <
> j...@funkthat.com
> > > >
> > > > > wrote:
> > > > > >
> > > > > > > Marcelo Araujo wrote this message on Thu, Aug 16, 2018 at 06:31
> > > +:
> > > > > > > > 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.cThu Aug 16 06:20:25
> 2018
> > > > > > > (r337886)
> > > > > > > > +++ head/usr.sbin/bhyve/pci_nvme.cThu Aug 16 06:31:54
> 2018
> > > > > > > (r337887)
> > > > > > > > @@ -1714,6 +1714,11 @@ pci_nvme_parse_opts(struct
> pci_nvme_softc
> > > *sc,
> > > > > > > char *o
> > > > > > >
> > > > > > > [...]
> > > > > > >
> > > > > > > >   memset(sc->ctrldata.sn, 0, sizeof(sc->
> > > > > ctrldata.sn
> > > > > > > ));
> > > > > > > >   strncpy(sc->ctrldata.sn, config,
> > > > > > > >   sizeof(sc->ctrldata.sn));
> > > > > > >
> > > > > > > This memset is unneeded, as strncpy will write NUL bytes to
> fill
> > > out
> > > > > > > the buffer:
> > > > > > > If src is less than len characters long, the remainder of
> > > > > > >  dst is filled with `\0' characters.
> > > > > > >
> > > > > >
> > > > > > It also looks like the comment was wrong. The newest NVMe
> standards
> > > say
> > > > > > these fields should be 7-bit and space-padded.
> > > > >
> > > > > lol, which is what the vendor that caused me grief with
> > > > > ata serial numbers did decades ago.
> > > > >
> > > > > --
> > > > > Rod Grimes
> > > > > rgri...@freebsd.org
> > > > >
> > > >
> > > > I have discussed a bit with imp@, but I will drop the patch here to
> get
> > > > other peoples opinion too.
> > > > So, name space and firmware number also need to be padded with
> spaces.
> > > >
> > > > I couldn't think in any other better way to do that.
> > > >
> > > > Does this patch looks reasonable?
> > > > https://people.freebsd.org/~araujo/pci_nvme.diff
> > >
> > > You should check that len<=dst_size and at least truncate rather than
> > > overflowing.  If the strings from userspace you need to return or log
> an
> > > error, if they come from the kernel, you can panic.
> >
> > Help me understand, I thought that the strnlen bounded what was copied.
>
> Apparently the standard calls for ' ' rather than '\0' padding.  The
> prop memcpy+memset does the job, but contains potential overflows.
>
> -- Brooks
>

Maybe I missed something, but when I call cpywithpad() I pass the dst_size,
even if the 'src' is bigger than the 'dst' it will be truncated because
with strnlen(src, dst_size) the src will be reduced to dst_size length.

I made couple tests and could not overflow it(example):

cd->fr maximum length is 8:
cpywithpad((char *)cd->fr, sizeof(cd->fr), "1.0090\0", ' ');

the output of cpywithpad:
len: 8 is <= dst_size: 8

Same tests I made with mn that has length of 40 adding a string with 244
characters.
Sorry my ignorance, but could you give me a better example?

Best,
-- 

-- 
Marcelo Araujo(__)ara...@freebsd.org
\\\'',)http://www.FreeBSD.org    \/  \ ^
Power To Server. .\. /_)
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Brooks Davis]

On Thu, Aug 16, 2018 at 05:23:26PM -0600, Warner Losh wrote:
> On Thu, Aug 16, 2018 at 5:16 PM, Brooks Davis  wrote:
> 
> > On Fri, Aug 17, 2018 at 07:04:05AM +0800, Marcelo Araujo wrote:
> > > 2018-08-17 3:29 GMT+08:00 Rodney W. Grimes  > net>:
> > >
> > > > > On Thu, Aug 16, 2018 at 11:06 AM, John-Mark Gurney  > >
> > > > wrote:
> > > > >
> > > > > > Marcelo Araujo wrote this message on Thu, Aug 16, 2018 at 06:31
> > +:
> > > > > > > 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.cThu Aug 16 06:20:25 2018
> > > > > > (r337886)
> > > > > > > +++ head/usr.sbin/bhyve/pci_nvme.cThu Aug 16 06:31:54 2018
> > > > > > (r337887)
> > > > > > > @@ -1714,6 +1714,11 @@ pci_nvme_parse_opts(struct pci_nvme_softc
> > *sc,
> > > > > > char *o
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > >   memset(sc->ctrldata.sn, 0, sizeof(sc->
> > > > ctrldata.sn
> > > > > > ));
> > > > > > >   strncpy(sc->ctrldata.sn, config,
> > > > > > >   sizeof(sc->ctrldata.sn));
> > > > > >
> > > > > > This memset is unneeded, as strncpy will write NUL bytes to fill
> > out
> > > > > > the buffer:
> > > > > > If src is less than len characters long, the remainder of
> > > > > >  dst is filled with `\0' characters.
> > > > > >
> > > > >
> > > > > It also looks like the comment was wrong. The newest NVMe standards
> > say
> > > > > these fields should be 7-bit and space-padded.
> > > >
> > > > lol, which is what the vendor that caused me grief with
> > > > ata serial numbers did decades ago.
> > > >
> > > > --
> > > > Rod Grimes
> > > > rgri...@freebsd.org
> > > >
> > >
> > > I have discussed a bit with imp@, but I will drop the patch here to get
> > > other peoples opinion too.
> > > So, name space and firmware number also need to be padded with spaces.
> > >
> > > I couldn't think in any other better way to do that.
> > >
> > > Does this patch looks reasonable?
> > > https://people.freebsd.org/~araujo/pci_nvme.diff
> >
> > You should check that len<=dst_size and at least truncate rather than
> > overflowing.  If the strings from userspace you need to return or log an
> > error, if they come from the kernel, you can panic.
> 
> Help me understand, I thought that the strnlen bounded what was copied.

Apparently the standard calls for ' ' rather than '\0' padding.  The
prop memcpy+memset does the job, but contains potential overflows.

-- Brooks


signature.asc
Description: PGP signature

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

[Warner Losh]

On Thu, Aug 16, 2018 at 5:16 PM, Brooks Davis  wrote:

> On Fri, Aug 17, 2018 at 07:04:05AM +0800, Marcelo Araujo wrote:
> > 2018-08-17 3:29 GMT+08:00 Rodney W. Grimes  net>:
> >
> > > > On Thu, Aug 16, 2018 at 11:06 AM, John-Mark Gurney  >
> > > wrote:
> > > >
> > > > > Marcelo Araujo wrote this message on Thu, Aug 16, 2018 at 06:31
> +:
> > > > > > 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.cThu Aug 16 06:20:25 2018
> > > > > (r337886)
> > > > > > +++ head/usr.sbin/bhyve/pci_nvme.cThu Aug 16 06:31:54 2018
> > > > > (r337887)
> > > > > > @@ -1714,6 +1714,11 @@ pci_nvme_parse_opts(struct pci_nvme_softc
> *sc,
> > > > > char *o
> > > > >
> > > > > [...]
> > > > >
> > > > > >   memset(sc->ctrldata.sn, 0, sizeof(sc->
> > > ctrldata.sn
> > > > > ));
> > > > > >   strncpy(sc->ctrldata.sn, config,
> > > > > >   sizeof(sc->ctrldata.sn));
> > > > >
> > > > > This memset is unneeded, as strncpy will write NUL bytes to fill
> out
> > > > > the buffer:
> > > > > If src is less than len characters long, the remainder of
> > > > >  dst is filled with `\0' characters.
> > > > >
> > > >
> > > > It also looks like the comment was wrong. The newest NVMe standards
> say
> > > > these fields should be 7-bit and space-padded.
> > >
> > > lol, which is what the vendor that caused me grief with
> > > ata serial numbers did decades ago.
> > >
> > > --
> > > Rod Grimes
> > > rgri...@freebsd.org
> > >
> >
> > I have discussed a bit with imp@, but I will drop the patch here to get
> > other peoples opinion too.
> > So, name space and firmware number also need to be padded with spaces.
> >
> > I couldn't think in any other better way to do that.
> >
> > Does this patch looks reasonable?
> > https://people.freebsd.org/~araujo/pci_nvme.diff
>
> You should check that len<=dst_size and at least truncate rather than
> overflowing.  If the strings from userspace you need to return or log an
> error, if they come from the kernel, you can panic.
>

Help me understand, I thought that the strnlen bounded what was copied.

Warner
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Brooks Davis]

On Fri, Aug 17, 2018 at 07:04:05AM +0800, Marcelo Araujo wrote:
> 2018-08-17 3:29 GMT+08:00 Rodney W. Grimes :
> 
> > > On Thu, Aug 16, 2018 at 11:06 AM, John-Mark Gurney 
> > wrote:
> > >
> > > > Marcelo Araujo wrote this message on Thu, Aug 16, 2018 at 06:31 +:
> > > > > 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.cThu Aug 16 06:20:25 2018
> > > > (r337886)
> > > > > +++ head/usr.sbin/bhyve/pci_nvme.cThu Aug 16 06:31:54 2018
> > > > (r337887)
> > > > > @@ -1714,6 +1714,11 @@ pci_nvme_parse_opts(struct pci_nvme_softc *sc,
> > > > char *o
> > > >
> > > > [...]
> > > >
> > > > >   memset(sc->ctrldata.sn, 0, sizeof(sc->
> > ctrldata.sn
> > > > ));
> > > > >   strncpy(sc->ctrldata.sn, config,
> > > > >   sizeof(sc->ctrldata.sn));
> > > >
> > > > This memset is unneeded, as strncpy will write NUL bytes to fill out
> > > > the buffer:
> > > > If src is less than len characters long, the remainder of
> > > >  dst is filled with `\0' characters.
> > > >
> > >
> > > It also looks like the comment was wrong. The newest NVMe standards say
> > > these fields should be 7-bit and space-padded.
> >
> > lol, which is what the vendor that caused me grief with
> > ata serial numbers did decades ago.
> >
> > --
> > Rod Grimes
> > rgri...@freebsd.org
> >
> 
> I have discussed a bit with imp@, but I will drop the patch here to get
> other peoples opinion too.
> So, name space and firmware number also need to be padded with spaces.
> 
> I couldn't think in any other better way to do that.
> 
> Does this patch looks reasonable?
> https://people.freebsd.org/~araujo/pci_nvme.diff

You should check that len<=dst_size and at least truncate rather than
overflowing.  If the strings from userspace you need to return or log an
error, if they come from the kernel, you can panic.

-- Brooks


signature.asc
Description: PGP signature

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

[Marcelo Araujo]

2018-08-17 3:29 GMT+08:00 Rodney W. Grimes :

> > On Thu, Aug 16, 2018 at 11:06 AM, John-Mark Gurney 
> wrote:
> >
> > > Marcelo Araujo wrote this message on Thu, Aug 16, 2018 at 06:31 +:
> > > > 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.cThu Aug 16 06:20:25 2018
> > > (r337886)
> > > > +++ head/usr.sbin/bhyve/pci_nvme.cThu Aug 16 06:31:54 2018
> > > (r337887)
> > > > @@ -1714,6 +1714,11 @@ pci_nvme_parse_opts(struct pci_nvme_softc *sc,
> > > char *o
> > >
> > > [...]
> > >
> > > >   memset(sc->ctrldata.sn, 0, sizeof(sc->
> ctrldata.sn
> > > ));
> > > >   strncpy(sc->ctrldata.sn, config,
> > > >   sizeof(sc->ctrldata.sn));
> > >
> > > This memset is unneeded, as strncpy will write NUL bytes to fill out
> > > the buffer:
> > > If src is less than len characters long, the remainder of
> > >  dst is filled with `\0' characters.
> > >
> >
> > It also looks like the comment was wrong. The newest NVMe standards say
> > these fields should be 7-bit and space-padded.
>
> lol, which is what the vendor that caused me grief with
> ata serial numbers did decades ago.
>
> --
> Rod Grimes
> rgri...@freebsd.org
>

I have discussed a bit with imp@, but I will drop the patch here to get
other peoples opinion too.
So, name space and firmware number also need to be padded with spaces.

I couldn't think in any other better way to do that.

Does this patch looks reasonable?
https://people.freebsd.org/~araujo/pci_nvme.diff


Best,
-- 

-- 
Marcelo Araujo(__)ara...@freebsd.org
\\\'',)http://www.FreeBSD.org    \/  \ ^
Power To Server. .\. /_)
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Rodney W. Grimes]

> On Thu, Aug 16, 2018 at 11:06 AM, John-Mark Gurney  wrote:
> 
> > Marcelo Araujo wrote this message on Thu, Aug 16, 2018 at 06:31 +:
> > > 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.cThu Aug 16 06:20:25 2018
> > (r337886)
> > > +++ head/usr.sbin/bhyve/pci_nvme.cThu Aug 16 06:31:54 2018
> > (r337887)
> > > @@ -1714,6 +1714,11 @@ pci_nvme_parse_opts(struct pci_nvme_softc *sc,
> > char *o
> >
> > [...]
> >
> > >   memset(sc->ctrldata.sn, 0, sizeof(sc->ctrldata.sn
> > ));
> > >   strncpy(sc->ctrldata.sn, config,
> > >   sizeof(sc->ctrldata.sn));
> >
> > This memset is unneeded, as strncpy will write NUL bytes to fill out
> > the buffer:
> > If src is less than len characters long, the remainder of
> >  dst is filled with `\0' characters.
> >
> 
> It also looks like the comment was wrong. The newest NVMe standards say
> these fields should be 7-bit and space-padded.

lol, which is what the vendor that caused me grief with
ata serial numbers did decades ago.

-- 
Rod Grimes rgri...@freebsd.org
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Warner Losh]

On Thu, Aug 16, 2018 at 11:06 AM, John-Mark Gurney  wrote:

> Marcelo Araujo wrote this message on Thu, Aug 16, 2018 at 06:31 +:
> > 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.cThu Aug 16 06:20:25 2018
> (r337886)
> > +++ head/usr.sbin/bhyve/pci_nvme.cThu Aug 16 06:31:54 2018
> (r337887)
> > @@ -1714,6 +1714,11 @@ pci_nvme_parse_opts(struct pci_nvme_softc *sc,
> char *o
>
> [...]
>
> >   memset(sc->ctrldata.sn, 0, sizeof(sc->ctrldata.sn
> ));
> >   strncpy(sc->ctrldata.sn, config,
> >   sizeof(sc->ctrldata.sn));
>
> This memset is unneeded, as strncpy will write NUL bytes to fill out
> the buffer:
> If src is less than len characters long, the remainder of
>  dst is filled with `\0' characters.
>

It also looks like the comment was wrong. The newest NVMe standards say
these fields should be 7-bit and space-padded.

Warner
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[John-Mark Gurney]

Marcelo Araujo wrote this message on Thu, Aug 16, 2018 at 06:31 +:
> 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.cThu Aug 16 06:20:25 2018
> (r337886)
> +++ head/usr.sbin/bhyve/pci_nvme.cThu Aug 16 06:31:54 2018
> (r337887)
> @@ -1714,6 +1714,11 @@ pci_nvme_parse_opts(struct pci_nvme_softc *sc, char *o

[...]

>   memset(sc->ctrldata.sn, 0, sizeof(sc->ctrldata.sn));
>   strncpy(sc->ctrldata.sn, config,
>   sizeof(sc->ctrldata.sn));

This memset is unneeded, as strncpy will write NUL bytes to fill out
the buffer:
If src is less than len characters long, the remainder of
 dst is filled with `\0' characters.

-- 
  John-Mark Gurney  Voice: +1 415 225 5579

 "All that I will do, has been done, All that I have, has not."
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Warner Losh]

On Thu, Aug 16, 2018 at 8:34 AM, Rodney W. Grimes <
free...@pdx.rh.cn85.dnsmgr.net> wrote:

> > On Thu, Aug 16, 2018 at 8:03 AM, Rodney W. Grimes <
> > free...@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.cThu Aug 16 06:20:25 2018
> > > (r337886)
> > > > +++ head/usr.sbin/bhyve/pci_nvme.cThu 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?


Grep says "Yes."

Warner
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Rodney W. Grimes]

> On Thu, Aug 16, 2018 at 8:03 AM, Rodney W. Grimes <
> free...@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.cThu Aug 16 06:20:25 2018
> > (r337886)
> > > +++ head/usr.sbin/bhyve/pci_nvme.cThu 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 rgri...@freebsd.org
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Warner Losh]

On Thu, Aug 16, 2018 at 8:03 AM, Rodney W. Grimes <
free...@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.cThu Aug 16 06:20:25 2018
> (r337886)
> > +++ head/usr.sbin/bhyve/pci_nvme.cThu 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). Keep in mind, though, that ATA is 100% irrelevant to NVMe,
since the NVMe standard specifies everything.

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

Warner
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Rodney W. Grimes]

> 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.cThu Aug 16 06:20:25 2018
> (r337886)
> +++ head/usr.sbin/bhyve/pci_nvme.cThu 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));
> 
> 

-- 
Rod Grimes rgri...@freebsd.org
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

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

[Marcelo Araujo]

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.
+*/
memset(sc->ctrldata.sn, 0, sizeof(sc->ctrldata.sn));
strncpy(sc->ctrldata.sn, config,
sizeof(sc->ctrldata.sn));
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"