Re: [PATCH v2 6/7] KVM: s390: Take srcu when importing watchpoint data

Am 04.08.26 um 20:40 schrieb [email protected]:
> 
> [Severity: High]
> Could holding the SRCU read lock across this call introduce a userfaultfd
> deadlock?
> 
> Inside kvm_s390_import_bp_data(), the very first operation is:
> 
> arch/s390/kvm/guestdbg.c:kvm_s390_import_bp_data() {
>      ...
>      bp_data = memdup_array_user(dbg->arch.hw_bp, dbg->arch.nr_hw_bp,
>                                  sizeof(*bp_data));
>      ...
> }
> 
> If user space maps this array using userfaultfd, the VCPU thread will block
> in copy_from_user() while holding the kvm->srcu read lock. If the user space
> userfaultfd handler attempts to execute a VM-wide KVM ioctl that modifies
> memslots (such as KVM_SET_USER_MEMORY_REGION), that ioctl will call
> synchronize_srcu(&kvm->srcu). The VCPU thread would wait for the page fault
> to be resolved, and the userfaultfd handler would wait for the SRCU lock to
> be released.


Now I am puzzled. Sashiko basically asked for srcu protection for the guest access
in the same review series. But I want to followup nevertheless.


There are other places in KVM code which do access guest memory under the srcu.

1.
KVM_SET_NESTED_STATE — arch/x86/kvm/x86.c
               idx = srcu_read_lock(&vcpu->kvm->srcu);
               r = kvm_x86_ops.nested_ops->set_state(vcpu, user_kvm_nested_state, &kvm_state);
               srcu_read_unlock(&vcpu->kvm->srcu, idx);
with  vmx_set_nested_state() then copying  the vmcs12 from user


2.
KVM_GET_MSRS / KVM_SET_MSRS — arch/x86/kvm/x86.c

       case KVM_SET_MSRS: {
               int idx = srcu_read_lock(&vcpu->kvm->srcu);
               r = msr_io(vcpu, argp, do_set_msr, 0);
               srcu_read_unlock(&vcpu->kvm->srcu, idx);
               break;
       }
argp goes in as a raw void __user *, and msr_io() does both user copies inside that section:

       if (copy_from_user(&msrs, user_msrs, sizeof(msrs)))


3.
KVM_S390_MEM_OP
kvm_s390_vm_mem_op_abs() has
         scoped_guard(srcu, &kvm->srcu) {
[...]
                 if (acc_mode == GACC_STORE && copy_from_user(tmpbuf, uaddr, mop->size))
                         return -EFAULT;


4.
KVM_PRE_FAULT_MEMORY generic KVM holds srcu across a loop of kvm_arch_vcpu_pre_fault_memory() calls that fault in guest memory,

5.
and last but not least,isnt vcpu_run also holding kvm->srcu when handling page faults?

In the end the cycle seems userspace constructed. It deadlocks only if its own uffd handler
is made to depend on a vcpu blocked on that same handler.The faulting side waits in
TASK_KILLABLE (mm/userfaultfd.c:2673) — SIGKILL to the process releases the srcu section
and lets the other thread finish.  So we can get out of it.


So I think this finding is wrong. Paolo?