Re: [PATCH 8/9] x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user()

[Linus Torvalds]

On Sat, May 30, 2020 at 11:39 AM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> Actually, it's somewhat less brittle than you think (on non-mips, at least)
> and not due to those long-ago access_ok().

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.

It _literally_ all depends on this optimistically lock-free code being
bug-free, and never using a slot that isn't a user slot. And as
mentioned, there _are_ non-user memslots.

It's fragile as hell.

And it's all completely and utterly pointless. ALL of the above is
incredibly much more expensive than just checking the damn address
range.

So the optimization is completely bogus to begin with, and all it
results in is that any bug in this _incredibly_ subtle code will be a
security proiblem.

And I don't understand why you mention set_fs() vs access_ok(). None
of this code has anything that messes with set_fs(). The access_ok()
is garbage and shouldn't exist, and those user accesses should all use
the checking versions and the double underscores are wrong.

I have no idea why you think the double underscores could _possibly_
be worth defending.

            Linus