Re: [PATCH 4.4 17/25] USB: serial: digi_acceleport: fix OOB data sanity check

[Greg Kroah-Hartman]

On Mon, Mar 13, 2017 at 06:14:37PM +0100, Johan Hovold wrote:
> On Fri, Feb 24, 2017 at 06:55:02PM +0100, Greg Kroah-Hartman wrote:
> > On Fri, Feb 24, 2017 at 06:33:04PM +0100, Johan Hovold wrote:
> > > On Fri, Feb 24, 2017 at 01:38:25PM +, Ben Hutchings wrote: > On Fri, 
> > > 2017-02-24 at 09:25 +0100, Greg Kroah-Hartman wrote:
> > > > > 4.4-stable review patch.  If anyone has any objections, please let me 
> > > > > know.
> > > > > 
> > > > > --
> > > > > 
> > > > > From: Johan Hovold 
> > > > > 
> > > > > commit 2d380889215fe20b8523345649dee0579821800c upstream.
> > > > > 
> > > > > 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.
> > > > 
> > > > This appears to *introduce* an off-by-one.  Which is not as serious as
> > > > the underflow, but is still a regression.
> > > > 
> > > > Suppose we have urb->actual_length == 4:
> > > > 
> > > > [...]
> > > > > - for (i = 0; i < urb->actual_length - 3;) {
> > > > 
> > > > i < 1 is true, so we would run the loop once.
> > > > 
> > > > > - 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) {
> > > > 
> > > > i < 0 is false, so we now skip the loop.
> > > 
> > > Good catch, thanks! The original loop condition was indeed correct
> > > (modulo the missing underflow check), and I'll post a follow-up fix to
> > > address this.
> > > 
> > > > > + opcode = buf[i];
> > > > > + line = buf[i + 1];
> > > > > + status = buf[i + 2];
> > > > > + val = buf[i + 3];
> > > 
> > > You should probably not apply this one until after the follow-up is in
> > > Linus' tree as this patch breaks TIOCMGET.
> > 
> > Ok, I'll drop this one from the stable tree now.  Remind me to pick this
> > one up when the fixup hits Linus's tree.
> 
> The follow-up fix is now in Linus's tree so both this one:
> 
>   2d380889215f ("USB: serial: digi_acceleport: fix OOB data sanity check")
> 
> and the follow-up:
> 
>   2e46565cf622 ("USB: serial: digi_acceleport: fix OOB-event processing")
> 
> can now be applied to the stable trees.

Thanks, now queued up.

greg k-h

Re: [PATCH 4.4 17/25] USB: serial: digi_acceleport: fix OOB data sanity check

[Greg Kroah-Hartman]

On Mon, Mar 13, 2017 at 06:14:37PM +0100, Johan Hovold wrote:
> On Fri, Feb 24, 2017 at 06:55:02PM +0100, Greg Kroah-Hartman wrote:
> > On Fri, Feb 24, 2017 at 06:33:04PM +0100, Johan Hovold wrote:
> > > On Fri, Feb 24, 2017 at 01:38:25PM +, Ben Hutchings wrote: > On Fri, 
> > > 2017-02-24 at 09:25 +0100, Greg Kroah-Hartman wrote:
> > > > > 4.4-stable review patch.  If anyone has any objections, please let me 
> > > > > know.
> > > > > 
> > > > > --
> > > > > 
> > > > > From: Johan Hovold 
> > > > > 
> > > > > commit 2d380889215fe20b8523345649dee0579821800c upstream.
> > > > > 
> > > > > 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.
> > > > 
> > > > This appears to *introduce* an off-by-one.  Which is not as serious as
> > > > the underflow, but is still a regression.
> > > > 
> > > > Suppose we have urb->actual_length == 4:
> > > > 
> > > > [...]
> > > > > - for (i = 0; i < urb->actual_length - 3;) {
> > > > 
> > > > i < 1 is true, so we would run the loop once.
> > > > 
> > > > > - 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) {
> > > > 
> > > > i < 0 is false, so we now skip the loop.
> > > 
> > > Good catch, thanks! The original loop condition was indeed correct
> > > (modulo the missing underflow check), and I'll post a follow-up fix to
> > > address this.
> > > 
> > > > > + opcode = buf[i];
> > > > > + line = buf[i + 1];
> > > > > + status = buf[i + 2];
> > > > > + val = buf[i + 3];
> > > 
> > > You should probably not apply this one until after the follow-up is in
> > > Linus' tree as this patch breaks TIOCMGET.
> > 
> > Ok, I'll drop this one from the stable tree now.  Remind me to pick this
> > one up when the fixup hits Linus's tree.
> 
> The follow-up fix is now in Linus's tree so both this one:
> 
>   2d380889215f ("USB: serial: digi_acceleport: fix OOB data sanity check")
> 
> and the follow-up:
> 
>   2e46565cf622 ("USB: serial: digi_acceleport: fix OOB-event processing")
> 
> can now be applied to the stable trees.

Thanks, now queued up.

greg k-h

Re: [PATCH 4.4 17/25] USB: serial: digi_acceleport: fix OOB data sanity check

[Johan Hovold]

On Fri, Feb 24, 2017 at 06:55:02PM +0100, Greg Kroah-Hartman wrote:
> On Fri, Feb 24, 2017 at 06:33:04PM +0100, Johan Hovold wrote:
> > On Fri, Feb 24, 2017 at 01:38:25PM +, Ben Hutchings wrote: > On Fri, 
> > 2017-02-24 at 09:25 +0100, Greg Kroah-Hartman wrote:
> > > > 4.4-stable review patch.  If anyone has any objections, please let me 
> > > > know.
> > > > 
> > > > --
> > > > 
> > > > From: Johan Hovold 
> > > > 
> > > > commit 2d380889215fe20b8523345649dee0579821800c upstream.
> > > > 
> > > > 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.
> > > 
> > > This appears to *introduce* an off-by-one.  Which is not as serious as
> > > the underflow, but is still a regression.
> > > 
> > > Suppose we have urb->actual_length == 4:
> > > 
> > > [...]
> > > > -   for (i = 0; i < urb->actual_length - 3;) {
> > > 
> > > i < 1 is true, so we would run the loop once.
> > > 
> > > > -   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) {
> > > 
> > > i < 0 is false, so we now skip the loop.
> > 
> > Good catch, thanks! The original loop condition was indeed correct
> > (modulo the missing underflow check), and I'll post a follow-up fix to
> > address this.
> > 
> > > > +   opcode = buf[i];
> > > > +   line = buf[i + 1];
> > > > +   status = buf[i + 2];
> > > > +   val = buf[i + 3];
> > 
> > You should probably not apply this one until after the follow-up is in
> > Linus' tree as this patch breaks TIOCMGET.
> 
> Ok, I'll drop this one from the stable tree now.  Remind me to pick this
> one up when the fixup hits Linus's tree.

The follow-up fix is now in Linus's tree so both this one:

2d380889215f ("USB: serial: digi_acceleport: fix OOB data sanity check")

and the follow-up:

2e46565cf622 ("USB: serial: digi_acceleport: fix OOB-event processing")

can now be applied to the stable trees.

Thanks,
Johan

Re: [PATCH 4.4 17/25] USB: serial: digi_acceleport: fix OOB data sanity check

[Johan Hovold]

On Fri, Feb 24, 2017 at 06:55:02PM +0100, Greg Kroah-Hartman wrote:
> On Fri, Feb 24, 2017 at 06:33:04PM +0100, Johan Hovold wrote:
> > On Fri, Feb 24, 2017 at 01:38:25PM +, Ben Hutchings wrote: > On Fri, 
> > 2017-02-24 at 09:25 +0100, Greg Kroah-Hartman wrote:
> > > > 4.4-stable review patch.  If anyone has any objections, please let me 
> > > > know.
> > > > 
> > > > --
> > > > 
> > > > From: Johan Hovold 
> > > > 
> > > > commit 2d380889215fe20b8523345649dee0579821800c upstream.
> > > > 
> > > > 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.
> > > 
> > > This appears to *introduce* an off-by-one.  Which is not as serious as
> > > the underflow, but is still a regression.
> > > 
> > > Suppose we have urb->actual_length == 4:
> > > 
> > > [...]
> > > > -   for (i = 0; i < urb->actual_length - 3;) {
> > > 
> > > i < 1 is true, so we would run the loop once.
> > > 
> > > > -   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) {
> > > 
> > > i < 0 is false, so we now skip the loop.
> > 
> > Good catch, thanks! The original loop condition was indeed correct
> > (modulo the missing underflow check), and I'll post a follow-up fix to
> > address this.
> > 
> > > > +   opcode = buf[i];
> > > > +   line = buf[i + 1];
> > > > +   status = buf[i + 2];
> > > > +   val = buf[i + 3];
> > 
> > You should probably not apply this one until after the follow-up is in
> > Linus' tree as this patch breaks TIOCMGET.
> 
> Ok, I'll drop this one from the stable tree now.  Remind me to pick this
> one up when the fixup hits Linus's tree.

The follow-up fix is now in Linus's tree so both this one:

2d380889215f ("USB: serial: digi_acceleport: fix OOB data sanity check")

and the follow-up:

2e46565cf622 ("USB: serial: digi_acceleport: fix OOB-event processing")

can now be applied to the stable trees.

Thanks,
Johan

Re: [PATCH 4.4 17/25] USB: serial: digi_acceleport: fix OOB data sanity check

[Johan Hovold]

On Fri, Feb 24, 2017 at 06:55:02PM +0100, Greg Kroah-Hartman wrote:
> On Fri, Feb 24, 2017 at 06:33:04PM +0100, Johan Hovold wrote:
> > On Fri, Feb 24, 2017 at 01:38:25PM +, Ben Hutchings wrote: > On Fri, 
> > 2017-02-24 at 09:25 +0100, Greg Kroah-Hartman wrote:
> > > > 4.4-stable review patch.  If anyone has any objections, please let me 
> > > > know.
> > > > 
> > > > --
> > > > 
> > > > From: Johan Hovold 
> > > > 
> > > > commit 2d380889215fe20b8523345649dee0579821800c upstream.
> > > > 
> > > > 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.
> > > 
> > > This appears to *introduce* an off-by-one.  Which is not as serious as
> > > the underflow, but is still a regression.

> > Good catch, thanks! The original loop condition was indeed correct
> > (modulo the missing underflow check), and I'll post a follow-up fix to
> > address this.

> > You should probably not apply this one until after the follow-up is in
> > Linus' tree as this patch breaks TIOCMGET.
> 
> Ok, I'll drop this one from the stable tree now.  Remind me to pick this
> one up when the fixup hits Linus's tree.

Thanks, will do.

Johan

Re: [PATCH 4.4 17/25] USB: serial: digi_acceleport: fix OOB data sanity check

[Johan Hovold]

On Fri, Feb 24, 2017 at 06:55:02PM +0100, Greg Kroah-Hartman wrote:
> On Fri, Feb 24, 2017 at 06:33:04PM +0100, Johan Hovold wrote:
> > On Fri, Feb 24, 2017 at 01:38:25PM +, Ben Hutchings wrote: > On Fri, 
> > 2017-02-24 at 09:25 +0100, Greg Kroah-Hartman wrote:
> > > > 4.4-stable review patch.  If anyone has any objections, please let me 
> > > > know.
> > > > 
> > > > --
> > > > 
> > > > From: Johan Hovold 
> > > > 
> > > > commit 2d380889215fe20b8523345649dee0579821800c upstream.
> > > > 
> > > > 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.
> > > 
> > > This appears to *introduce* an off-by-one.  Which is not as serious as
> > > the underflow, but is still a regression.

> > Good catch, thanks! The original loop condition was indeed correct
> > (modulo the missing underflow check), and I'll post a follow-up fix to
> > address this.

> > You should probably not apply this one until after the follow-up is in
> > Linus' tree as this patch breaks TIOCMGET.
> 
> Ok, I'll drop this one from the stable tree now.  Remind me to pick this
> one up when the fixup hits Linus's tree.

Thanks, will do.

Johan

Re: [PATCH 4.4 17/25] USB: serial: digi_acceleport: fix OOB data sanity check

[Greg Kroah-Hartman]

On Fri, Feb 24, 2017 at 06:33:04PM +0100, Johan Hovold wrote:
> On Fri, Feb 24, 2017 at 01:38:25PM +, Ben Hutchings wrote: > On Fri, 
> 2017-02-24 at 09:25 +0100, Greg Kroah-Hartman wrote:
> > > 4.4-stable review patch.  If anyone has any objections, please let me 
> > > know.
> > > 
> > > --
> > > 
> > > From: Johan Hovold 
> > > 
> > > commit 2d380889215fe20b8523345649dee0579821800c upstream.
> > > 
> > > 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.
> > 
> > This appears to *introduce* an off-by-one.  Which is not as serious as
> > the underflow, but is still a regression.
> > 
> > Suppose we have urb->actual_length == 4:
> > 
> > [...]
> > > - for (i = 0; i < urb->actual_length - 3;) {
> > 
> > i < 1 is true, so we would run the loop once.
> > 
> > > - 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) {
> > 
> > i < 0 is false, so we now skip the loop.
> 
> Good catch, thanks! The original loop condition was indeed correct
> (modulo the missing underflow check), and I'll post a follow-up fix to
> address this.
> 
> > > + opcode = buf[i];
> > > + line = buf[i + 1];
> > > + status = buf[i + 2];
> > > + val = buf[i + 3];
> 
> You should probably not apply this one until after the follow-up is in
> Linus' tree as this patch breaks TIOCMGET.

Ok, I'll drop this one from the stable tree now.  Remind me to pick this
one up when the fixup hits Linus's tree.

thanks,

greg k-h

Re: [PATCH 4.4 17/25] USB: serial: digi_acceleport: fix OOB data sanity check

[Greg Kroah-Hartman]

On Fri, Feb 24, 2017 at 06:33:04PM +0100, Johan Hovold wrote:
> On Fri, Feb 24, 2017 at 01:38:25PM +, Ben Hutchings wrote: > On Fri, 
> 2017-02-24 at 09:25 +0100, Greg Kroah-Hartman wrote:
> > > 4.4-stable review patch.  If anyone has any objections, please let me 
> > > know.
> > > 
> > > --
> > > 
> > > From: Johan Hovold 
> > > 
> > > commit 2d380889215fe20b8523345649dee0579821800c upstream.
> > > 
> > > 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.
> > 
> > This appears to *introduce* an off-by-one.  Which is not as serious as
> > the underflow, but is still a regression.
> > 
> > Suppose we have urb->actual_length == 4:
> > 
> > [...]
> > > - for (i = 0; i < urb->actual_length - 3;) {
> > 
> > i < 1 is true, so we would run the loop once.
> > 
> > > - 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) {
> > 
> > i < 0 is false, so we now skip the loop.
> 
> Good catch, thanks! The original loop condition was indeed correct
> (modulo the missing underflow check), and I'll post a follow-up fix to
> address this.
> 
> > > + opcode = buf[i];
> > > + line = buf[i + 1];
> > > + status = buf[i + 2];
> > > + val = buf[i + 3];
> 
> You should probably not apply this one until after the follow-up is in
> Linus' tree as this patch breaks TIOCMGET.

Ok, I'll drop this one from the stable tree now.  Remind me to pick this
one up when the fixup hits Linus's tree.

thanks,

greg k-h

Re: [PATCH 4.4 17/25] USB: serial: digi_acceleport: fix OOB data sanity check

[Johan Hovold]

On Fri, Feb 24, 2017 at 01:38:25PM +, Ben Hutchings wrote: > On Fri, 
2017-02-24 at 09:25 +0100, Greg Kroah-Hartman wrote:
> > 4.4-stable review patch.  If anyone has any objections, please let me know.
> > 
> > --
> > 
> > From: Johan Hovold 
> > 
> > commit 2d380889215fe20b8523345649dee0579821800c upstream.
> > 
> > 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.
> 
> This appears to *introduce* an off-by-one.  Which is not as serious as
> the underflow, but is still a regression.
> 
> Suppose we have urb->actual_length == 4:
> 
> [...]
> > -   for (i = 0; i < urb->actual_length - 3;) {
> 
> i < 1 is true, so we would run the loop once.
> 
> > -   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) {
> 
> i < 0 is false, so we now skip the loop.

Good catch, thanks! The original loop condition was indeed correct
(modulo the missing underflow check), and I'll post a follow-up fix to
address this.

> > +   opcode = buf[i];
> > +   line = buf[i + 1];
> > +   status = buf[i + 2];
> > +   val = buf[i + 3];

You should probably not apply this one until after the follow-up is in
Linus' tree as this patch breaks TIOCMGET.

Thanks,
Johan

Re: [PATCH 4.4 17/25] USB: serial: digi_acceleport: fix OOB data sanity check

[Johan Hovold]

On Fri, Feb 24, 2017 at 01:38:25PM +, Ben Hutchings wrote: > On Fri, 
2017-02-24 at 09:25 +0100, Greg Kroah-Hartman wrote:
> > 4.4-stable review patch.  If anyone has any objections, please let me know.
> > 
> > --
> > 
> > From: Johan Hovold 
> > 
> > commit 2d380889215fe20b8523345649dee0579821800c upstream.
> > 
> > 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.
> 
> This appears to *introduce* an off-by-one.  Which is not as serious as
> the underflow, but is still a regression.
> 
> Suppose we have urb->actual_length == 4:
> 
> [...]
> > -   for (i = 0; i < urb->actual_length - 3;) {
> 
> i < 1 is true, so we would run the loop once.
> 
> > -   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) {
> 
> i < 0 is false, so we now skip the loop.

Good catch, thanks! The original loop condition was indeed correct
(modulo the missing underflow check), and I'll post a follow-up fix to
address this.

> > +   opcode = buf[i];
> > +   line = buf[i + 1];
> > +   status = buf[i + 2];
> > +   val = buf[i + 3];

You should probably not apply this one until after the follow-up is in
Linus' tree as this patch breaks TIOCMGET.

Thanks,
Johan

Re: [PATCH 4.4 17/25] USB: serial: digi_acceleport: fix OOB data sanity check

[Ben Hutchings]

On Fri, 2017-02-24 at 09:25 +0100, Greg Kroah-Hartman wrote:
> 4.4-stable review patch.  If anyone has any objections, please let me know.
> 
> --
> 
> From: Johan Hovold 
> 
> commit 2d380889215fe20b8523345649dee0579821800c upstream.
> 
> 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.

This appears to *introduce* an off-by-one.  Which is not as serious as
the underflow, but is still a regression.

Suppose we have urb->actual_length == 4:

[...]
> - for (i = 0; i < urb->actual_length - 3;) {

i < 1 is true, so we would run the loop once.

> - 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) {

i < 0 is false, so we now skip the loop.

> + opcode = buf[i];
> + line = buf[i + 1];
> + status = buf[i + 2];
> + val = buf[i + 3];
[...]

Ben.

-- 
Ben Hutchings
All the simple programs have been written, and all the good names
taken.



signature.asc
Description: This is a digitally signed message part

Re: [PATCH 4.4 17/25] USB: serial: digi_acceleport: fix OOB data sanity check

[Ben Hutchings]

On Fri, 2017-02-24 at 09:25 +0100, Greg Kroah-Hartman wrote:
> 4.4-stable review patch.  If anyone has any objections, please let me know.
> 
> --
> 
> From: Johan Hovold 
> 
> commit 2d380889215fe20b8523345649dee0579821800c upstream.
> 
> 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.

This appears to *introduce* an off-by-one.  Which is not as serious as
the underflow, but is still a regression.

Suppose we have urb->actual_length == 4:

[...]
> - for (i = 0; i < urb->actual_length - 3;) {

i < 1 is true, so we would run the loop once.

> - 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) {

i < 0 is false, so we now skip the loop.

> + opcode = buf[i];
> + line = buf[i + 1];
> + status = buf[i + 2];
> + val = buf[i + 3];
[...]

Ben.

-- 
Ben Hutchings
All the simple programs have been written, and all the good names
taken.



signature.asc
Description: This is a digitally signed message part

[PATCH 4.4 17/25] USB: serial: digi_acceleport: fix OOB data sanity check

[Greg Kroah-Hartman]

4.4-stable review patch.  If anyone has any objections, please let me know.

--

From: Johan Hovold 

commit 2d380889215fe20b8523345649dee0579821800c upstream.

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")
Reviewed-by: Greg Kroah-Hartman 
Signed-off-by: Johan Hovold 
Signed-off-by: Greg Kroah-Hartman 

---
 drivers/usb/serial/digi_acceleport.c |   14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

--- a/drivers/usb/serial/digi_acceleport.c
+++ b/drivers/usb/serial/digi_acceleport.c
@@ -1483,16 +1483,20 @@ static int digi_read_oob_callback(struct
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;
+
/* 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];
 
dev_dbg(>dev, "digi_read_oob_callback: opcode=%d, 
line=%d, status=%d, val=%d\n",
opcode, line, status, val);

[PATCH 4.4 17/25] USB: serial: digi_acceleport: fix OOB data sanity check

[Greg Kroah-Hartman]

4.4-stable review patch.  If anyone has any objections, please let me know.

--

From: Johan Hovold 

commit 2d380889215fe20b8523345649dee0579821800c upstream.

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")
Reviewed-by: Greg Kroah-Hartman 
Signed-off-by: Johan Hovold 
Signed-off-by: Greg Kroah-Hartman 

---
 drivers/usb/serial/digi_acceleport.c |   14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

--- a/drivers/usb/serial/digi_acceleport.c
+++ b/drivers/usb/serial/digi_acceleport.c
@@ -1483,16 +1483,20 @@ static int digi_read_oob_callback(struct
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;
+
/* 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];
 
dev_dbg(>dev, "digi_read_oob_callback: opcode=%d, 
line=%d, status=%d, val=%d\n",
opcode, line, status, val);