Re: [PATCH 06/10] x86/ucode: Enforce invariant about module selection

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/10/2024 1:56 pm, Jan Beulich wrote:
> On 28.10.2024 10:18, Andrew Cooper wrote:
>> The work to add the `ucode=nmi` cmdline option left a subtle corner case.
>> Both scan and an explicit index could be selected, and we could really find a
>> CPIO archive and explicit microcode file.
>>
>> Worse, because the if/else chains for processing ucode_{blob,mod} are 
>> opposite
>> ways around in early_microcode_load() and microcode_init_cache(), we can
>> genuinely perform early microcode loading from the CPIO archive, then cache
>> from the explicit file.
>>
>> Therefore, enforce that only one selection method can be active.
> Question is - is this really the best of all possible behaviors? One may want
> to use one approach as the fallback for the other, e.g. preferably use what
> the CPIO has, but fall back to something pre-installed on the boot or EFI
> partition.

It is unfortunate behaviour.

I've seen explicit complains about it on the archlinux forums; putting a
CPIO fragment in EFI's ucode= argument and getting confused as to why it
doesn't work.

However, it is (reasonably) necessary to dis-enagle this path, and we
currently document it as "undefined behaviour".

I think that we should re-evaluate the behaviour, after the rest of the
boot cleanup is done and we have an easier time reasoning about what is
what.
>> @@ -139,12 +148,16 @@ static int __init cf_check parse_ucode(const char *s)
>>          else if ( !ucode_mod_forced ) /* Not forced by EFI */
>>          {
>>              if ( (val = parse_boolean("scan", s, ss)) >= 0 )
>> -                ucode_scan = val;
>> +            {
>> +                opt_scan = val;
>> +                opt_mod_idx = 0;
>> +            }
>>              else
>>              {
>>                  const char *q;
>>  
>> -                ucode_mod_idx = simple_strtol(s, &q, 0);
>> +                opt_scan = false;
>> +                opt_mod_idx = simple_strtol(s, &q, 0);
>>                  if ( q != ss )
>>                      rc = -EINVAL;
>>              }
> I think this latter part rather wants to be
>
>                 opt_mod_idx = simple_strtol(s, &q, 0);
>                 if ( q != ss )
>                 {
>                     opt_mod_idx = 0;
>                     rc = -EINVAL;
>                 }
>                 else
>                     opt_scan = false;
>
> to prevent a malformed ucode= to clobber an earlier wellformed ucode=scan.
> (There are limits to this of course, as an out-of-range value would still
> invalidate the "scan" request.)

Fine.  I'm not overly fussed.  We don't make any pretence that erroneous
cmdline settings are handled nicely.

>
>> @@ -817,17 +830,42 @@ static int __init early_microcode_load(struct 
>> boot_info *bi)
>>      const void *data = NULL;
>>      size_t size;
>>      struct microcode_patch *patch;
>> +    int idx = opt_mod_idx;
>> +
>> +    /*
>> +     * Cmdline parsing ensures this invariant holds, so that we don't end up
>> +     * trying to mix multiple ways of finding the microcode.
>> +     */
>> +    ASSERT(idx == 0 || !opt_scan);
>>  
>> -    if ( ucode_mod_idx < 0 )
>> -        ucode_mod_idx += bi->nr_modules;
>> -    if ( ucode_mod_idx <= 0 || ucode_mod_idx >= bi->nr_modules ||
>> -         !__test_and_clear_bit(ucode_mod_idx, bi->module_map) )
>> -        goto scan;
>> -    ucode_mod = *bi->mods[ucode_mod_idx].mod;
>> - scan:
> Oh, the goto and label are going away here anyway. Never mind the comment on
> the earlier patch then.

Thanks, and yes.

I did play with the order quite a lot.  The first iteration did clean
this up as part of merging into early_microcode_load(), but it turned
into a mess.

This way around (straight fold in the previous patch, rework here given
the invariant) was the cleanest I came up with.

~Andrew