[U-Boot] [PATCH 3/3] sandbox: sandbox_flash.c: Ensure NUL-termination on product/vendor strings

[Simon Glass]

Hi Tom,

On 8 December 2015 at 16:58, Tom Rini <trini at konsulko.com> wrote:
>
> On Tue, Dec 08, 2015 at 04:42:17PM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Dec 8, 2015 4:28 PM, "Tom Rini" <trini at konsulko.com> wrote:
> > >
> > > On Tue, Dec 08, 2015 at 12:35:23PM -0700, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On 7 December 2015 at 20:26, Tom Rini <trini at konsulko.com> wrote:
> > > > > Coverity notes that we do not ensure when we copy in the
> > product/vendor
> > > > > strings that they have NULL termination.  In this case the answer is
> > to
> > > > > increase the buffer we have and then set the last entry to NULL.
> > > > >
> > > > > Reported-by: Coverity (CID 131095)
> > > > > Cc: Simon Glass <sjg at chromium.org>
> > > > > Signed-off-by: Tom Rini <trini at konsulko.com>
> > > > > ---
> > > > >  drivers/usb/emul/sandbox_flash.c |    6 ++++--
> > > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/usb/emul/sandbox_flash.c
> > b/drivers/usb/emul/sandbox_flash.c
> > > > > index 0965ad0..2811f52 100644
> > > > > --- a/drivers/usb/emul/sandbox_flash.c
> > > > > +++ b/drivers/usb/emul/sandbox_flash.c
> > > > > @@ -79,8 +79,8 @@ struct scsi_inquiry_resp {
> > > > >         u8 data_format;
> > > > >         u8 additional_len;
> > > > >         u8 spare[3];
> > > > > -       char vendor[8];
> > > > > -       char product[16];
> > > > > +       char vendor[9]; /* 8 + NUL-termination */
> > > > > +       char product[17]; /* 16 + NUL-termination */
> > > > >         char revision[4];
> > > > >  };
> > > > >
> > > > > @@ -260,6 +260,8 @@ static int handle_ufi_command(struct
> > sandbox_flash_plat *plat,
> > > > >                 strncpy(resp->product,
> > > > >                         plat->flash_strings[STRINGID_PRODUCT - 1].s,
> > > > >                         sizeof(resp->product));
> > > > > +               resp->vendor[8] = 0;
> > > > > +               resp->product[16] = 0;
> > > > >                 strncpy(resp->revision, "1.0",
> > sizeof(resp->revision));
> > > > >                 setup_response(priv, resp, sizeof(*resp));
> > > > >                 break;
> > > > > --
> > > > > 1.7.9.5
> > > > >
> > > >
> > > > Does this work? It seems like it is changing the SCSI protocol spec,
> > > > which we can't do.
> > >
> > > Yes.  We memset resp to NULL to start with so we know it will always be
> > > NULL terminated now.  Elsewhere in our SCSI code is where I got the
> > > idea, common/usb_storage.c::usb_stor_get_info for example.
> >
> > Yes but you are changing the struct.
>
> Yeah, true too.  I'm having a bit of trouble tracing how all of this
> ties back into where in the rest of the stack.  Can you figure out if
> this is safe or if we should NULL terminate [7]/etc or what?  Thanks!

See usb_inquiry() which issues the request and usb_stor_get_info()
which processes the response.. Sandbox needs to send the response back
in the expected format or it will not work. You can run the driver
model usb tests to check. However at present there are three separate
test breakages in driver model...

Regards,
Simon