Re: [PATCH 02/10] x86/ucode: Delete the microcode_init() initcall

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/10/2024 1:38 pm, Jan Beulich wrote:
> On 28.10.2024 10:18, Andrew Cooper wrote:
>> The comment highlights just how bogus this really is.  Being an initcall, the
>> boot allocator is long gone, and bootstrap_unmap() is a no-op.
> How's the boot allocator coming into the picture here? This is all about
> (un)mapping, not allocating.
>
>> The fact there is nothing to do should be a giant red flag about the validity
>> of the mappings "being freed".  Indeed, they both constitute use-after-frees.
> I can't spot any use-after-free; the pointers in question ...
>
>> --- a/xen/arch/x86/cpu/microcode/core.c
>> +++ b/xen/arch/x86/cpu/microcode/core.c
>> @@ -758,28 +758,6 @@ int microcode_update(XEN_GUEST_HANDLE(const_void) buf,
>>      return continue_hypercall_on_cpu(0, microcode_update_helper, buffer);
>>  }
>>  
>> -static int __init cf_check microcode_init(void)
>> -{
>> -    /*
>> -     * At this point, all CPUs should have updated their microcode
>> -     * via the early_microcode_* paths so free the microcode blob.
>> -     */
>> -    if ( ucode_blob.size )
>> -    {
>> -        bootstrap_unmap();
>> -        ucode_blob.size = 0;
>> -        ucode_blob.data = NULL;
>> -    }
>> -    else if ( ucode_mod.mod_end )
>> -    {
>> -        bootstrap_unmap();
>> -        ucode_mod.mod_end = 0;
>> -    }
>> -
>> -    return 0;
>> -}
>> -__initcall(microcode_init);
> ... aren't used anywhere. bootstrap_unmap() is "just in case" (perhaps indeed
> a no-op at least nowadays), and the rest is field clobbering. I'm okay with 
> the
> code change, so
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> yet I'd like to ask for the description to be "softened" some.

As I said, this could be folded into patch 9, given this particular
arrangement of the series.

The UAFs are apparent *because* the comment demonstrates a false line of
reasoning.

ucode_mod literally is used after free.  ucode=$n is genuinely buggy
today, because its a stash of a physical pointer across move_xen().

ucode_blob stashes a virtual pointer.  This was even noticed in
dc380df12acf ("x86/ucode: load microcode earlier on boot CPU")

---
It needs to rescan the modules in order to find the new virtual address
of the ucode blob because it changes during the boot process, e.g.
from 0x00000000010802fc to 0xffff83204dac52fc.
---

which highlighted the problem but duct-taped over it.

~Andrew