Re: CONFIG_KCOV causing crash in svm_vcpu_run()

[Dmitry Vyukov]

On Mon, May 14, 2018 at 7:25 PM, Eric Biggers <ebiggers3@xxxxxxxxx> 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.

>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?