On Tue, Jan 31, 2017 at 05:38:58PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Jan 31, 2017 at 05:17:27PM +0100, Johan Hovold wrote:
> > Make sure to check for short transfers to avoid underflow in a loop
> > condition when parsing the receive buffer.
> >
> > Also fix an off-by-one error in the incomplete sanity check which could
> > lead to invalid data being parsed.
> >
> > Fixes: 8c209e6782ca ("USB: make actual_length in struct urb field u32")
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Cc: stable <[email protected]> # v2.6.30
> > Signed-off-by: Johan Hovold <[email protected]>
> > ---
> > drivers/usb/serial/digi_acceleport.c | 14 +++++++++-----
> > 1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/usb/serial/digi_acceleport.c
> > b/drivers/usb/serial/digi_acceleport.c
> > index 6a1df9e824ca..3b610f1e3f7c 100644
> > --- a/drivers/usb/serial/digi_acceleport.c
> > +++ b/drivers/usb/serial/digi_acceleport.c
> > @@ -1482,16 +1482,20 @@ static int digi_read_oob_callback(struct urb *urb)
> > struct usb_serial *serial = port->serial;
> > struct tty_struct *tty;
> > struct digi_port *priv = usb_get_serial_port_data(port);
> > + unsigned char *buf = urb->transfer_buffer;
> > int opcode, line, status, val;
> > int i;
> > unsigned int rts;
> >
> > + if (urb->actual_length < 4)
> > + return -1;
>
> A real error number perhaps?
Yeah, that was my inclination too, but the function is documented as
returning -1 when the sanity checks fail so I stuck to that for now:
/*
* Digi Read OOB Callback
*
* Digi Read OOB Callback handles reads on the out of band port.
* When called we know port and port->private are not NULL and
* the port->serial is valid. It returns 0 if successful, and
* -1 if the sanity checks failed.
*/
> > +
> > /* handle each oob command */
> > - for (i = 0; i < urb->actual_length - 3;) {
> > - opcode = ((unsigned char *)urb->transfer_buffer)[i++];
> > - line = ((unsigned char *)urb->transfer_buffer)[i++];
> > - status = ((unsigned char *)urb->transfer_buffer)[i++];
> > - val = ((unsigned char *)urb->transfer_buffer)[i++];
> > + for (i = 0; i < urb->actual_length - 4; i += 4) {
> > + opcode = buf[i];
> > + line = buf[i + 1];
> > + status = buf[i + 2];
> > + val = buf[i + 3];
>
> Other than that nit, looks good, thanks for fixing my really old bugs :)
>
> Reviewed-by: Greg Kroah-Hartman <[email protected]>
Thanks for the review.
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
