Re: [PATCH 5/5] x86/ucode: Relax digest check when Entrysign is fixed in firmware

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.10.2025 15:19, Andrew Cooper wrote:
> @@ -597,3 +598,81 @@ static void __init __constructor 
> test_digests_sorted(void)
>      }
>  }
>  #endif /* CONFIG_SELF_TESTS */
> +
> +/*
> + * The Entrysign vulnerability affects all Zen1 thru Zen5 CPUs.

And older ones are fine, or merely have no fixes produced?

>  Firmware
> + * fixes were produced in Nov/Dec 2025.  Zen3 thru Zen5 can continue to take
> + * OS-loadable microcode updates using a new signature scheme, as long as
> + * firmware has been updated first.
> + */

Yet what about Zen1/2?

> +void __init amd_check_entrysign(void)
> +{
> +    unsigned int curr_rev;
> +    uint8_t fixed_rev;
> +
> +    if ( boot_cpu_data.vendor != X86_VENDOR_AMD ||
> +         boot_cpu_data.family < 0x17 ||
> +         boot_cpu_data.family > 0x1a )
> +        return;
> +
> +    /*
> +     * Table taken from Linux, which is the only known source of information
> +     * about client revisions.
> +     */
> +    curr_rev = this_cpu(cpu_sig).rev;
> +    switch ( curr_rev >> 8 )
> +    {
> +    case 0x080012: fixed_rev = 0x6f; break;
> +    case 0x080082: fixed_rev = 0x0f; break;

In your reply you mentioned a "general off-by-1" when comparing with Linux,
but I'm in trouble understanding how both can be correct. Leaving aside the
1st line (for which you sent a Linux patch anyway), how can our
"(uint8_t)curr_rev >= fixed_rev" (i.e. "(uint8_t)curr_rev >= 0x0f") further
below be correct at the same time as Linux'es "return cur_rev <= 0x800820f"
(indicating to the caller whether a SHA check is needed) is also correct?
We say 0x0f is okay, while they demand a SHA check for that revision.

In any event, whatever (legitimate) off-by-1 it is that I'm failing to spot,
I think this would want explaining in the comment above.

> +    case 0x083010: fixed_rev = 0x7c; break;
> +    case 0x086001: fixed_rev = 0x0e; break;
> +    case 0x086081: fixed_rev = 0x08; break;
> +    case 0x087010: fixed_rev = 0x34; break;
> +    case 0x08a000: fixed_rev = 0x0a; break;
> +    case 0x0a0010: fixed_rev = 0x7a; break;
> +    case 0x0a0011: fixed_rev = 0xda; break;
> +    case 0x0a0012: fixed_rev = 0x43; break;
> +    case 0x0a0082: fixed_rev = 0x0e; break;
> +    case 0x0a1011: fixed_rev = 0x53; break;
> +    case 0x0a1012: fixed_rev = 0x4e; break;
> +    case 0x0a1081: fixed_rev = 0x09; break;
> +    case 0x0a2010: fixed_rev = 0x2f; break;
> +    case 0x0a2012: fixed_rev = 0x12; break;
> +    case 0x0a4041: fixed_rev = 0x09; break;
> +    case 0x0a5000: fixed_rev = 0x13; break;
> +    case 0x0a6012: fixed_rev = 0x0a; break;
> +    case 0x0a7041: fixed_rev = 0x09; break;
> +    case 0x0a7052: fixed_rev = 0x08; break;
> +    case 0x0a7080: fixed_rev = 0x09; break;
> +    case 0x0a70c0: fixed_rev = 0x09; break;
> +    case 0x0aa001: fixed_rev = 0x16; break;
> +    case 0x0aa002: fixed_rev = 0x18; break;
> +    case 0x0b0021: fixed_rev = 0x46; break;
> +    case 0x0b1010: fixed_rev = 0x46; break;
> +    case 0x0b2040: fixed_rev = 0x31; break;
> +    case 0x0b4040: fixed_rev = 0x31; break;
> +    case 0x0b6000: fixed_rev = 0x31; break;
> +    case 0x0b7000: fixed_rev = 0x31; break;

Without at least brief model related comments this looks extremely opaque.
Linux, as a minimal reference, at least has cpuid_to_ucode_rev() and the
accompanying union zen_patch_rev. Background of my remark is that I would
have expected there to be more models per Zen<N>, seeing in particular how
many different BKDGs / PPRs and RGs there are. Many RGs in particular say
they apply to a range of models, yet no similar ranges are covered here
(unless my deciphering attempts went wrong).

Jan