Re: [PATCH] KVM: check userspace_addr for all memslots

[Paolo Bonzini]

On 11/06/20 16:44, Maxim Levitsky wrote:
> On Mon, 2020-06-01 at 04:21 -0400, Paolo Bonzini wrote:
>> The userspace_addr alignment and range checks are not performed for private
>> memory slots that are prepared by KVM itself.  This is unnecessary and makes
>> it questionable to use __*_user functions to access memory later on.  We also
>> rely on the userspace address being aligned since we have an entire family
>> of functions to map gfn to pfn.
>>
>> Fortunately skipping the check is completely unnecessary.  Only x86 uses
>> private memslots and their userspace_addr is obtained from vm_mmap,
>> therefore it must be below PAGE_OFFSET.  In fact, any attempt to pass
>> an address above PAGE_OFFSET would have failed because such an address
>> would return true for kvm_is_error_hva.
>>
>> Reported-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> 
> I bisected this patch to break a VM on my AMD system (3970X)
> 
> The reason it happens, is because I have avic enabled (which uses
> a private KVM memslot), but it is permanently disabled for that VM,
> since I enabled nesting for that VM (+svm) and that triggers the code
> in __x86_set_memory_region to set userspace_addr of the disabled
> memslot to non canonical address (0xdeadull << 48) which is later rejected in __kvm_set_memory_region
> after that patch, and that makes it silently not disable the memslot, which hangs the guest.
> 
> The call is from avic_update_access_page, which is called from svm_pre_update_apicv_exec_ctrl
> which discards the return value.
> 
> 
> I think that the fix for this would be to either make access_ok always return
> true for size==0, or __kvm_set_memory_region should treat size==0 specially
> and skip that check for it.

Or just set hva to 0.  Deletion goes through kvm_delete_memslot so that
dummy hva is not used anywhere.  If we really want to poison the hva of
deleted memslots we should not do it specially in
__x86_set_memory_region.  I'll send a patch.

Paolo