Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
From:   Qiuxu Zhuo <qiuxu.zhuo@intel.com>
To:     tglx@linutronix.de
Cc:     arjan@linux.intel.com, ashok.raj@intel.com, bp@alien8.de,
        chang.seok.bae@intel.com, linux-kernel@vger.kernel.org,
        nik.borisov@suse.com, x86@kernel.org, qiuxu.zhuo@intel.com
Subject: Re: [patch V3 30/30] x86/microcode/intel: Add a minimum required revision for late-loads
Date:   Tue, 26 Sep 2023 00:20:32 +0800
Message-Id: <20230925162032.56408-1-qiuxu.zhuo@intel.com>
In-Reply-To: <20230912065502.631634574@linutronix.de>
References: <20230912065502.631634574@linutronix.de>
Precedence: bulk

> ...
> From: Ashok Raj <ashok.raj@intel.com>
> 
> In general users don't have the necessary information to determine whether
> late loading of a new microcode version is safe and does not modify
> anything which the currently running kernel uses already, e.g. removal of
> CPUID bits or behavioural changes of MSRs.
> ...
> 
> The check is always enabled, but by default not enforced. It can be
> enforced via Kconfig or kernel command line.
> 
> If enforced, the kernel refuses to late load microcode with a minium

s/minium/minimum/

> required version field which is zero or when the currently loaded microcode
> revision is smaller than the minimum required revision.
> 
> ...
> --- a/arch/x86/kernel/cpu/microcode/intel.c
> +++ b/arch/x86/kernel/cpu/microcode/intel.c
> @@ -463,16 +463,40 @@ static enum ucode_state apply_microcode_
>  	return ret;
>  }
>  
> +static bool ucode_validate_minrev(struct microcode_header_intel *mc_header)
> +{
> +	int cur_rev = boot_cpu_data.microcode;
> +
> +	/*
> +	 * When late-loading, ensure the header declares a minimum revision
> +	 * required to perform a late-load. The previously reserved field
> +	 * is 0 in older microcode blobs.
> +	 */
> +	if (!mc_header->min_req_ver) {
> +		pr_info("Unsafe microcode update: Microcode header does not specify a required min version\n");
> +		return false;
> +	}
> +
> +	/*
> +	 * Check whether the minimum revision specified in the header is either
> +	 * greater or equal to the current revision.
> +	 */

Seems like the above comment doesn't match the following 'if' check.
Perhaps the comment is:

   "Check whether the current revision is either greater or
    equal to the minimum revision specified in the header."

> +	if (cur_rev < mc_header->min_req_ver) {
> +		pr_info("Unsafe microcode update: Current revision 0x%x too old\n", cur_rev);
> +		pr_info("Current should be at 0x%x or higher. Use early loading instead\n", mc_header->min_req_ver);
> +		return false;
> +	}
> +	return true;
> +}
> ...