Re: CONFIG_KCOV causing crash in svm_vcpu_run()

[Eric Biggers]

On Mon, May 14, 2018 at 07:14:41AM +0200, Dmitry Vyukov wrote:
> On Mon, May 14, 2018 at 5:02 AM, Eric Biggers <ebiggers3@xxxxxxxxx> wrote:
> > Sorry, messed up address for KVM mailing list.  See message below.
> >
> > On Sun, May 13, 2018 at 08:00:07PM -0700, Eric Biggers wrote:
> >> With CONFIG_KCOV=y and an AMD processor, running the following program crashes
> >> the kernel with no output (I'm testing in a VM, so it's using nested
> >> virtualization):
> >>
> >>       #include <fcntl.h>
> >>       #include <linux/kvm.h>
> >>       #include <sys/ioctl.h>
> >>
> >>       int main()
> >>       {
> >>               int dev, vm, cpu;
> >>               char page[4096] __attribute__((aligned(4096))) = { 0 };
> >>               struct kvm_userspace_memory_region memreg = {
> >>                       .memory_size = 4096,
> >>                       .userspace_addr = (unsigned long)page,
> >>               };
> >>               dev = open("/dev/kvm", O_RDONLY);
> >>               vm = ioctl(dev, KVM_CREATE_VM, 0);
> >>               cpu = ioctl(vm, KVM_CREATE_VCPU, 0);
> >>               ioctl(vm, KVM_SET_USER_MEMORY_REGION, &memreg);
> >>               ioctl(cpu, KVM_RUN, 0);
> >>       }
> >>
> >> It bisects down to commit b2ac58f90540e39 ("KVM/SVM: Allow direct access to
> >> MSR_IA32_SPEC_CTRL").  The bug is apparently that due to the new code for
> >> managing the SPEC_CTRL MSR, __sanitizer_cov_trace_pc() is being called from
> >> svm_vcpu_run() before the host's MSR_GS_BASE has been restored, which causes a
> >> crash somehow.  The following patch fixes it, though I don't know that it's the
> >> right solution; maybe KCOV should be disabled in the function instead, or maybe
> >> there's a more fundamental problem.  What do people think?
> 
> 
> If __sanitizer_cov_trace_pc() crashes, I would expect there must be
> few more of them here:
> 
> if (unlikely(!msr_write_intercepted(vcpu, MSR_IA32_SPEC_CTRL)))
>     svm->spec_ctrl = native_read_msr(MSR_IA32_SPEC_CTRL);
> 
> if (svm->spec_ctrl)
>     native_wrmsrl(MSR_IA32_SPEC_CTRL, 0);
> 
> Compiler inserts these callbacks into every basic block/edge.. Aren't there?
> 
> Unfortunately we don't have an attribute that disables instrumentation
> of a single function. This is currently possible only on file level.
> 

Yes, due to the code dealing with MSR_IA32_SPEC_CTRL, there were several calls
to __sanitizer_cov_trace_pc() before the write to MSR_GS_BASE.  The patch I
tested moves the write to MSR_GS_BASE to before all of them, so it's once again
the first thing after the asm block.  Again I'm not sure it's the proper
solution, but it did make it stop crashing.

Also I'm guessing this isn't specific to nested virtualization; I just didn't
have KCOV enabled on the host, thus the host didn't crash.

- Eric