linux-kernel - Re: [PATCH] KVM: selftests: Fix hyperv_features test failure when built on Clang

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] [day] [month] [year] [list]

Message-ID: <CAHVum0fSz26O0KrmNYPQzpUHBBsf-invuow9gV5dWZPUGJRm8A@mail.gmail.com>
Date:   Wed, 21 Sep 2022 23:03:24 -0700
From:   Vipin Sharma <vipinsh@...gle.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     Jim Mattson <jmattson@...gle.com>, pbonzini@...hat.com,
        vkuznets@...hat.com, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KVM: selftests: Fix hyperv_features test failure when
 built on Clang

On Wed, Sep 21, 2022 at 4:46 PM Sean Christopherson <seanjc@...gle.com> wrote:
>
> On Wed, Sep 21, 2022, Jim Mattson wrote:
> > On Wed, Sep 21, 2022 at 4:11 PM Vipin Sharma <vipinsh@...gle.com> wrote:
> > >
> > > hyperv_features test fails when built on Clang. It throws error:
> > >
> > >          Failed guest assert: !hcall->ud_expected || res == hcall->expect at
> > >          x86_64/hyperv_features.c:90
> > >
> > > On GCC, EAX is set to 0 before the hypercall whereas in Clang it is not,
> > > this causes EAX to have garbage value when hypercall is returned in Clang
> > > binary.
> > >
> > > Fix by executing the guest assertion only when ud_expected is false.
>
> TL;DR: please rewrite the changelog to explain the actual bug (checking the
> result when the hypercall is expected to fault) and the fix, and only mention the
> gcc vs. clang behavior as a footnote.

On it.
>
> As Jim pointed out, the bug has nothing to do with clang.  Ha, figured out why
> gcc passes; it uses RAX as the scratch reg that the asm blob loads into R8,
> i.e. loads RAX with @output_address.  So ignore my earlier suggestion of:
>
>         *hv_status = -EFAULT,
>
> even better is to do:
>
> diff --git a/tools/testing/selftests/kvm/x86_64/hyperv_features.c b/tools/testing/selftests/kvm/x86_64/hyperv_features.c
> index 79ab0152d281..673085f6edfd 100644
> --- a/tools/testing/selftests/kvm/x86_64/hyperv_features.c
> +++ b/tools/testing/selftests/kvm/x86_64/hyperv_features.c
> @@ -26,7 +26,8 @@ static inline uint8_t hypercall(u64 control, vm_vaddr_t input_address,
>                      : "=a" (*hv_status),
>                        "+c" (control), "+d" (input_address),
>                        KVM_ASM_SAFE_OUTPUTS(vector)
> -                    : [output_address] "r"(output_address)
> +                    : [output_address] "r"(output_address),
> +                      "a"(-EFAULT)
>                      : "cc", "memory", "r8", KVM_ASM_SAFE_CLOBBERS);
>         return vector;
>  }
>
> so that there is zero chance of getting a false positive due to the compiler
> (but not KVM) modifying RAX.
>

Taking it.

> Anyways, this bug is not clang's fault, with above patch it fails as "expected".
>
> ==== Test Assertion Failure ====
>   x86_64/hyperv_features.c:622: false
>   pid=283847 tid=283847 errno=4 - Interrupted system call
>      1  0x0000000000402842: guest_test_hcalls_access at hyperv_features.c:622
>      2   (inlined by) main at hyperv_features.c:642
>      3  0x00007f23fc513082: ?? ??:0
>      4  0x0000000000402ebd: _start at ??:?
>   Failed guest assert: !hcall->ud_expected || res == hcall->expect at x86_64/hyperv_features.c:90
> arg1 = 0, arg2 = fffffff2
>
>
> > > Fixes: cc5851c6be86 ("KVM: selftests: Use exception fixup for #UD/#GP Hyper-V MSR/hcall tests")
> > > Signed-off-by: Vipin Sharma <vipinsh@...gle.com>
> > > Suggested-by: Sean Christopherson <seanjc@...gle.com>
> > >
> > > ---
> > >  tools/testing/selftests/kvm/x86_64/hyperv_features.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > In case Sean doesn't point it out, be wary of starting a shortlog with
> > "Fix." You may later regret it.

I am doing it already! :D

>
> Heh, I think our record is "Really, really fix xyz" for a shortlog.
>
> > Also, I think the "clang" part is a red herring. You are fixing a
> > latent bug in the code.
>
> Ya, it's definitely a good idea to call out why a bug was missed, but clang is
> not to blame here, at all.

I agree, my understanding was not complete for this bug. I will send
out a v2 with an updated shortlog. Thanks, Jim and Sean, for pointing
it out.