From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751200AbdFBSkL convert rfc822-to-8bit (ORCPT ); Fri, 2 Jun 2017 14:40:11 -0400 Received: from mx2.suse.de ([195.135.220.15]:41917 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750918AbdFBSkK (ORCPT ); Fri, 2 Jun 2017 14:40:10 -0400 Date: Fri, 2 Jun 2017 20:40:07 +0200 From: Jean Delvare To: Andy Shevchenko Cc: LKML , Dmitry Torokhov , Mika Westerberg , Linus Walleij Subject: Re: [PATCH] firmware: dmi: Check DMI structure length Message-ID: <20170602204007.100d9d34@endymion> In-Reply-To: References: <20170601150839.08fdb556@endymion> <20170601164019.0a4035a4@endymion> Organization: SUSE Linux X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.31; x86_64-suse-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 1 Jun 2017 19:06:36 +0300, Andy Shevchenko wrote: > On Thu, Jun 1, 2017 at 5:40 PM, Jean Delvare wrote: > > On Thu, 1 Jun 2017 16:16:05 +0300, Andy Shevchenko wrote: > >> On Thu, Jun 1, 2017 at 4:08 PM, Jean Delvare wrote: > > >> > - const u8 *d = (u8 *) dm + index; > >> > + const u8 *d; > >> > >> > + d = (u8 *) dm + index; > >> > >> I think you may leave this as is and make it compiler's burden to optimize. > > > > Is there any benefit except making the patch smaller? > > Your commit message should answer to the question why and what. > You didn't put it there. > Moreover, the change above per se doesn't belong to this — one logical > change per patch. I'm confused. These changes totally belong to this patch. They belong so much to it, that's the very reason why they are not described separately in the commit message. The purpose of the patch is to check that the records are large enough to contain the fields we need to access. Setting a pointer beyond the end of the record _before_ performing that check makes no sense. I did not include these changes as performance optimizations, I included them because they make the code conceptually correct. It's even clearer for the last instance, as we are dereferencing the pointer immediately, but in my opinion, even setting a pointer to a location which may not exist is equally wrong and confusing for the reader. That's why I moved that code after the length checks. -- Jean Delvare SUSE L3 Support