On Sat, May 30, 2020 at 11:52:44AM -0700, Linus Torvalds wrote: > > It really isn't. > > Your very first statement shows how broken it is: > > > FWIW, the kvm side of things (vhost is yet another pile of fun) is > > > > [x86] kvm_hv_set_msr_pw(): > > arch/x86/kvm/hyperv.c:1027: if (__copy_to_user((void __user *)addr, instructions, 4)) > > HV_X64_MSR_HYPERCALL > > arch/x86/kvm/hyperv.c:1132: if (__clear_user((void __user *)addr, sizeof(u32))) > > HV_X64_MSR_VP_ASSIST_PAGE > > in both cases addr comes from > > gfn = data >> HV_X64_MSR_VP_ASSIST_PAGE_ADDRESS_SHIFT; > > addr = kvm_vcpu_gfn_to_hva(vcpu, gfn); > > if (kvm_is_error_hva(addr)) > > return 1; > > Just look at that. You have _zero_ indication that 'adds" is a user > space address. It could be a kernel address. > > That kvm_vcpu_gfn_to_hva() function is a complicated mess that first > looks for the right 'memslot', and basically uses a search with a > default slot to try to figure it out. It doesn't even use locking for > any of it, but assumes the arrays are stable, and that it can use > atomics to reliably read and set the last successfully found slot. > > And none of that code verifies that the end result is a user address. kvm_is_error_hva() is return addr >= PAGE_OFFSET;
