| Message ID | 20241028091856.2151603-8-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: > We've got a perfectly good vendor abstraction already for microcode. No need > for a second ad-hoc one in microcode_scan_module(). > > This is in preparation to use ucode_ops.cpio_path in multiple places. > > These paths are only used during __init, so take the opportunity to move them > into __initconst. As an alternative to this, how about ... > --- a/xen/arch/x86/cpu/microcode/private.h > +++ b/xen/arch/x86/cpu/microcode/private.h > @@ -59,6 +59,13 @@ struct microcode_ops { > */ > enum microcode_match_result (*compare_patch)( > const struct microcode_patch *new, const struct microcode_patch *old); > + > + /* > + * For Linux inird microcode compatibliity. > + * > + * The path where this vendor's microcode can be found in CPIO. > + */ > + const char *cpio_path; const char cpio_path[]; inheriting the __initconst from the struct instances? Acked-by: Jan Beulich <jbeulich@suse.com> with a slight preference to the form without the extra pointer. Except that: gcc14 looks to be buggy when it comes to the copying of such a struct. The example below yields an internal compiler error. And the direct structure assignment also doesn't quite do what I would expect it to do (visible when commenting out the "else" branch. Bottom line - leave the code as is. Jan struct s { unsigned long ul; int i; char ac[]; }; const struct s gs = { 3, -4, "abcdef" }; void copy_s(struct s*d, const struct s*s) { *d = *s; } unsigned test(struct s*ps, _Bool direct) { if(direct) *ps = gs; else copy_s(ps, &gs); return sizeof(*ps); } unsigned size(void) { return sizeof(gs); }
On 28/10/2024 2:25 pm, Jan Beulich wrote: > On 28.10.2024 10:18, Andrew Cooper wrote: >> We've got a perfectly good vendor abstraction already for microcode. No need >> for a second ad-hoc one in microcode_scan_module(). >> >> This is in preparation to use ucode_ops.cpio_path in multiple places. >> >> These paths are only used during __init, so take the opportunity to move them >> into __initconst. > As an alternative to this, how about ... > >> --- a/xen/arch/x86/cpu/microcode/private.h >> +++ b/xen/arch/x86/cpu/microcode/private.h >> @@ -59,6 +59,13 @@ struct microcode_ops { >> */ >> enum microcode_match_result (*compare_patch)( >> const struct microcode_patch *new, const struct microcode_patch *old); >> + >> + /* >> + * For Linux inird microcode compatibliity. >> + * >> + * The path where this vendor's microcode can be found in CPIO. >> + */ >> + const char *cpio_path; > const char cpio_path[]; > > inheriting the __initconst from the struct instances? > Acked-by: Jan Beulich <jbeulich@suse.com> > with a slight preference to the form without the extra pointer. I'm slightly surprised at this request, given that the form with the pointer results in less data held at runtime. > Except that: > gcc14 looks to be buggy when it comes to the copying of such a struct. The > example below yields an internal compiler error. And the direct structure > assignment also doesn't quite do what I would expect it to do (visible when > commenting out the "else" branch. Bottom line - leave the code as is. It's unfortunate to hit an ICE, but the copy cannot possibly work in the first place. ucode_ops is in a separate translation unit and has no space allocated after the flexible member. Any copy into it is memory corruption of whatever object happens to be sequentially after ucode_ops. The only way it would work is having `const char cpio_path[40];` which is long enough for anything we'd expect to find. But again, that involves holding init-only data post init. ~Andrew
On 28.10.2024 15:38, Andrew Cooper wrote: > On 28/10/2024 2:25 pm, Jan Beulich wrote: >> On 28.10.2024 10:18, Andrew Cooper wrote: >>> We've got a perfectly good vendor abstraction already for microcode. No need >>> for a second ad-hoc one in microcode_scan_module(). >>> >>> This is in preparation to use ucode_ops.cpio_path in multiple places. >>> >>> These paths are only used during __init, so take the opportunity to move them >>> into __initconst. >> As an alternative to this, how about ... >> >>> --- a/xen/arch/x86/cpu/microcode/private.h >>> +++ b/xen/arch/x86/cpu/microcode/private.h >>> @@ -59,6 +59,13 @@ struct microcode_ops { >>> */ >>> enum microcode_match_result (*compare_patch)( >>> const struct microcode_patch *new, const struct microcode_patch *old); >>> + >>> + /* >>> + * For Linux inird microcode compatibliity. >>> + * >>> + * The path where this vendor's microcode can be found in CPIO. >>> + */ >>> + const char *cpio_path; >> const char cpio_path[]; >> >> inheriting the __initconst from the struct instances? >> Acked-by: Jan Beulich <jbeulich@suse.com> >> with a slight preference to the form without the extra pointer. > > I'm slightly surprised at this request, given that the form with the > pointer results in less data held at runtime. No, it doesn't. Yet I only now realize that ... >> Except that: >> gcc14 looks to be buggy when it comes to the copying of such a struct. The >> example below yields an internal compiler error. And the direct structure >> assignment also doesn't quite do what I would expect it to do (visible when >> commenting out the "else" branch. Bottom line - leave the code as is. > > It's unfortunate to hit an ICE, but the copy cannot possibly work in the > first place. > > ucode_ops is in a separate translation unit and has no space allocated > after the flexible member. Any copy into it is memory corruption of > whatever object happens to be sequentially after ucode_ops. ... my expectation of how the copy ought to work (and how the C standard, at least in close enough an example, specifies it) would specifically _not_ suit our needs. The copy ought to only cover sizeof(struct ...), i.e. not the string. Yet we'd need that string to be copied to be usable for our purposes. > The only way it would work is having `const char cpio_path[40];` which > is long enough for anything we'd expect to find. > > But again, that involves holding init-only data post init. This, indeed, would increase post-init size. Yet with the compiler issue no question arises anyway as to how this needs doing. Jan
On 10/28/24 05:18, Andrew Cooper wrote: > We've got a perfectly good vendor abstraction already for microcode. No need > for a second ad-hoc one in microcode_scan_module(). > > This is in preparation to use ucode_ops.cpio_path in multiple places. > > These paths are only used during __init, so take the opportunity to move them > into __initconst. > > 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> > --- Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>
diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c index 0fe869eff119..c7a779c1d885 100644 --- a/xen/arch/x86/cpu/microcode/amd.c +++ b/xen/arch/x86/cpu/microcode/amd.c @@ -443,11 +443,15 @@ static struct microcode_patch *cf_check cpu_request_microcode( return patch; } +static const char __initconst amd_cpio_path[] = + "kernel/x86/microcode/AuthenticAMD.bin"; + static const struct microcode_ops __initconst_cf_clobber amd_ucode_ops = { .cpu_request_microcode = cpu_request_microcode, .collect_cpu_info = collect_cpu_info, .apply_microcode = apply_microcode, .compare_patch = compare_patch, + .cpio_path = amd_cpio_path, }; void __init ucode_probe_amd(struct microcode_ops *ops) diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c index fc31ab35c3c8..6a87390ab770 100644 --- a/xen/arch/x86/cpu/microcode/core.c +++ b/xen/arch/x86/cpu/microcode/core.c @@ -170,25 +170,19 @@ static int __init cf_check parse_ucode(const char *s) } custom_param("ucode", parse_ucode); +static struct microcode_ops __ro_after_init ucode_ops; + static void __init microcode_scan_module(struct boot_info *bi) { uint64_t *_blob_start; unsigned long _blob_size; struct cpio_data cd; - const char *p = NULL; int i; ucode_blob.size = 0; if ( !opt_scan ) return; - if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD ) - p = "kernel/x86/microcode/AuthenticAMD.bin"; - else if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ) - p = "kernel/x86/microcode/GenuineIntel.bin"; - else - return; - /* * Try all modules and see whichever could be the microcode blob. */ @@ -207,7 +201,7 @@ static void __init microcode_scan_module(struct boot_info *bi) } cd.data = NULL; cd.size = 0; - cd = find_cpio_data(p, _blob_start, _blob_size); + cd = find_cpio_data(ucode_ops.cpio_path, _blob_start, _blob_size); if ( cd.data ) { ucode_blob.size = cd.size; @@ -218,8 +212,6 @@ static void __init microcode_scan_module(struct boot_info *bi) } } -static struct microcode_ops __ro_after_init ucode_ops; - static DEFINE_SPINLOCK(microcode_mutex); DEFINE_PER_CPU(struct cpu_signature, cpu_sig); diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c index bad51f64724a..aad6a693e800 100644 --- a/xen/arch/x86/cpu/microcode/intel.c +++ b/xen/arch/x86/cpu/microcode/intel.c @@ -405,11 +405,15 @@ static bool __init can_load_microcode(void) return !(mcu_ctrl & MCU_CONTROL_DIS_MCU_LOAD); } +static const char __initconst intel_cpio_path[] = + "kernel/x86/microcode/GenuineIntel.bin"; + static const struct microcode_ops __initconst_cf_clobber intel_ucode_ops = { .cpu_request_microcode = cpu_request_microcode, .collect_cpu_info = collect_cpu_info, .apply_microcode = apply_microcode, .compare_patch = compare_patch, + .cpio_path = intel_cpio_path, }; void __init ucode_probe_intel(struct microcode_ops *ops) diff --git a/xen/arch/x86/cpu/microcode/private.h b/xen/arch/x86/cpu/microcode/private.h index c72f060ac394..c9dd8ba066f9 100644 --- a/xen/arch/x86/cpu/microcode/private.h +++ b/xen/arch/x86/cpu/microcode/private.h @@ -59,6 +59,13 @@ struct microcode_ops { */ enum microcode_match_result (*compare_patch)( const struct microcode_patch *new, const struct microcode_patch *old); + + /* + * For Linux inird microcode compatibliity. + * + * The path where this vendor's microcode can be found in CPIO. + */ + const char *cpio_path; }; /*
We've got a perfectly good vendor abstraction already for microcode. No need for a second ad-hoc one in microcode_scan_module(). This is in preparation to use ucode_ops.cpio_path in multiple places. These paths are only used during __init, so take the opportunity to move them into __initconst. 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/amd.c | 4 ++++ xen/arch/x86/cpu/microcode/core.c | 14 +++----------- xen/arch/x86/cpu/microcode/intel.c | 4 ++++ xen/arch/x86/cpu/microcode/private.h | 7 +++++++ 4 files changed, 18 insertions(+), 11 deletions(-)