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 <[email protected]>
> 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
