linux-kernel - Re: [PATCH] selftests: kvm: fix sync_regs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 8 Oct 2019 11:40:07 -0700
From:   Sean Christopherson <sean.j.christopherson@...el.com>
To:     Jim Mattson <jmattson@...gle.com>
Cc:     Vitaly Kuznetsov <vkuznets@...hat.com>,
        kvm list <kvm@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: [PATCH] selftests: kvm: fix sync_regs_test with newer gccs

On Tue, Oct 08, 2019 at 11:29:32AM -0700, Jim Mattson wrote:
> On Tue, Oct 8, 2019 at 11:08 AM Vitaly Kuznetsov <vkuznets@...hat.com> wrote:
> >
> > Commit 204c91eff798a ("KVM: selftests: do not blindly clobber registers in
> >  guest asm") was intended to make test more gcc-proof, however, the result
> > is exactly the opposite: on newer gccs (e.g. 8.2.1) the test breaks with
> >
> > ==== Test Assertion Failure ====
> >   x86_64/sync_regs_test.c:168: run->s.regs.regs.rbx == 0xBAD1DEA + 1
> >   pid=14170 tid=14170 - Invalid argument
> >      1  0x00000000004015b3: main at sync_regs_test.c:166 (discriminator 6)
> >      2  0x00007f413fb66412: ?? ??:0
> >      3  0x000000000040191d: _start at ??:?
> >   rbx sync regs value incorrect 0x1.
> >
> > Apparently, compile is still free to play games with registers even
> > when they have variables attaches.
> >
> > Re-write guest code with 'asm volatile' by embedding ucall there and
> > making sure rbx is preserved.
> >
> > Fixes: 204c91eff798a ("KVM: selftests: do not blindly clobber registers in guest asm")
> > Signed-off-by: Vitaly Kuznetsov <vkuznets@...hat.com>
> > ---
> >  .../selftests/kvm/x86_64/sync_regs_test.c     | 21 ++++++++++---------
> >  1 file changed, 11 insertions(+), 10 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/x86_64/sync_regs_test.c b/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
> > index 11c2a70a7b87..5c8224256294 100644
> > --- a/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
> > +++ b/tools/testing/selftests/kvm/x86_64/sync_regs_test.c
> > @@ -22,18 +22,19 @@
> >
> >  #define VCPU_ID 5
> >
> > +#define UCALL_PIO_PORT ((uint16_t)0x1000)
> > +
> > +/*
> > + * ucall is embedded here to protect against compiler reshuffling registers
> > + * before calling a function. In this test we only need to get KVM_EXIT_IO
> > + * vmexit and preserve RBX, no additional information is needed.
> > + */
> >  void guest_code(void)
> >  {
> > -       /*
> > -        * use a callee-save register, otherwise the compiler
> > -        * saves it around the call to GUEST_SYNC.
> > -        */
> > -       register u32 stage asm("rbx");
> > -       for (;;) {
> > -               GUEST_SYNC(0);
> > -               stage++;
> > -               asm volatile ("" : : "r" (stage));
> > -       }
> > +       asm volatile("1: in %[port], %%al\n"
> > +                    "add $0x1, %%rbx\n"
> > +                    "jmp 1b"
> > +                    : : [port] "d" (UCALL_PIO_PORT) : "rax", "rbx");
> >  }
> A better solution might be something like:
> 
> register u32 stage = 0;
> for (;;) {
>         asm volatile("in %[port], %%al"
>              :
>              : "b" (stage), [port] "d" (UCALL_PIO_PORT)
>              : "rax");
>         stage++;
> }
> 
> (Gmail no doubt has mangled the indentation. Sorry.)

The incoming value of rbx matters, I believe it's set to 0xBAD1DEA and the
host then expects 0xBAD1DEA + 1.