| 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
| ||
|
Message-ID: <YvycggAits6wBXP8@gmail.com>
Date: Wed, 17 Aug 2022 09:45:06 +0200
From: Ingo Molnar <mingo@...nel.org>
To: Ashok Raj <ashok.raj@...el.com>
Cc: Borislav Petkov <bp@...en8.de>,
Thomas Gleixner <tglx@...utronix.de>,
Tony Luck <tony.luck@...el.com>,
Dave Hansen <dave.hansen@...el.com>,
LKML Mailing List <linux-kernel@...r.kernel.org>,
X86-kernel <x86@...nel.org>,
Andy Lutomirski <luto@...capital.net>,
Tom Lendacky <thomas.lendacky@....com>,
Jacon Jun Pan <jacob.jun.pan@...el.com>
Subject: Re: [PATCH v3 2/5] x86/microcode/intel: Allow a late-load only if a
min rev is specified
* Ashok Raj <ashok.raj@...el.com> wrote:
> In general users don't have the necessary information to determine
> whether a late-load of a new microcode version has removed any feature
> (MSR, CPUID etc) between what is currently loaded and this new microcode.
> To address this issue, Intel has added a "minimum required version" field to
> a previously reserved field in the file header. Microcode updates
> should only be applied if the current microcode version is equal
> to, or greater than this minimum required version.
>
> https://lore.kernel.org/linux-kernel/alpine.DEB.2.21.1909062237580.1902@nanos.tec.linutronix.de/
>
> Thomas made some suggestions on how meta-data in the microcode file could
> provide Linux with information to decide if the new microcode is suitable
> candidate for late-load. 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. 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.
>
>
> Tested-by: William Xie <william.xie@...el.com>
> Reviewed-by: Tony Luck <tony.luck@...el.com>
> Signed-off-by: Ashok Raj <ashok.raj@...el.com>
> ---
> arch/x86/include/asm/microcode_intel.h | 4 +++-
> arch/x86/kernel/cpu/microcode/intel.c | 20 ++++++++++++++++++++
> 2 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
> index 4c92cea7e4b5..16b8715e0984 100644
> --- a/arch/x86/include/asm/microcode_intel.h
> +++ b/arch/x86/include/asm/microcode_intel.h
> @@ -14,7 +14,9 @@ struct microcode_header_intel {
> unsigned int pf;
> unsigned int datasize;
> unsigned int totalsize;
> - unsigned int reserved[3];
> + unsigned int reserved1;
> + unsigned int min_req_id;
> + unsigned int reserved3;
> };
>
> struct microcode_intel {
> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> index c4b11e2fbe33..1eb202ec2302 100644
> --- a/arch/x86/kernel/cpu/microcode/intel.c
> +++ b/arch/x86/kernel/cpu/microcode/intel.c
> @@ -178,6 +178,7 @@ static int microcode_sanity_check(void *mc, int print_err)
> struct extended_sigtable *ext_header = NULL;
> u32 sum, orig_sum, ext_sigcount = 0, i;
> struct extended_signature *ext_sig;
> + struct ucode_cpu_info uci;
>
> total_size = get_totalsize(mc_header);
> data_size = get_datasize(mc_header);
> @@ -248,6 +249,25 @@ static int microcode_sanity_check(void *mc, int print_err)
> return -EINVAL;
> }
>
> + /*
> + * Enforce for late-load that min_req_id is specified in the header.
> + * Otherwise its an old format microcode, reject it.
s/its
/it's
...
> + */
> + if (print_err) {
> + if (!mc_header->min_req_id) {
> + pr_warn("Header MUST specify min version for late-load\n");
> + return -EINVAL;
> + }
> +
> + intel_cpu_collect_info(&uci);
> + if (uci.cpu_sig.rev < mc_header->min_req_id) {
> + pr_warn("Current revision 0x%x is too old to update,"
> + "must be at 0x%x version or higher\n",
> + uci.cpu_sig.rev, mc_header->min_req_id);
Please don't line-break user-visible syslog strings, just because
checkpatch is stupid.
If the user sees it as a single line, developers should see that same line
too...
Thanks,
Ingo
Powered by blists - more mailing lists