Re: [Xen-devel] kexec and xen/arch/x86/boot/head.S trampoline

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/02/18 14:08, Trammell Hudson wrote:
> This is a belated followup to my post in 2016, which was a followup to a
> post by Ward Vandewege in 2008 about problems introduced by Xen 3.1.3's
> changes in the trampoline allocation code:
>
> https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg01208.html
>
> I've been maintaining an out-of-tree patch for using Xen with coreboot
> and the Heads runtime since my previous post and finally decided to track
> down what in coreboot was causing the issue.  It turns out that it is
> not a coreboot problem, but caused by kexec.
>
> kexec allocates a 1 page segment at 0x0 and memsets most of it to zero,
> wiping out coreboot's EBDA structure, which xen's head.S consulted to
> allocate the trampoline.  Xen's code looks like this:

This sounds like a bug as and of itself.  I presume this is to do with
IVT handling?

>
>         /* Set up trampoline segment 64k below EBDA */
>         movzwl  0x40e,%ecx          /* EBDA segment */
>         cmp     $0xa000,%ecx        /* sanity check (high) */
>         jae     0f
>         cmp     $0x4000,%ecx        /* sanity check (low) */
>         jae     1f
> 0:
>         movzwl  0x413,%ecx          /* use base memory size on failure */
>         shl     $10-4,%ecx
> 1:
>         /*
>          * Compare the value in the BDA with the information from the
>          * multiboot structure (if available) and use the smallest.
>          */
>         testb   $MBI_MEMLIMITS,(%ebx)
>         jz      2f                  /* not available? BDA value will be fine 
> */
>         mov     MB_mem_lower(%ebx),%edx
>         cmp     $0x100,%edx         /* is the multiboot value too small? */
>         jb      2f                  /* if so, do not use it */
>         shl     $10-4,%edx
>         cmp     %ecx,%edx           /* compare with BDA value */
>         cmovb   %edx,%ecx           /* and use the smaller */
>
> 2:      /* Reserve 64kb for the trampoline */
>         sub     $0x1000,%ecx
>
> Since 0x40e is zero, it goes into the base memory size fallback,
> which also results in %ecx being zero.  Since zero is less than whatever
> is in the MBI, Xen decides to use minus 0x1000 for the trampoline and faults
> soon afterwards as a result.

The original code which introduced this is 46fce9fd2b355, but I agree
that the obviously wrong when we fail the low sanity check.

> Are there reasons to prefer EBDA over mbi->mem_lower?
>
> If not, it seems that easiest way to patch it would be always trust the
> mbi lower memory value if the memlimits bit is set (which is what my
> out-of-tree patch did) and only fall back to EBDA data if the mbi->flags
> MEMLIMITS bit is not set.  And even then it would be good to to duplicate
> the guard for %ecx < 0x4000 || %ecx > 0xa000 when reading from 0x413
> and signal an error rather than faulting.

I think the expectation was that the EBDA would be more reliable than
mbi->mem_lower.  However, we recently hit a similar bug with PVH
handling (c/s a232346b1fe), and these days, the EBDA is quite likely not
to be present.

I think we probably can use mbi->mem_lower (if available and sane) by
default.  If the bootloader has messed that up, all bets are off
anyway.  Irrespective, we should fix the EBDA lower sanity check.

What is the current size of the trampoline?  ISTR Jan or Juergen (CC'd)
suggesting that there was some shrinking work which could be done.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel