| Message ID | 20241028091856.2151603-7-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/ucode: Fix module-handling use-aftere-free's | expand |
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. > @@ -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.) > @@ -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. Jan
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
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c index 4c4003bf9687..fc31ab35c3c8 100644 --- a/xen/arch/x86/cpu/microcode/core.c +++ b/xen/arch/x86/cpu/microcode/core.c @@ -60,7 +60,6 @@ #define MICROCODE_UPDATE_TIMEOUT_US 1000000 static module_t __initdata ucode_mod; -static signed int __initdata ucode_mod_idx; static bool __initdata ucode_mod_forced; static unsigned int nr_cores; @@ -97,11 +96,6 @@ struct patch_with_flags { }; static struct ucode_mod_blob __initdata ucode_blob; -/* - * By default we will NOT parse the multiboot modules to see if there is - * cpio image with the microcode images. - */ -static bool __initdata ucode_scan; /* By default, ucode loading is done in NMI handler */ static bool ucode_in_nmi = true; @@ -109,13 +103,28 @@ static bool ucode_in_nmi = true; /* Protected by microcode_mutex */ static const struct microcode_patch *microcode_cache; +/* + * opt_mod_idx and opt_scan have subtle semantics. + * + * The cmdline can either identify a module by number (inc -ve back-reference) + * containing a raw microcode container, or select scan which instructs Xen to + * search all modules for an uncompressed CPIO archive containing a file with + * a vendor-dependent name. + * + * These options do not make sense when combined, so for the benefit of module + * location we require that they are not both active together. + */ +static int __initdata opt_mod_idx; +static bool __initdata opt_scan; + /* * Used by the EFI path only, when xen.cfg identifies an explicit microcode * file. Overrides ucode=<int>|scan on the regular command line. */ void __init microcode_set_module(unsigned int idx) { - ucode_mod_idx = idx; + opt_mod_idx = idx; + opt_scan = false; ucode_mod_forced = 1; } @@ -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; } @@ -166,7 +179,7 @@ static void __init microcode_scan_module(struct boot_info *bi) int i; ucode_blob.size = 0; - if ( !ucode_scan ) + if ( !opt_scan ) return; if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD ) @@ -792,7 +805,7 @@ static int __init cf_check microcode_init_cache(void) if ( !ucode_ops.apply_microcode ) return -ENODEV; - if ( ucode_scan ) + if ( opt_scan ) /* Need to rescan the modules because they might have been relocated */ microcode_scan_module(bi); @@ -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: - if ( ucode_scan ) + if ( opt_scan ) /* Scan for a CPIO archive */ microcode_scan_module(bi); + if ( idx ) /* Specific module nominated */ + { + /* + * Negative indicies can be used to reference from the end of the + * modules. e.g. ucode=-1 refers to the last module. + */ + if ( idx < 0 ) + idx += bi->nr_modules; + + if ( idx <= 0 || idx >= bi->nr_modules ) + { + printk(XENLOG_WARNING "Microcode: Chosen module %d out of range [1, %u)\n", + idx, bi->nr_modules); + return -ENODEV; + } + + if ( !__test_and_clear_bit(idx, bi->module_map) ) + { + printk(XENLOG_WARNING "Microcode: Chosen module %d already used\n", idx); + return -ENODEV; + } + + ucode_mod = *bi->mods[idx].mod; + } + if ( !ucode_mod.mod_end && !ucode_blob.size ) return 0;
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. Rename ucode_{scan,mod_idx} to have an opt_ prefix. This is both for consistency with the rest of Xen, and to guarantee that we've got all instances of the variables covered in this change. Explain how they're use. During cmdline/config parsing, always update both variables in pairs. In early_microcode_load(), ASSERT() the invariant just in case. Use a local variable for idx rather than operating on the static; we're the only consume of the value. Expand the index selection logic with comments and warnings to the user when something went wrong. Fixes: 5ed12565aa32 ("microcode: rendezvous CPUs in NMI handler and load ucode") Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Daniel P. Smith <dpsmith@apertussolutions.com> Despite the fixes tag, this can't be backported (at least not in this form). --- xen/arch/x86/cpu/microcode/core.c | 76 +++++++++++++++++++++++-------- 1 file changed, 57 insertions(+), 19 deletions(-)