From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754081AbbCBNkd (ORCPT ); Mon, 2 Mar 2015 08:40:33 -0500 Received: from userp1040.oracle.com ([156.151.31.81]:43842 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750914AbbCBNka (ORCPT ); Mon, 2 Mar 2015 08:40:30 -0500 Date: Mon, 2 Mar 2015 14:42:12 +0100 From: Quentin Casasnovas To: Borislav Petkov Cc: Quentin Casasnovas , x86-ml , lkml Subject: Re: [GIT PULL] microcode loader updates Message-ID: <20150302134212.GC14850@chrystal.home> References: <20150302123441.GB17521@pd.tnic> <20150302130336.GB14850@chrystal.home> <20150302132950.GC17521@pd.tnic> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150302132950.GC17521@pd.tnic> User-Agent: Mutt/1.5.22 (2013-10-16) X-Source-IP: acsinet22.oracle.com [141.146.126.238] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Mar 02, 2015 at 02:29:50PM +0100, Borislav Petkov wrote: > On Mon, Mar 02, 2015 at 02:03:36PM +0100, Quentin Casasnovas wrote: > > So at the last loop iteration for j == i, we'll do kfree(saved_ptr[j]) > > which AFAICT hasn't been initialized yet. Using a kcalloc() your first > > allocation for saved_ptr should just work since the memory will be cleared > > and kfree(NULL) doesn't do anything. > > You're correct, but(!)... > > Practically, this is not a problem because @mc_saved_src being handed > down to save_microcode() is at both call sites initialized up to > mc_saved_count elements and the loop in save_microcode() only inspects > this far. > > So actually, this test is not really needed: > > if (!mc_saved_src[i]) { > ret = -EINVAL; > goto err; > } > > AFAICT and if I'm not missing anything else, of course. > > In any case, I'd like to keep this series cleanup-only (well, except > this one) and address your comments later. Don't worry, I haven't > forgotten them - I want to *not* fix everything in one go. > > Agreed? > Hey up to you, really :) It's just that this potential-but-very-very-likely-impossible kfree() on garbage wasn't present in the original code - so I thought changing the kmalloc() => kcalloc() was small enough to add in your serie. I'd also be fine removing the early loop termination condition if you think it's dead code since that'll make sure this will never happen. A static analyzer or maybe some cocinnelle semantic patches are likely to start complaining about this otherwise, I think. Quentin