linux-kernel - Re: [PATCH v1 Part2 2/5] x86/microcode/intel: Add minimum required revision to microcode header

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: <Y8nFk/0D4XVPOx5a@a4bf019067fa.jf.intel.com>
Date:   Thu, 19 Jan 2023 14:34:59 -0800
From:   Ashok Raj <ashok.raj@...el.com>
To:     Thomas Gleixner <tglx@...utronix.de>
CC:     Borislav Petkov <bp@...en8.de>,
        LKML <linux-kernel@...r.kernel.org>, x86 <x86@...nel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Tony Luck <tony.luck@...el.com>,
        Dave Hansen <dave.hansen@...el.com>,
        Alison Schofield <alison.schofield@...el.com>,
        Reinette Chatre <reinette.chatre@...el.com>,
        Tom Lendacky <thomas.lendacky@....com>,
        Stefan Talpalaru <stefantalpalaru@...oo.com>,
        David Woodhouse <dwmw2@...radead.org>,
        "Benjamin Herrenschmidt" <benh@...nel.crashing.org>,
        Jonathan Corbet <corbet@....net>,
        "Rafael J . Wysocki" <rafael@...nel.org>,
        Peter Zilstra <peterz@...radead.org>,
        Andy Lutomirski <luto@...nel.org>,
        Andrew Cooper <Andrew.Cooper3@...rix.com>,
        Ashok Raj <ashok.raj@...el.com>
Subject: Re: [PATCH v1 Part2 2/5] x86/microcode/intel: Add minimum required
 revision to microcode header

On Thu, Jan 19, 2023 at 11:03:14PM +0100, Thomas Gleixner wrote:
> On Fri, Jan 13 2023 at 09:29, Ashok Raj wrote:
> > In general users don't have the necessary information to determine
> > whether a late loading of a new microcode version has removed any feature
> > (MSR, CPUID etc) between what is currently loaded and this new microcode.
> 
> s/this new microcode/a newer microcode revision/

Yes.

> 
> > To address this issue, Intel has added a "minimum required version" field
> > to a previously reserved field in the file header. Microcode updates
> 
> s/file header/microcode header/ perhaps?

Yep!
> 
> > should only be applied if the current microcode version is equal
> > to, or greater than this minimum required version.
> >
> > Thomas made some suggestions[1] on how meta-data in the microcode file
> > could provide Linux with information to decide if the new microcode is
> > suitable candidate for late loading. But even the "simpler" option#1
> > requires a lot of metadata and corresponding kernel code to parse it.
> >
> > The proposal here is an even simpler option.
> 
> IIRC this was also suggested by this Thomas dude, right?

Same dude.. might have been your twin :-)

I'll fix it.

> 
> > Simply "OS visible features" such as CPUID and MSRs are the only two
> > examples. The microcode must not change these OS visible features
> > because they cause problems after late loading. When microcode changes
> > features, microcode will change the min_rev to prevent such microcodes
> > from being late loaded.
> >
> > Pseudo code for late loading is as follows:
> >
> > if header.min_required_id == 0
> >         This is old format microcode, block late loading
> > else if current_ucode_version < header.min_required_id
> >         Current version is too old, block late loading of this microcode.
> > else
> >         OK to proceed with late loading.
> >
> > Any microcode that modifies the interface to an OS-visible feature
> > will set the min_version to itself. This will enforce this microcode is
> > not suitable for late loading unless the currently loaded revision is
> > greater or equal to the new microcode affecting the change.
> 
> Up to this paragraph the changelog made sense.
> 
> If the currently loaded revision is the same as the to be loaded
> revision, then there is nothing to do.
> 
> If the currently loaded revision is greater than the to be loaded
> revision then it is not loaded as the kernel does not support
> downgrading in the first place.
> 
> Even if it would support downgrading then this would be outright wrong
> for this case:
> 
> Rev:        10
> Min-Rev:    10
> 
> Rev:        20
> Min-Rev:    20
> 
> If Rev 20 is loaded, then you absolutely cannot load Rev 10 because that
> would have the reverse side effects due to which Rev 20 prevents late
> loading.
> 
> See?

Yes, that's accurate, and in sprit it works that way.

The current_rev > mc_hdr->rev is done in apply_microcode_intel() but I
suppose we could do that check early. 

I didn't touch those parts to make sure only minimal changes were done and
we can do cleanup's later.  I should certainly add a note to make sure we
aren't breaking the rev is always greater than what's in the CPU for
clarity.

I do have several cleanups lined up, but didn't want to hold the minrev and
the nmi series.

Cheers,
Ashok