kexec and xen/arch/x86/boot/head.S trampoline

kexec and xen/arch/x86/boot/head.S trampoline

[Trammell Hudson]

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:

        /* 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.

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.


-- 
Trammell

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

Re: kexec and xen/arch/x86/boot/head.S trampoline

[Andrew Cooper]

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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: kexec and xen/arch/x86/boot/head.S trampoline

[Trammell Hudson]

On Wed, Feb 28, 2018 at 03:07:41PM +0000, Andrew Cooper wrote:
> On 28/02/18 14:08, Trammell Hudson wrote:
> > 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?

Maybe?  That's the right address and it only populates 0x78 bytes
(in qemu) with contents, which would be about right for the interrupt
table.  I haven't tracked down where that data comes from.

> [...]
> > Are there reasons to prefer EBDA over mbi->mem_lower?
> 
> 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 do you think of something like this instead?

        /*
         * If the multiboot structure memory limits are available,
         * use it for the trampoline.
         */
        testb   $MBI_MEMLIMITS,(%ebx)
        jz      1f                  /* not available? BDA value will be fine */
        mov     MB_mem_lower(%ebx),%ecx
        cmp     $0x100,%ecx         /* is the multiboot value too small? */
        jae     2f

1:
        /* MBI not available, 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     2f
0:
        movzwl  0x413,%ecx          /* use base memory size on failure */
        cmp     $0xa000,%ecx        /* sanity check (high) */
        jae     bad_ebda
        cmp     $0x4000,%ecx        /* sanity check (low) */
        jb      bad_ebda

2:      /* Reserve 64kb for the trampoline */
        shl     $10-4,%ecx          /* convert to bytes */
        sub     $0x1000,%ecx

-- 
Trammell

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