Re: CONFIG_KCOV causing crash in svm_vcpu_run()

[Eric Biggers <ebiggers@kernel.org>]

Reviving this old thread because it wasn't fully fixed after all...

On Tue, May 15, 2018 at 07:33:48AM +0200, Dmitry Vyukov wrote:
> On Mon, May 14, 2018 at 7:25 PM, Eric Biggers <ebiggers3@gmail.com> 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@gmail.com> 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.
> 
> From KCOV perspective:
> This is definitely the simplest and less intrusive solution.
> It's somewhat unreliable. But it's hard to tell if/when it will
> actually break in practice. Compiler can decide to insert the callback
> after asm block, or a branch can be added to wrmsrl (e.g. under some
> debug config). More reliable solution would be to restore registers in
> asm block itself, or move this to a separate file and disable
> instrumentation of that file (though, will not save from non-inlined
> wrmsrl). But again, the proposed solution may work well for the next
> 10 years, so additional complexity may not worth it.
> 
> Btw, I don't see anything about fs/gs in vmx_vcpu_run. Is it VMLAUNCH
> that saves/restores them?

So it turns out there *is* a branch in wrmsrl() when CONFIG_PARAVIRT=y &&
CONFIG_PARAVIRT_DEBUG=y, and that causes the same crash: the compiler inserts a
call to __sanitizer_cov_trace_pc() prior to the GS_BASE register being restored
in svm_vcpu_run().

	#ifdef CONFIG_PARAVIRT_DEBUG
	#define PVOP_TEST_NULL(op)      BUG_ON(pv_ops.op == NULL)
	#else
	#define PVOP_TEST_NULL(op)      ((void)pv_ops.op)
	#endif

Dmitry, in the long run maybe this should be solved by adding a function
attribute to gcc that disables coverage for a function?

But for now maybe CONFIG_KCOV should depend on !CONFIG_PARAVIRT_DEBUG?  Or does
anyone have a better idea?  Alternatively as Dmitry suggested, svm_vcpu_run()
could be moved to a separate file and compiled with different flags...

- Eric