| Message ID | 1412103942-28331-4-git-send-email-eric.nelson@boundarydevices.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On 14-09-30 12:05 PM, Eric Nelson wrote: > Without NULL termination, various commands will read past the > end of input. In particular, this was noticed with error() > calls in cb_getvar and simple_strtoul() in cb_download. > > Since the download callback happens elsewhere, the 4k buffer > should always be sufficient to handle command arguments. > > Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com> > --- > drivers/usb/gadget/f_fastboot.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c > index 86700f5..0950ea8 100644 > --- a/drivers/usb/gadget/f_fastboot.c > +++ b/drivers/usb/gadget/f_fastboot.c > @@ -542,6 +542,13 @@ static void rx_handler_command(struct usb_ep *ep, struct usb_request *req) > error("unknown command: %s\n", cmdbuf); > fastboot_tx_write_str("FAILunknown command"); > } else { > + if (req->actual < req->length) { > + u8 *buf = (u8 *)req->buf; > + buf[req->actual] = 0; > + func_cb(ep, req); > + } else { > + error("buffer overflow\n"); fastboot_tx_write_str("FAILbuffer overflow"); ADD this line > + } > func_cb(ep, req); AND delete this line (otherwise the func_cb() is called twice!!!) > } > > I have not experienced this issue, however, if it is to be accepted, then please update these two lines.... Afterwards: Tested-by: Steve Rae <srae@broadcom.com>
Thanks Steve, On 10/01/2014 01:40 PM, Steve Rae wrote: > > > On 14-09-30 12:05 PM, Eric Nelson wrote: >> Without NULL termination, various commands will read past the >> end of input. In particular, this was noticed with error() >> calls in cb_getvar and simple_strtoul() in cb_download. >> >> Since the download callback happens elsewhere, the 4k buffer >> should always be sufficient to handle command arguments. >> >> Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com> >> --- >> drivers/usb/gadget/f_fastboot.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/drivers/usb/gadget/f_fastboot.c >> b/drivers/usb/gadget/f_fastboot.c >> index 86700f5..0950ea8 100644 >> --- a/drivers/usb/gadget/f_fastboot.c >> +++ b/drivers/usb/gadget/f_fastboot.c >> @@ -542,6 +542,13 @@ static void rx_handler_command(struct usb_ep *ep, >> struct usb_request *req) >> error("unknown command: %s\n", cmdbuf); >> fastboot_tx_write_str("FAILunknown command"); >> } else { >> + if (req->actual < req->length) { >> + u8 *buf = (u8 *)req->buf; >> + buf[req->actual] = 0; >> + func_cb(ep, req); >> + } else { >> + error("buffer overflow\n"); > fastboot_tx_write_str("FAILbuffer overflow"); > ADD this line >> + } >> func_cb(ep, req); > AND delete this line (otherwise the func_cb() is called twice!!!) >> } >> Ouch. It appears I pooched the patch when trying to make checkpatch happy with the extra(neous) braces. I'll forward V2 shortly. Regards, Eric
diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index 86700f5..0950ea8 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -542,6 +542,13 @@ static void rx_handler_command(struct usb_ep *ep, struct usb_request *req) error("unknown command: %s\n", cmdbuf); fastboot_tx_write_str("FAILunknown command"); } else { + if (req->actual < req->length) { + u8 *buf = (u8 *)req->buf; + buf[req->actual] = 0; + func_cb(ep, req); + } else { + error("buffer overflow\n"); + } func_cb(ep, req); }
Without NULL termination, various commands will read past the end of input. In particular, this was noticed with error() calls in cb_getvar and simple_strtoul() in cb_download. Since the download callback happens elsewhere, the 4k buffer should always be sufficient to handle command arguments. Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com> --- drivers/usb/gadget/f_fastboot.c | 7 +++++++ 1 file changed, 7 insertions(+)