linux-kernel - Re: [PATCH 06/21] x86: microcode: Convert to hotplug state machine

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20160907145146.p7escazpjvt6n7s7@linutronix.de>
Date:   Wed, 7 Sep 2016 16:51:47 +0200
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     Borislav Petkov <bp@...en8.de>
Cc:     linux-kernel@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>, rt@...utronix.de,
        tglx@...utronix.de
Subject: Re: [PATCH 06/21] x86: microcode: Convert to hotplug state machine

On 2016-09-07 13:36:40 [+0200], Borislav Petkov wrote:
> You mean this:
> 
>         /* The CPU refused to come up during a system resume */
>         if (action == CPU_UP_CANCELED_FROZEN)
>                 microcode_fini_cpu(cpu);
correct.
> ?
> 
> It clears internal state, i.e., invalidates the current microcode patch.

it cleans the memory on Intel but not on AMD

> > diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
> > index df04b2d033f6..4fc67b51e22e 100644
> > --- a/arch/x86/kernel/cpu/microcode/core.c
> > +++ b/arch/x86/kernel/cpu/microcode/core.c
> 
> ...
> 
> > -static struct notifier_block mc_cpu_notifier = {
> > -	.notifier_call	= mc_cpu_callback,
> > -};
> > +static int mc_cpu_down_prep(unsigned int cpu)
> > +{
> > +	struct device *dev;
> > +
> > +	dev = get_cpu_device(cpu);
> > +	/* Suspend is in progress, only remove the interface */
> > +	sysfs_remove_group(&dev->kobj, &mc_attr_group);
> > +	pr_debug("CPU%d removed\n", cpu);
> > +	return 0;
> > +}
> > +
> > +static int mc_cpu_dead(unsigned int cpu)
> > +{
> > +#ifdef CONFIG_SMP
> > +	if (cpuhp_tasks_frozen)
> > +		microcode_fini_cpu(cpu);
> > +#endif
> > +	return 0;
> > +}
> 
> If this is corresponding to CPU_DEAD, then I'd like to point to that comment:
> 
>         /*
>          * case CPU_DEAD:
>          *
>          * When a CPU goes offline, don't free up or invalidate the copy of
>          * the microcode in kernel memory, so that we can reuse it when the
>          * CPU comes back online without unnecessarily requesting the userspace
>          * for it again.
>          */
> 
> IOW, you don't need mc_cpu_dead().
Okay. After a second look I would say so, too. On Intel we free memory
in this case but we don't set uci->valid back. Which means if the CPU
did not come up after resume we free the memory. If we try it (manually)
again and for some reason the CPU manages to get up, it will end up in
the ONLINE callback with no memory and uci->valid set.

I will prepare a patch with the DEAD state gone.

Sebastian