x86/boot: Fix comment about setting up the real mode stack

[PATCH] x86/boot: Fix comment about setting up the real mode stack

Posted by Andrew Cooper 1 year, 2 months ago

It may have taken a while, but it occurs to me that the mentioned commit fixed
a second problem too.

On entering trampoline_boot_cpu_entry(), %esp points at the trampoline stack,
but in a 32bit flat segment.  It happens to be page aligned.

When dropping into 16bit mode, the stack segment operates on %sp, preserving
the upper bits.  Prior to 1ed76797439e, the top nibble of %sp would depend on
where the trampoline was placed in low memory, and only had a 1/16 chance of
being 0 and therefore operating on the intended stack.

There was a 15/16 chance of using a different page in the trampoline as if it
were the stack.  Therefore, zeroing %esp was correct, but for more reasons
than realised at the time.

Update the comment to explain why zeroing %esp is correct in all cases.  Move
it marginally earlier to when a 16bit stack segment is first loaded.

Fixes: 1ed76797439e ("x86/boot: fix BIOS memory corruption on certain IBM systems")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Daniel P. Smith <dpsmith@apertussolutions.com>
CC: Frediano Ziglio <frediano.ziglio@cloud.com>
CC: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
 xen/arch/x86/boot/trampoline.S | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S
index 924bda37c1b7..f5a1d61280c5 100644
--- a/xen/arch/x86/boot/trampoline.S
+++ b/xen/arch/x86/boot/trampoline.S
@@ -176,6 +176,12 @@ trampoline_boot_cpu_entry:
         mov     %eax,%gs
         mov     %eax,%ss
 
+        /*
+         * The stack is at trampoline_phys + 64k, which for a 16bit stack
+         * segment wants %sp starting at 0.
+         */
+        xor     %esp, %esp
+
         /* Switch to pseudo-rm CS, enter real mode, and flush insn queue. */
         mov     %cr0,%eax
         dec     %eax
@@ -190,8 +196,6 @@ trampoline_boot_cpu_entry:
         mov     %ax,%es
         mov     %ax,%ss
 
-        /* Initialise stack pointer and IDT, and enable irqs. */
-        xor     %esp,%esp
         lidt    bootsym(rm_idt)
         sti
 

base-commit: 41c80496084aa3601230e01c3bc579a0a6d8359a
prerequisite-patch-id: 6eb3b54ccd19effe3a89768e0ec5c7a2496a233a
prerequisite-patch-id: b9c480479c1f4021e9c3fe659811e28bf88f6eca
prerequisite-patch-id: 68f0d0fff4312fb059861efddbef95ddf4635b0e
prerequisite-patch-id: 66902cf11d58181ff63b8ee4efb4078df5828490
-- 
2.39.5

Re: [PATCH] x86/boot: Fix comment about setting up the real mode stack

Posted by Frediano Ziglio 1 year, 2 months ago

On Thu, Nov 14, 2024 at 6:22 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> It may have taken a while, but it occurs to me that the mentioned commit fixed
> a second problem too.
>
> On entering trampoline_boot_cpu_entry(), %esp points at the trampoline stack,
> but in a 32bit flat segment.  It happens to be page aligned.
>
> When dropping into 16bit mode, the stack segment operates on %sp, preserving
> the upper bits.  Prior to 1ed76797439e, the top nibble of %sp would depend on
> where the trampoline was placed in low memory, and only had a 1/16 chance of
> being 0 and therefore operating on the intended stack.
>
> There was a 15/16 chance of using a different page in the trampoline as if it
> were the stack.  Therefore, zeroing %esp was correct, but for more reasons
> than realised at the time.
>

It's not clear the additional reasons, even the original commit
mentioned wrong usage of upper bits.

> Update the comment to explain why zeroing %esp is correct in all cases.  Move
> it marginally earlier to when a 16bit stack segment is first loaded.
>

I don't see such an explanation, I mean, from "The stack is at
trampoline_phys + 64k, which for a 16bit stack segment wants %sp
starting at 0" I could assume "xor %sp, %sp" is correct too.

> Fixes: 1ed76797439e ("x86/boot: fix BIOS memory corruption on certain IBM systems")

Does this commit really "fixes" something.

The subject "Fix comment about setting up the real mode stack" seems
to indicate an update of a comment, is it considered a fix?

This commit also moves the initialisation of %esp. Is there a reason for this?

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Daniel P. Smith <dpsmith@apertussolutions.com>
> CC: Frediano Ziglio <frediano.ziglio@cloud.com>
> CC: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> ---
>  xen/arch/x86/boot/trampoline.S | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S
> index 924bda37c1b7..f5a1d61280c5 100644
> --- a/xen/arch/x86/boot/trampoline.S
> +++ b/xen/arch/x86/boot/trampoline.S
> @@ -176,6 +176,12 @@ trampoline_boot_cpu_entry:
>          mov     %eax,%gs
>          mov     %eax,%ss
>
> +        /*
> +         * The stack is at trampoline_phys + 64k, which for a 16bit stack
> +         * segment wants %sp starting at 0.
> +         */
> +        xor     %esp, %esp
> +
>          /* Switch to pseudo-rm CS, enter real mode, and flush insn queue. */
>          mov     %cr0,%eax
>          dec     %eax
> @@ -190,8 +196,6 @@ trampoline_boot_cpu_entry:
>          mov     %ax,%es
>          mov     %ax,%ss
>
> -        /* Initialise stack pointer and IDT, and enable irqs. */

Fine, surely this commit without stack pointer handling is useless.

> -        xor     %esp,%esp
>          lidt    bootsym(rm_idt)
>          sti
>
>
> base-commit: 41c80496084aa3601230e01c3bc579a0a6d8359a
> prerequisite-patch-id: 6eb3b54ccd19effe3a89768e0ec5c7a2496a233a
> prerequisite-patch-id: b9c480479c1f4021e9c3fe659811e28bf88f6eca
> prerequisite-patch-id: 68f0d0fff4312fb059861efddbef95ddf4635b0e
> prerequisite-patch-id: 66902cf11d58181ff63b8ee4efb4078df5828490

Frediano

Re: [PATCH] x86/boot: Fix comment about setting up the real mode stack

Posted by Jan Beulich 1 year, 2 months ago

On 14.11.2024 19:22, Andrew Cooper wrote:
> It may have taken a while, but it occurs to me that the mentioned commit fixed
> a second problem too.
> 
> On entering trampoline_boot_cpu_entry(), %esp points at the trampoline stack,
> but in a 32bit flat segment.  It happens to be page aligned.
> 
> When dropping into 16bit mode, the stack segment operates on %sp, preserving
> the upper bits.  Prior to 1ed76797439e, the top nibble of %sp would depend on
> where the trampoline was placed in low memory, and only had a 1/16 chance of
> being 0 and therefore operating on the intended stack.
> 
> There was a 15/16 chance of using a different page in the trampoline as if it
> were the stack.  Therefore, zeroing %esp was correct, but for more reasons
> than realised at the time.

I'm afraid I don't follow this analysis. Said commit replaced clearing of %sp
by clearing of %esp. That made no difference for anything using the 16-bit
register. I don't see how the top nibble of %sp could have been non-zero
prior to that change.

> Update the comment to explain why zeroing %esp is correct in all cases.  Move
> it marginally earlier to when a 16bit stack segment is first loaded.

The movement is fine, and the comment is fine by itself, too. It doesn't
cover the significance of using 32-bit operand size, though (which may or may
not be relevant, to a fair degree depending on the above).

Jan

> --- a/xen/arch/x86/boot/trampoline.S
> +++ b/xen/arch/x86/boot/trampoline.S
> @@ -176,6 +176,12 @@ trampoline_boot_cpu_entry:
>          mov     %eax,%gs
>          mov     %eax,%ss
>  
> +        /*
> +         * The stack is at trampoline_phys + 64k, which for a 16bit stack
> +         * segment wants %sp starting at 0.
> +         */
> +        xor     %esp, %esp
> +
>          /* Switch to pseudo-rm CS, enter real mode, and flush insn queue. */
>          mov     %cr0,%eax
>          dec     %eax
> @@ -190,8 +196,6 @@ trampoline_boot_cpu_entry:
>          mov     %ax,%es
>          mov     %ax,%ss
>  
> -        /* Initialise stack pointer and IDT, and enable irqs. */
> -        xor     %esp,%esp
>          lidt    bootsym(rm_idt)
>          sti
>

Re: [PATCH] x86/boot: Fix comment about setting up the real mode stack

Posted by Andrew Cooper 11 months, 2 weeks ago

On 15/11/2024 9:28 am, Jan Beulich wrote:
> On 14.11.2024 19:22, Andrew Cooper wrote:
>> It may have taken a while, but it occurs to me that the mentioned commit fixed
>> a second problem too.
>>
>> On entering trampoline_boot_cpu_entry(), %esp points at the trampoline stack,
>> but in a 32bit flat segment.  It happens to be page aligned.
>>
>> When dropping into 16bit mode, the stack segment operates on %sp, preserving
>> the upper bits.  Prior to 1ed76797439e, the top nibble of %sp would depend on
>> where the trampoline was placed in low memory, and only had a 1/16 chance of
>> being 0 and therefore operating on the intended stack.
>>
>> There was a 15/16 chance of using a different page in the trampoline as if it
>> were the stack.  Therefore, zeroing %esp was correct, but for more reasons
>> than realised at the time.
> I'm afraid I don't follow this analysis. Said commit replaced clearing of %sp
> by clearing of %esp.

Correct

> That made no difference for anything using the 16-bit
> register.

True, but Xen's 16bit code isn't very relevant to the analysis. 
Fujitsu's BIOS is.

> I don't see how the top nibble of %sp could have been non-zero
> prior to that change.

Oh, that's a typo.  It should have been the 5th nibble of %esp.

Said nibble depends entirely on where the trampoline is placed in low
memory.

We first enter the trampoline in 32bit flat mode with %esp being the
absolute address of the stack. i.e. it's 0x000yyyyy with a 15/16th's
chance of the 5th nibble being non-zero.

Then we drop down into Real mode (non flat, because the trampoline never
overlaps the IVT at 0).  At this point we used to zero %sp which
preserves %esp's 5th nibble.

And in the case that went wrong, INT $0x15 corrupted memory that
happened to be in the Xen image.

Anyway, when I was debugging 11 years ago, I noticed that %esp was
nonzero in its upper half and, despite deciding this was suspicious,
couldn't figure out why and zeroing it all fixed the memory corruption.

I also didn't appreciate that `xor %sp, %sp` was strongly dependent on
the trampoline being at exactly trampoline_start + 64k.

Anyway, given that everyone seemed to be confused, I guess I need to try
rewriting the commit message.

~Andrew

Re: [PATCH] x86/boot: Fix comment about setting up the real mode stack

Posted by Frediano Ziglio 1 year, 2 months ago

On Fri, Nov 15, 2024 at 9:29 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 14.11.2024 19:22, Andrew Cooper wrote:
> > It may have taken a while, but it occurs to me that the mentioned commit fixed
> > a second problem too.
> >
> > On entering trampoline_boot_cpu_entry(), %esp points at the trampoline stack,
> > but in a 32bit flat segment.  It happens to be page aligned.
> >
> > When dropping into 16bit mode, the stack segment operates on %sp, preserving
> > the upper bits.  Prior to 1ed76797439e, the top nibble of %sp would depend on
> > where the trampoline was placed in low memory, and only had a 1/16 chance of
> > being 0 and therefore operating on the intended stack.
> >
> > There was a 15/16 chance of using a different page in the trampoline as if it
> > were the stack.  Therefore, zeroing %esp was correct, but for more reasons
> > than realised at the time.
>
> I'm afraid I don't follow this analysis. Said commit replaced clearing of %sp
> by clearing of %esp. That made no difference for anything using the 16-bit
> register. I don't see how the top nibble of %sp could have been non-zero
> prior to that change.
>

I think it refers to 1ed76797439e, not this change.

> > Update the comment to explain why zeroing %esp is correct in all cases.  Move
> > it marginally earlier to when a 16bit stack segment is first loaded.
>
> The movement is fine, and the comment is fine by itself, too. It doesn't
> cover the significance of using 32-bit operand size, though (which may or may
> not be relevant, to a fair degree depending on the above).
>

I assume a usage in the firmware, besides, we switch to 32/64 at some
point too so having the upper part 0 is safer in any case.

> Jan
>
> > --- a/xen/arch/x86/boot/trampoline.S
> > +++ b/xen/arch/x86/boot/trampoline.S
> > @@ -176,6 +176,12 @@ trampoline_boot_cpu_entry:
> >          mov     %eax,%gs
> >          mov     %eax,%ss
> >
> > +        /*
> > +         * The stack is at trampoline_phys + 64k, which for a 16bit stack
> > +         * segment wants %sp starting at 0.
> > +         */
> > +        xor     %esp, %esp
> > +
> >          /* Switch to pseudo-rm CS, enter real mode, and flush insn queue. */
> >          mov     %cr0,%eax
> >          dec     %eax
> > @@ -190,8 +196,6 @@ trampoline_boot_cpu_entry:
> >          mov     %ax,%es
> >          mov     %ax,%ss
> >
> > -        /* Initialise stack pointer and IDT, and enable irqs. */
> > -        xor     %esp,%esp
> >          lidt    bootsym(rm_idt)
> >          sti
> >
>