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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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().

Otherwise, LGTM.

~Andrew