| Message ID | 20241028091856.2151603-6-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: > --- a/xen/arch/x86/cpu/microcode/core.c > +++ b/xen/arch/x86/cpu/microcode/core.c > @@ -205,19 +205,6 @@ static void __init microcode_scan_module(struct boot_info *bi) > } > } > > -static void __init microcode_grab_module(struct boot_info *bi) > -{ > - 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 ) > - microcode_scan_module(bi); > -} > - > static struct microcode_ops __ro_after_init ucode_ops; > > static DEFINE_SPINLOCK(microcode_mutex); > @@ -831,7 +818,15 @@ static int __init early_microcode_load(struct boot_info *bi) > size_t size; > struct microcode_patch *patch; > > - microcode_grab_module(bi); > + 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 ) > + microcode_scan_module(bi); > > if ( !ucode_mod.mod_end && !ucode_blob.size ) > return 0; Could I talk you into doing away with the goto at the same time, by inverting the if()'s condition? Preferably with that: Acked-by: Jan Beulich <jbeulich@suse.com> Jan
On 10/28/24 05:18, Andrew Cooper wrote: > No functional change. > > 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> > --- > xen/arch/x86/cpu/microcode/core.c | 23 +++++++++-------------- > 1 file changed, 9 insertions(+), 14 deletions(-) > > diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c > index 0b00e70f3b07..4c4003bf9687 100644 > --- a/xen/arch/x86/cpu/microcode/core.c > +++ b/xen/arch/x86/cpu/microcode/core.c > @@ -205,19 +205,6 @@ static void __init microcode_scan_module(struct boot_info *bi) > } > } > > -static void __init microcode_grab_module(struct boot_info *bi) > -{ > - 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 ) > - microcode_scan_module(bi); > -} > - > static struct microcode_ops __ro_after_init ucode_ops; > > static DEFINE_SPINLOCK(microcode_mutex); > @@ -831,7 +818,15 @@ static int __init early_microcode_load(struct boot_info *bi) > size_t size; > struct microcode_patch *patch; > > - microcode_grab_module(bi); > + 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 ) > + microcode_scan_module(bi); > > if ( !ucode_mod.mod_end && !ucode_blob.size ) > return 0; Just a suggestion, but would it be useful to add a comment to explain the three conditions of ucode_mod_idx that are being handled before scanning is attempted? These being negative value, a valid module index, and module index wasn't already claimed in the module map. Otherwise, Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>
On 02/11/2024 4:33 pm, Daniel P. Smith wrote: > On 10/28/24 05:18, Andrew Cooper wrote: >> @@ -831,7 +818,15 @@ static int __init early_microcode_load(struct >> boot_info *bi) >> size_t size; >> struct microcode_patch *patch; >> - microcode_grab_module(bi); >> + 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 ) >> + microcode_scan_module(bi); >> if ( !ucode_mod.mod_end && !ucode_blob.size ) >> return 0; > > Just a suggestion, but would it be useful to add a comment to explain > the three conditions of ucode_mod_idx that are being handled before > scanning is attempted? The already did that, but it's in the next patch, which also rearranges this block massively. ~Andrew
On 11/5/24 18:07, Andrew Cooper wrote: > On 02/11/2024 4:33 pm, Daniel P. Smith wrote: >> On 10/28/24 05:18, Andrew Cooper wrote: >>> @@ -831,7 +818,15 @@ static int __init early_microcode_load(struct >>> boot_info *bi) >>> size_t size; >>> struct microcode_patch *patch; >>> - microcode_grab_module(bi); >>> + 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 ) >>> + microcode_scan_module(bi); >>> if ( !ucode_mod.mod_end && !ucode_blob.size ) >>> return 0; >> >> Just a suggestion, but would it be useful to add a comment to explain >> the three conditions of ucode_mod_idx that are being handled before >> scanning is attempted? > > The already did that, but it's in the next patch, which also rearranges > this block massively. I realized that and meant to come back to adjust my response. As such, Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c index 0b00e70f3b07..4c4003bf9687 100644 --- a/xen/arch/x86/cpu/microcode/core.c +++ b/xen/arch/x86/cpu/microcode/core.c @@ -205,19 +205,6 @@ static void __init microcode_scan_module(struct boot_info *bi) } } -static void __init microcode_grab_module(struct boot_info *bi) -{ - 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 ) - microcode_scan_module(bi); -} - static struct microcode_ops __ro_after_init ucode_ops; static DEFINE_SPINLOCK(microcode_mutex); @@ -831,7 +818,15 @@ static int __init early_microcode_load(struct boot_info *bi) size_t size; struct microcode_patch *patch; - microcode_grab_module(bi); + 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 ) + microcode_scan_module(bi); if ( !ucode_mod.mod_end && !ucode_blob.size ) return 0;
No functional change. 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> --- xen/arch/x86/cpu/microcode/core.c | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-)