Re: [PATCH] x86/ucode: Exit early from early_update_cache() if loading not available

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

On Thu, Jun 01, 2023 at 03:38:13PM +0100, Andrew Cooper wrote:
> If for any reason early_microcode_init() concludes that no microcode loading
> is available, early_update_cache() will fall over a NULL function pointer:
> 
>   (XEN) Xen call trace:
>   (XEN)    [<ffff82d04037372e>] R show_code+0x91/0x18f
>   (XEN)    [<ffff82d040373a49>] F show_execution_state+0x2d/0x1fc
>   (XEN)    [<ffff82d040374210>] F fatal_trap+0x87/0x19a
>   (XEN)    [<ffff82d040647f2c>] F init_idt_traps+0/0x1bd
>   (XEN)    [<ffff82d04063854f>] F early_page_fault+0x8f/0x94
>   (XEN)    [<0000000000000000>] F 0000000000000000
>   (XEN)    [<ffff82d040628c46>] F 
> arch/x86/cpu/microcode/core.c#early_update_cache+0x11/0x74
>   (XEN)    [<ffff82d040628e5c>] F microcode_init_cache+0x5a/0x5c
>   (XEN)    [<ffff82d04064388f>] F __start_xen+0x1e11/0x27ee
>   (XEN)    [<ffff82d040206184>] F __high_start+0x94/0xa0
> 
> which is actually parse_blob()'s use of ucode_ops.collect_cpu_info.
> 
> Skip trying to cache anything if microcode loading is unavailable.
> [...]
> ---
>  xen/arch/x86/cpu/microcode/core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/xen/arch/x86/cpu/microcode/core.c 
> b/xen/arch/x86/cpu/microcode/core.c
> index 5a5c0a8c70db..9029301107d6 100644
> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -789,6 +789,9 @@ int __init microcode_init_cache(unsigned long *module_map,
>  {
>      int rc = 0;
>  
> +    if ( !ucode_ops.apply_microcode )
> +        return -ENODEV;
> +
>      if ( ucode_scan )
>          /* Need to rescan the modules because they might have been relocated 
> */
>          microcode_scan_module(module_map, mbi);

Ugh. These bugs are forever. IMO, it would be helpful to have a default set
of stubs (ucode_ops_default?) that unconditionally return -ENODEV when
called. At least the whole system won't crash under our feet if we forgot
an "if ( !ucode_ops.foo ) return -1".

It's still imperfect but there's far less room for errors.

Alejandro