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

Message-ID: <YyuiWk/4TFYqf0qN@google.com>
Date:   Wed, 21 Sep 2022 23:46:34 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Jim Mattson <jmattson@...gle.com>
Cc:     Vipin Sharma <vipinsh@...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, 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.

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.

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.

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.