Re: [PATCH v9 6/6] s390/vfio-ap: replace guest-reachable WARNs with ratelimited warnings

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] vfio_ap: AB-BA deadlock between guests_lock and pqap_hook_rwsem
- [High] vfio_ap: returning negative errors from handle_pqap() causes guest-triggerable VM crash
- [High] vfio_ap: clobbering guest registers before fallback to userspace breaks emulation
- [High] vfio_ap: 3-way SRCU deadlock via guests_lock and kvm->lock
- [Medium] vfio_ap: dev_warn_ratelimited() uses static state, breaking per-mdev isolation
--

commit 5bf97ebc06bca2480f470ccf5163316139db70cc
Author: Anthony Krowiak <akrowiak@linux.ibm.com>

s390/vfio-ap: replace guest-reachable WARNs with ratelimited warnings

This commit replaces WARN and WARN_ONCE macros in code paths reachable by a
guest with ratelimited warning functions to prevent malicious guests from
flooding the kernel log. It also refactors handle_pqap() to hold both
guests_lock and mdevs_lock during execution, using a single exit point to
prevent one guest's mdev from suppressing messages for another.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 89efb73d70355..7b1da96146271 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -226,6 +226,58 @@ static struct vfio_ap_queue *vfio_ap_mdev_get_queue(
>  	return NULL;
>  }
>  
> +static void report_tapq_rc(struct vfio_ap_queue *q, u8 rc)
> +{
> +	if (q->matrix_mdev)
> +		dev_warn_ratelimited(mdev_dev(q->matrix_mdev->mdev),
> +				     "PQAP(TAPQ) for %02x.%04x failed with invalid rc=%#02x\n",
> +				     AP_QID_CARD(q->apqn),
> +				     AP_QID_QUEUE(q->apqn), rc);

[Severity: Medium]
Will this actually provide per-mdev isolation?

The commit explicitly states that dev_warn_ratelimited() was chosen so the
rate limit state is per-mdev. However, dev_warn_ratelimited() relies on
dev_level_ratelimited(), which defines a static DEFINE_RATELIMIT_STATE at
the call site:

include/linux/dev_printk.h:dev_level_ratelimited() {
    ...
    static DEFINE_RATELIMIT_STATE(_rs, ...);
    ...
}

Does this mean any guest spamming invalid requests will exhaust the static
rate limit for this source line, silencing the warnings for all other guests?

[ ... ]

> @@ -787,32 +836,39 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
>  
>  	apqn = vcpu->run->s.regs.gprs[0] & 0xffff;
>  
> -	/* If we do not use the AIV facility just go to userland */
> -	if (!(vcpu->arch.sie_block->eca & ECA_AIV)) {
> -		VFIO_AP_DBF_WARN("%s: AIV facility not installed: apqn=0x%04x, eca=0x%04x\n",
> -				 __func__, apqn, vcpu->arch.sie_block->eca);
> -
> -		return -EOPNOTSUPP;
> -	}
> -
> +	mutex_lock(&matrix_dev->guests_lock);

[Severity: High]
Does this code introduce an AB-BA deadlock between guests_lock and
pqap_hook_rwsem?

A guest executing PQAP causes KVM to call handle_pqap() under the read-side
of pqap_hook_rwsem:

arch/s390/kvm/s390/priv.c:kvm_s390_handle_b2() {
    ...
    down_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem);
    ...
}

This patch makes handle_pqap() unconditionally acquire guests_lock here.

Concurrently, a host thread executing a configuration change (e.g. via
get_update_locks_for_kvm()) acquires guests_lock and then attempts to
acquire the write-side of pqap_hook_rwsem:

drivers/s390/crypto/vfio_ap_ops.c:get_update_locks_for_kvm() {
    ...
    mutex_lock(&matrix_dev->guests_lock);
    if (kvm) {
        mutex_lock(&kvm->lock);
        down_write(&kvm->arch.crypto.pqap_hook_rwsem);
    }
    ...
}

[Severity: High]
Does acquiring guests_lock here also introduce a 3-way SRCU deadlock?

KVM holds srcu_read_lock(&kvm->srcu) when calling the instruction intercept
handlers. The KVM locking documentation explicitly forbids acquiring any lock
that wraps kvm->lock while holding KVM's SRCU read lock:

Documentation/virt/kvm/locking.rst:
synchronize_srcu(&kvm->srcu) is called inside critical sections for
kvm->lock ... These locks _cannot_ be taken inside a kvm->srcu read-side
critical section.

Can this result in a circular dependency where:
1. The vCPU thread holds SRCU and waits for guests_lock.
2. The mdev configuration thread holds guests_lock and waits for kvm->lock
   (via get_update_locks_for_kvm()).
3. Another KVM thread holding kvm->lock calls synchronize_srcu(&kvm->srcu),
   waiting on the vCPU thread?

>  	mutex_lock(&matrix_dev->mdevs_lock);
> +	matrix_mdev = vfio_ap_mdev_for_apqn(apqn);
> +	if (!matrix_mdev || !matrix_mdev->kvm) {
> +		ret = -ENODEV;
> +		goto out_unlock;
> +	}

[Severity: High]
Can returning a negative error code here cause a guest-triggerable VM crash?

If a guest executes PQAP for an unassigned queue or missing mdev, the patched
handle_pqap() returns -ENODEV or -EINVAL instead of returning 0 and setting a
guest CC.

KVM's instruction intercept handler (__vcpu_run) propagates non-EOPNOTSUPP
negative errors directly to userspace:

arch/s390/kvm/s390/s390.c:vcpu_post_run() {
    ...
    if (rc != -EOPNOTSUPP)
        return rc;
    ...
}

QEMU treats an unexpected KVM_RUN failure as a fatal error and terminates
the VM, allowing an unprivileged guest to crash its hypervisor process.

[ ... ]

> -	matrix_mdev = container_of(vcpu->kvm->arch.crypto.pqap_hook,
> -				   struct ap_matrix_mdev, pqap_hook);
> -
> -	/* If the there is no guest using the mdev, there is nothing to do */
> -	if (!matrix_mdev->kvm) {
> -		vfio_ap_le_guid_to_be_uuid(&matrix_mdev->mdev->uuid, uuid);
> -		VFIO_AP_DBF_WARN("%s: mdev %08lx-%04lx-%04lx-%04lx-%04lx%08lx not in use: apqn=0x%04x\n",
> -				 __func__, uuid[0],  uuid[1], uuid[2],
> -				 uuid[3], uuid[4], uuid[5], apqn);
> +	/* If we do not use the AIV facility just go to userland */
> +	if (!(vcpu->arch.sie_block->eca & ECA_AIV)) {
> +		VFIO_AP_DBF_WARN("%s: AIV facility not installed: apqn=0x%04x, eca=0x%04x\n",
> +				 __func__, apqn, vcpu->arch.sie_block->eca);
> +		dev_warn_ratelimited(mdev_dev(matrix_mdev->mdev),
> +				     "PQAP(AQIC) for %02x.%04x: AIV facility not installed\n",
> +				     AP_QID_CARD(apqn), AP_QID_QUEUE(apqn));
> +		ret = -EOPNOTSUPP;
>  		goto out_unlock;
>  	}

[ ... ]

> @@ -835,7 +895,8 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
>  	memcpy(&vcpu->run->s.regs.gprs[1], &qstatus, sizeof(qstatus));
>  	vcpu->run->s.regs.gprs[1] >>= 32;
>  	mutex_unlock(&matrix_dev->mdevs_lock);
> -	return 0;
> +	mutex_unlock(&matrix_dev->guests_lock);
> +	return ret;
>  }

[Severity: High]
Does routing the -EOPNOTSUPP fallback through out_unlock clobber guest
registers and break emulation?

If the KVM AIV facility is not installed, handle_pqap() returns -EOPNOTSUPP
to defer emulation to userspace.

However, since this now passes through out_unlock, it unconditionally
executes memcpy(&vcpu->run->s.regs.gprs[1], &qstatus, sizeof(qstatus)).

Will this overwrite the guest's actual instruction parameters in gprs[1]
with an error code before KVM exits to userspace, causing QEMU to attempt
emulation with corrupted inputs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929121837.2715710-1-akrowiak@linux.ibm.com?part=6