Re: [PATCH v3 4/4] x86/ucode: Add Kconfig option to remove microcode loading

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.01.2026 17:12, Alejandro Vallejo wrote:
> On Tue Jan 13, 2026 at 4:27 PM CET, Jan Beulich wrote:
>> On 13.01.2026 13:21, Alejandro Vallejo wrote:
>>> @@ -469,7 +471,7 @@ struct ucode_buf {
>>>      char buffer[];
>>>  };
>>>  
>>> -static long cf_check ucode_update_hcall_cont(void *data)
>>> +static long cf_check __maybe_unused ucode_update_hcall_cont(void *data)
>>>  {
>>>      struct microcode_patch *patch = NULL;
>>>      int ret, result;
>>
>> Why this change when ...
>>
>>> @@ -613,6 +615,7 @@ static long cf_check ucode_update_hcall_cont(void *data)
>>>      return ret;
>>>  }
>>>  
>>> +#ifdef CONFIG_MICROCODE_LOADING
>>
>> ... this can simply be moved up accordingly? After all ...
>>
>>>  int ucode_update_hcall(XEN_GUEST_HANDLE(const_void) buf,
>>>                         unsigned long len, unsigned int flags)
>>>  {
>>> @@ -645,6 +648,7 @@ int ucode_update_hcall(XEN_GUEST_HANDLE(const_void) buf,
>>>       */
>>>      return continue_hypercall_on_cpu(0, ucode_update_hcall_cont, buffer);
>>
>> ... this is the only user of that other function.
> 
> To minimise the scope of the ifdef. It's hard to know where things start/end
> when they cover several functions. This way it's (imo) clearer.
> 
> I don't mind much though.
> 
>>
>>> --- a/xen/arch/x86/cpu/microcode/intel.c
>>> +++ b/xen/arch/x86/cpu/microcode/intel.c
>>> @@ -408,17 +408,20 @@ 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,
>>> +    .cpu_request_microcode            = 
>>> MICROCODE_OP(cpu_request_microcode),
>>>      .collect_cpu_info                 = collect_cpu_info,
>>> -    .apply_microcode                  = apply_microcode,
>>> -    .compare                          = intel_compare,
>>> -    .cpio_path                        = intel_cpio_path,
>>> +    .apply_microcode                  = MICROCODE_OP(apply_microcode),
>>> +    .compare                          = MICROCODE_OP(intel_compare),
>>> +    .cpio_path                        = MICROCODE_OP(intel_cpio_path),
>>>  };
>>
>> While I appreciate the intention with MICROCODE_OP(), I'm not really happy
>> with function pointer members left in place just for them to be NULL
>> everywhere. What if a call site remains unguarded? With PV guests that
>> would be a privilege escalation XSA.
> 
> I see where you're coming from, but these are already NULL if microcode
> loading is not exposed by the underlying hypervisor (if any), or is blocked by
> hardware in Intel, so arguably that worry is orthogonal to this.

Yes and no. Paths taken differ between what we have now and what we will have
when your work has gone in.

> Also, only a privileged domain can perform late microcode loading, so the XSM
> check would prevent any such XSA from coming to pass. dom0 crashing the system
> on a bad hypercall (while wrong) would just be unfortunate, not a security
> issue, as far as I can tell.

Okay, together with Andrew's response it wouldn't be calls through NULL, so
perhaps indeed not an XSA. The hypercall being Dom0-only I am, however, less
convinced would necessarily matter here. We interact with remote CPUs, after
all, and hence having one which happens to run a PV DomU call through NULL
would still be in need of an XSA.

Jan