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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.05.2023 12:51, Roger Pau Monné wrote:
> On Tue, May 02, 2023 at 12:28:55PM +0200, Jan Beulich wrote:
>> On 02.05.2023 11:54, 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.
>>
>> I'm curious: What firmware loads Xen directly? I thought there was
>> always a boot loader involved (except for xen.efi of course).
> 
> This was a result of a bug in firmware plus a bug in grub, there's
> also one pending change for grub, see:
> 
> https://lists.gnu.org/archive/html/grub-devel/2023-04/msg00157.html
> 
> The firmware would return error for some calls to Boot Services
> allocate_pages method, and that triggered a bug in grub that resulted
> in the memory allocated for Xen not being aligned as requested.
> 
>> I'm further a little puzzled by this talking about alignment and not
>> xen.efi: xen.gz only specifies alignment for MB2 afaik. For MB1 all
>> it does specify is the physical address (2Mb) that it wants to be
>> loaded at. So maybe MB2 wants mentioning here as well, for clarity?
> 
> "We have found a broken firmware where grub2 would end up loading Xen
> at a non 2M aligned region when using the multiboot2 protocol, and
> that caused a very difficult to debug triple fault."
> 
> Would that be better?

Yes indeed, thanks.

>>>> @@ -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
>>
>> Or yet more simply "test $..., %esi" and then "jnz ..."?
> 
> As replied to Andrew, I would rather keep this inline with the address
> used to build the PDE, which is sym_esi(_start).

Well, I won't insist, and you've got Andrew's R-b already. (What I would
appreciate though as a minimal change is to switch from AND to TEST. We
really should avoid using AND or SUB when in fact we only care about the
flags output, and hence TEST or CMP can be used. It doesn't matter much
on one-time paths like the one here, but each unnecessary use sets a bad
precedent.)

Jan