Re: [PATCH v3] x86/microcode/intel: Reject problematic loading on Granite Rapids systems

[Sohil Mehta]

>  /* Scan blob for microcode matching the boot CPUs family, model, stepping */
>  static __init struct microcode_intel *scan_microcode(void *data, size_t size,
>  						     struct ucode_cpu_info *uci,
> @@ -330,6 +350,9 @@ static __init struct microcode_intel *scan_microcode(void *data, size_t size,
>  		if (!intel_find_matching_signature(data, &uci->cpu_sig))
>  			continue;
>  
> +		if (!revision_is_safe(&uci->cpu_sig, mc_header->rev))
> +			continue;
> +
>  		/*
>  		 * For saving the early microcode, find the matching revision which
>  		 * was loaded on the BSP.
> @@ -878,6 +901,9 @@ static enum ucode_state parse_microcode_blobs(int cpu, struct iov_iter *iter)
>  		if (!intel_find_matching_signature(mc, &uci->cpu_sig))
>  			continue;
>  
> +		if (!revision_is_safe(&uci->cpu_sig, mc_header.rev))
> +			continue;
> +
>  		is_safe = ucode_validate_minrev(&mc_header);
>  		if (force_minrev && !is_safe)
>  			continue;

TL;dr: Should there be a revision_is_safe() check during
__apply_microcode()?

The patch only adds the revision check during blob selection. Have we
evaluated the corner cases hinted by sashiko when this check might be
bypassed? It talks about suspend/resume and the CPU hotplug cases.

v1:
https://sashiko.dev/#/patchset/20260901231634.714144-1-chang.seok.bae%40intel.com?part=1

v3:
https://sashiko.dev/#/patchset/20260916225939.1144524-1-chang.seok.bae%40intel.com?part=1

The suspend/resume path probably doesn't matter for GNR servers and most
of the CPU hotplug flows also seem to be covered. But, what about ucode
update on cores that are not enabled at boot? If there are brought
online later, would the ucode update skip the above check?

For example, maxcpus=N prevents certain CPUs from showing up in
cpus_booted_once_mask. So the checks in setup_cpus() during late-loading
would not catch them. IIUC, the BIOS version can be < 0x1000405, early
load can bump the booted cpus to 0x1000405, and then late-loading can
load newer versions and only cache the latest ucode revision.

All this while, the non-booted cores would be stuck at a revision less
than 0x1000405. So, when they are brought online later, could they
directly jump to a revision greater than 0x1000405?

Would it be safer to add the revision_is_safe() check in
__apply_microcode() so that all of such cases are covered?

Maybe sashiko just made me paranoid. I only started looking at this
because it complained! :(