On Mon, May 14, 2018 at 10:25:08AM -0700, Eric Biggers wrote:
> 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.
>
Okay, this was (apparently coincidentally) fixed by commit 15e6c22fd8e5a:
"KVM: SVM: Move spec control call after restore of GS". Thanks Thomas!
- Eric
