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

On 8/24/26 10:11 AM, [email protected] wrote:
> 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?

This is an interesting question. I took a look at the AP bus to see how this
response code is handled; it returns 0 from the function that processes the
PQAP(AQIC) call to enable interrupts, treating it as a successful 
completion.
This has generated several questions for which I'm seeking answers. 
Depending
on those answers, a new patch will be posted should this require a fix.

>
> 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.

Looking at the existing code, all other guest-triggered warning paths in
this function already use VFIO_AP_DBF_WARN() — the default branch is the
odd one out. The fix is to drop the pr_warn entirely and use
VFIO_AP_DBF_WARN() which routes to the s390 debug facility (ring buffer)
and is not exposed to dmesg flooding. Since it wasn't introduced by this
patch, it will come with a separate series along with the fix to the comment
below which is for a similar problem.

>
>> -		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?

With panic_on_warn=1 this is a guest-triggerable host panic — a real DoS.
The fix is to replace the WARN_ONCE with VFIO_AP_DBF_WARN like that which
is used in the vfio_ap_irq_enable() function. The fix will be posted in 
a separate
series along with the fix mentioned above since neither was introduced via
this patch and are related.

>
>> +		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;
>>   	}