Re: [PATCH 1/4] s390/vfio-ap: Fix leak of pinned NIB and registered NISC in vfio_ap_irq_enable()

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] `vfio_ap_irq_enable` incorrectly treats `AP_RESPONSE_OTHERWISE_CHANGED` (0x07) as a failure, unpinning the NIB while the hardware has successfully configured it. This allows the AP hardware to DMA into freed host memory.
- [High] Unprivileged guest can flood the host dmesg by repeatedly triggering a `pr_warn()` in the `default` case of the `ap_aqic()` response code switch.
- [High] A malicious or misbehaving guest can trigger host-side WARN_ONCE macros by disabling IRQs on a checkstopped or busy AP queue, leading to kernel panics if panic_on_warn is enabled.
--

--- Patch [1]: [PATCH 1/4] s390/vfio-ap: Fix leak of pinned NIB and registered NISC in vfio_ap_irq_enable() ---
commit c278a02f835924e412b00c7f6b715a44741cbcce
Author: Anthony Krowiak <[email protected]>

s390/vfio-ap: Fix leak of pinned NIB and registered NISC in vfio_ap_irq_enable()

This commit addresses a resource leak of pinned NIB and registered NISC
pages during AP queue interrupt enablement. It updates the error handling
path to unpin and unregister resources directly when an unexpected status
is returned. This avoids incorrectly invoking the interrupt disable routine
on a failing queue.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 940c0ff668bee..a46bf381ab726 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -503,9 +503,14 @@ static struct ap_queue_status vfio_ap_irq_enable(struct vfio_ap_queue *q,
>  		vfio_unpin_pages(&q->matrix_mdev->vdev, nib, 1);
>  		break;

[Severity: Critical]
This is a pre-existing issue, but does vfio_ap_irq_enable() incorrectly
treat AP_RESPONSE_OTHERWISE_CHANGED (0x07) as a failure?

When a guest triggers PQAP(AQIC) to enable interrupts on an AP queue that
is not empty, the hardware returns AP_RESPONSE_OTHERWISE_CHANGED. This
indicates successful IRQ modification but a non-empty queue.

Because this code treats it as an error and unpins the NIB pages, couldn't
the AP hardware later DMA interrupt data into the physical address of the
unpinned NIB, corrupting host memory if those pages are reallocated?

>  	default:
> -		pr_warn("%s: apqn %04x: response: %02x\n", __func__, q->apqn,
> -			status.response_code);

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to leave this
pr_warn() un-ratelimited in the default case?

If an unprivileged KVM guest executes PQAP(AQIC) in a tight loop on a
queue that is in a state returning an unhandled response code (e.g.,
AP_RESPONSE_Q_NOT_AVAIL), couldn't they flood the host dmesg?

This violates KVM subsystem rules which mandate that guest-reachable
warnings must be rate-limited or removed to prevent log flood Denial of
Service.

> -		vfio_ap_irq_disable(q);

[Severity: High]
This patch correctly removes the call to vfio_ap_irq_disable() here, but
this highlights a pre-existing issue: is it possible for a malicious guest
to trigger the WARN_ONCE macros inside vfio_ap_irq_disable() directly?

If the guest executes PQAP(AQIC) with the interrupt enable bit set to 0
(disable) on a queue that returns AP_RESPONSE_DECONFIGURED or
AP_RESPONSE_CHECKSTOPPED, it will directly invoke vfio_ap_irq_disable():

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_irq_disable() {
    [ ... ]
	case AP_RESPONSE_Q_NOT_AVAIL:
	case AP_RESPONSE_DECONFIGURED:
	case AP_RESPONSE_CHECKSTOPPED:
	case AP_RESPONSE_INVALID_ADDRESS:
	default:
		/* All cases in default means AP not operational */
		WARN_ONCE(1, "%s: ap_aqic status %d\n", __func__,
			  status.response_code);
    [ ... ]
}

Since this path is fully reachable by an unprivileged guest, couldn't this
lead to a host kernel panic if panic_on_warn is enabled?

> +		pr_warn("%s: PQAP(AQIC) failed with response code %02x for apqn %04x\n",
> +			__func__, status.response_code, q->apqn);
> +		/* We could not modify IRQ settings: clear new configuration */
> +		ret = kvm_s390_gisc_unregister(kvm, isc);
> +		if (ret)
> +			VFIO_AP_DBF_WARN("%s: kvm_s390_gisc_unregister: rc=%d isc=%d, apqn=%#04x\n",
> +					 __func__, ret, isc, q->apqn);
> +		vfio_unpin_pages(&q->matrix_mdev->vdev, nib, 1);
>  		break;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1