Re: [PATCH 3/5] x86/microcode/intel: Allow a late-load only if a min rev is specified

From: Ashok Raj <ashok.raj@intel.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Borislav Petkov <bp@alien8.de>,
	Thomas Gleixner <tglx@linutronix.de>,
	Tony Luck <tony.luck@intel.com>,
	Dave Hansen <dave.hansen@intel.com>,
	"LKML Mailing List" <linux-kernel@vger.kernel.org>,
	X86-kernel <x86@kernel.org>,
	Andy Lutomirski <luto@amacapital.net>,
	Tom Lendacky <thomas.lendacky@amd.com>,
	Ashok Raj <ashok.raj@intel.com>
Subject: Re: [PATCH 3/5] x86/microcode/intel: Allow a late-load only if a min rev is specified
Date: Mon, 15 Aug 2022 12:41:06 +0000	[thread overview]
Message-ID: <Yvo+4odYjQEkvmRr@araj-dh-work> (raw)
In-Reply-To: <Yvn5vBRNz9z8Y4A9@worktop.programming.kicks-ass.net>

On Mon, Aug 15, 2022 at 09:46:04AM +0200, Peter Zijlstra wrote:
> On Sat, Aug 13, 2022 at 10:38:23PM +0000, Ashok Raj wrote:
> 
> > The proposal here is an even simpler option. The criteria for a microcode to
> > be a viable late-load candidate is that no CPUID or OS visible MSR features
> > are removed with respect to an earlier version of the microcode.
> > 
> > Pseudocode for late-load is as follows:
> > 
> > if header.min_required_id == 0
> > 	This is old format microcode, block late-load
> > else if current_ucode_version < header.min_required_id
> > 	Current version is too old, block late-load of this microcode.
> > else
> > 	OK to proceed with late-load.
> > 
> > Any microcode that removes a feature will set the min_version to itself.
> > This will enforce this microcode is not suitable for late-loading.
> > 
> > The enforcement is not in hardware and limited to kernel loader enforcing
> > the requirement. It is not required for early loading of microcode to
> > enforce this requirement, since the new features are only
> > evaluated after early loading in the boot process.
> > 
> > 
> > Test cases covered:
> > 
> > 1. With new kernel, attempting to load an older format microcode with the
> >    min_rev=0 should be blocked by kernel.
> > 
> >    [  210.541802] microcode: Header MUST specify min version for late-load
> > 
> > 2. New microcode with a non-zero min_rev in the header, but the specified
> >    min_rev is greater than what is currently loaded in the CPU should be
> >    blocked by kernel.
> > 
> >    245.139828] microcode: Current revision 0x8f685300 is too old to update,
> > must be at 0xaa000050 version or higher
> > 
> > 3. New microcode with a min_rev < currently loaded should allow loading the
> >    microcode
> > 
> > 4. Build initrd with microcode that has min_rev=0, or min_rev > currently
> >    loaded should permit early loading microcode from initrd.
> 
> What if any validation do you have to ensure min_rev does as promised?

Today microcode release has a process by which these are packaged and
released. Qualifying a new update with a min version is a new step to their
process. This even limits their scope of validation to only revs >= min_rev
added in the header.

> That is, ucode can very easily lie about the number and still remove an
> MSR or CPUID enumerated feature.

Sorry I'm probably missing something. You mean someone maliciouly changes
the min_rev from what was released from Intel? 

OR

The release missed specifying a min-rev before release, accidently even
though its actually removing a feature? That would be in the bug category.

Release should have validation tests to cover all known feature bits and
such and check for any misses after an update as part of the qual process.

Cheers,
Ashok