Re: [PATCH 1/2] x86/head: check base address alignment

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.05.2023 12:28, Roger Pau Monné wrote:
> On Tue, May 02, 2023 at 10:54:55AM +0100, Andrew Cooper wrote:
>> On 02/05/2023 10:22 am, Roger Pau Monne wrote:
>>> Ensure that the base address is 2M aligned, or else the page table
>>> entries created would be corrupt as reserved bits on the PDE end up
>>> set.
>>>
>>> We have found a broken firmware where the loader would end up loading
>>> Xen at a non 2M aligned region, and that caused a very difficult to
>>> debug triple fault.
>>
>> It's probably worth saying that in this case, the OEM has fixed their
>> firmware.
>>
>>>
>>> If the alignment is not as required by the page tables print an error
>>> message and stop the boot.
>>>
>>> The check could be performed earlier, but so far the alignment is
>>> required by the page tables, and hence feels more natural that the
>>> check lives near to the piece of code that requires it.
>>>
>>> Note that when booted as an EFI application from the PE entry point
>>> the alignment check is already performed by
>>> efi_arch_load_addr_check(), and hence there's no need to add another
>>> check at the point where page tables get built in
>>> efi_arch_memory_setup().
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>> ---
>>>  xen/arch/x86/boot/head.S | 9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
>>> index 0fb7dd3029f2..ff73c1d274c4 100644
>>> --- a/xen/arch/x86/boot/head.S
>>> +++ b/xen/arch/x86/boot/head.S
>>> @@ -121,6 +121,7 @@ multiboot2_header:
>>>  .Lbad_ldr_nst: .asciz "ERR: EFI SystemTable is not provided by bootloader!"
>>>  .Lbad_ldr_nih: .asciz "ERR: EFI ImageHandle is not provided by bootloader!"
>>>  .Lbad_efi_msg: .asciz "ERR: EFI IA-32 platforms are not supported!"
>>> +.Lbag_alg_msg: .asciz "ERR: Xen must be loaded at a 2Mb boundary!"
>>>  
>>>          .section .init.data, "aw", @progbits
>>>          .align 4
>>> @@ -146,6 +147,9 @@ bad_cpu:
>>>  not_multiboot:
>>>          add     $sym_offs(.Lbad_ldr_msg),%esi   # Error message
>>>          jmp     .Lget_vtb
>>> +not_aligned:
>>> +        add     $sym_offs(.Lbag_alg_msg),%esi   # Error message
>>> +        jmp     .Lget_vtb
>>>  .Lmb2_no_st:
>>>          /*
>>>           * Here we are on EFI platform. vga_text_buffer was zapped earlier
>>> @@ -670,6 +674,11 @@ trampoline_setup:
>>>          cmp     %edi, %eax
>>>          jb      1b
>>>  
>>> +        /* Check that the image base is aligned. */
>>> +        lea     sym_esi(_start), %eax
>>> +        and     $(1 << L2_PAGETABLE_SHIFT) - 1, %eax
>>> +        jnz     not_aligned
>>
>> You just want to check the value in %esi, which is the base of the Xen
>> image.  Something like:
>>
>> mov %esi, %eax
>> and ...
>> jnz
>>
>> No need to reference the _start label, or use sym_esi().
> 
> The reason for using sym_esi(_start) is because that's exactly the
> address used when building the PDE, so it's clearer to keep those in
> sync IMO.

Hmm, while I see your point using sym_esi() here merely means
subtracting __XEN_VIRT_START. Which would better remain 2Mb- (and
even 1Gb-)aligned (and if you wanted to guard for that, you could
add a build-time check instead of a runtime one, e.g. for sym_esi(0)
to be suitably aligned).

Jan

> That's also the reason for doing the check here rather than earlier,
> so it's closer to the point where the value is used and not being
> aligned would lead to corrupt entries.
> 
> Thanks, Roger.