Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755184AbbEZNtm (ORCPT ); Tue, 26 May 2015 09:49:42 -0400 Received: from frisell.zx2c4.com ([192.95.5.64]:39402 "EHLO frisell.zx2c4.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754901AbbEZNtg (ORCPT ); Tue, 26 May 2015 09:49:36 -0400 MIME-Version: 1.0 In-Reply-To: <20150526133221.GG11588@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> Date: Tue, 26 May 2015 15:49:27 +0200 Message-ID: Subject: Re: [PATCH v2 1/4] ozwpan: Use proper check to prevent heap overflow From: "Jason A. Donenfeld" To: Dan Carpenter Cc: Shigekatsu Tateno , linux-kernel@vger.kernel.org, Greg Kroah-Hartman , devel@driverdev.osuosl.org Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1610 Lines: 41 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. > > 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. -- 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/