From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751314AbdBXRzR (ORCPT ); Fri, 24 Feb 2017 12:55:17 -0500 Received: from mail.linuxfoundation.org ([140.211.169.12]:38982 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751253AbdBXRzK (ORCPT ); Fri, 24 Feb 2017 12:55:10 -0500 Date: Fri, 24 Feb 2017 18:55:02 +0100 From: Greg Kroah-Hartman To: Johan Hovold Cc: Ben Hutchings , linux-kernel@vger.kernel.org, stable@vger.kernel.org Subject: Re: [PATCH 4.4 17/25] USB: serial: digi_acceleport: fix OOB data sanity check Message-ID: <20170224175502.GA23284@kroah.com> References: <20170224082128.156304123@linuxfoundation.org> <20170224082130.561381811@linuxfoundation.org> <1487943505.26310.4.camel@decadent.org.uk> <20170224173304.GA2631@localhost> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20170224173304.GA2631@localhost> User-Agent: Mutt/1.7.2 (2016-11-26) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Feb 24, 2017 at 06:33:04PM +0100, Johan Hovold wrote: > On Fri, Feb 24, 2017 at 01:38:25PM +0000, 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