linux-kernel - Re: [GIT PULL] microcode loader updates

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150302134212.GC14850@chrystal.home>
Date:	Mon, 2 Mar 2015 14:42:12 +0100
From:	Quentin Casasnovas <quentin.casasnovas@...cle.com>
To:	Borislav Petkov <bp@...en8.de>
Cc:	Quentin Casasnovas <quentin.casasnovas@...cle.com>,
	x86-ml <x86@...nel.org>, lkml <linux-kernel@...r.kernel.org>
Subject: Re: [GIT PULL] microcode loader updates

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/