Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755245AbbEZN46 (ORCPT ); Tue, 26 May 2015 09:56:58 -0400 Received: from userp1040.oracle.com ([156.151.31.81]:38336 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753339AbbEZN4x (ORCPT ); Tue, 26 May 2015 09:56:53 -0400 Date: Tue, 26 May 2015 16:56:35 +0300 From: Dan Carpenter To: "Jason A. Donenfeld" Cc: Greg Kroah-Hartman , devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 1/4] ozwpan: Use proper check to prevent heap overflow Message-ID: <20150526135635.GH11588@mwanda> References: <1431543500-4847-1-git-send-email-Jason@zx2c4.com> <1432642669-7289-1-git-send-email-Jason@zx2c4.com> <1432642669-7289-2-git-send-email-Jason@zx2c4.com> <20150526133221.GG11588@mwanda> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-Source-IP: userv0022.oracle.com [156.151.31.74] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1876 Lines: 51 On Tue, May 26, 2015 at 03:49:27PM +0200, Jason A. Donenfeld wrote: > On Tue, May 26, 2015 at 3:32 PM, Dan Carpenter wrote: > > On Tue, May 26, 2015 at 02:17:46PM +0200, Jason A. Donenfeld wrote: > >> + data_len = elt->length - > >> sizeof(struct oz_get_desc_rsp) + 1; > > > > This was in the original code, but I wonder where the + 1 comes from. > > Does anyone know? > > I know. It's because oz_get_desc_rsp has a 1 byte data member as it's > last element, that's just meant as a placeholder for a variable amount > of data. elt->length is supposed to be the size of the struct elements > plus the total data section, which runs after the struct. But because > of this placeholder goofiness, when we take sizeof we have to subtract > one. > > struct oz_get_desc_rsp { > [... bla bla ...] > u8 data[1]; > } PACKED; > > This is sort of horrible, but it is what it is. I'd recommend these > security-CRITICAL patches get merged immediately, and then cleaning up > other problems with this driver can be addressed after, preferably by > the maintainer. > Ah thanks. You are right on all counts, let's merge this. > > > > > To be honest, I would prefer if we just checked: > > > > if (elt->length < sizeof(struct oz_get_desc_rsp) + 1) > > return; > > data_len = elt->length - sizeof(struct oz_get_desc_rsp) + 1; > > > > Shouldn't there be an upper bound on length? Shigekatsu? > > elt->length is a u8, so the upper bound is 255. Yes. I know that, but is 255 correct? regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/