On Wed, Oct 16, 2019 at 12:53 PM Jim Mattson <jmattson@xxxxxxxxxx> wrote:
>
> On Sat, Oct 12, 2019 at 4:59 PM Bill Wendling <morbo@xxxxxxxxxx> wrote:
> >
> > It's fragile to try to retrieve the stack pointer by taking the address
> > of a variable on the stack. For instance, clang reserves more stack
> > space than gcc here, indicating that the variable may not be at the
> > start of the stack. Instead of relying upon this to work, retrieve the
> > "%rbp" value, which contains the value of "%rsp" before stack
> > allocation.
> >
> > Signed-off-by: Bill Wendling <morbo@xxxxxxxxxx>
> > ---
> > x86/realmode.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/x86/realmode.c b/x86/realmode.c
> > index cf45fd6..7c89dd1 100644
> > --- a/x86/realmode.c
> > +++ b/x86/realmode.c
> > @@ -518,11 +518,12 @@ extern void retf_imm(void);
> >
> > static void test_call(void)
> > {
> > - u32 esp[16];
> > u32 addr;
> >
> > inregs = (struct regs){ 0 };
> > - inregs.esp = (u32)esp;
> > +
> > + // At this point the original stack pointer is in %ebp.
> > + asm volatile ("mov %%ebp, %0" : "=rm"(inregs.esp));
>
> I don't think we should assume the existence of frame pointers.
> Moreover, I think %esp is actually the value that should be saved
> here, regardless of how large the current frame is.
Never mind. After taking a closer look, esp[] is meant to provide
stack space for the code under test, but inregs.esp should point to
the top of this stack rather than the bottom. This is apparently a
long-standing bug, similar to the one Avi fixed for test_long_jmp()
in commit 4aa22949 ("realmode: fix esp in long jump test").
For consistency with test_long_jmp, I'd suggest changing the
inregs.esp assignment to:
inregs.esp = (u32)(esp+16);
Note that you absolutely must preserve the esp[] array!
> > MK_INSN(call1, "mov $test_function, %eax \n\t"
> > "call *%eax\n\t");
> > --
> > 2.23.0.700.g56cf767bdb-goog
> >
