Re: [PATCH v6 1/5] s390/vfio-ap: Fix leak of pinned NIB and registered GISC in vfio_ap_irq_enable/disable()

[Anthony Krowiak <akrowiak@linux.ibm.com>]

On 9/4/26 5:51 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [High] Guest-reachable WARN_ONCE macros violate KVM guidelines and can cause a host denial of service (DoS) under panic_on_warn.
>
> Pre-existing issues:
> - [High] Pinned NIB page and registered GISC are permanently leaked when a queue is removed from the host AP configuration or becomes physically unavailable.
> --
>
> commit ea9124e6c06c371c42de8146e0e3493413dc0829
> Author: Anthony Krowiak <akrowiak@linux.ibm.com>
> s390/vfio-ap: Fix leak of pinned NIB and registered GISC in vfio_ap_irq_enable/disable()
>
> This commit updates the interrupt enable and disable handlers for vfio_ap
> to correctly manage pinned NIB pages and GISC registrations. It introduces
> an asynchronous wait process for interrupt clearing to prevent wild DMA
> writes, leaving resources allocated if the hardware doesn't confirm the
> operation.
>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 940c0ff668bee..383ec9f5c8107 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> [ ... ]
>
>> @@ -257,12 +269,15 @@ static void vfio_ap_wait_for_irqclear(int apqn)
>>   		default:
>>   			WARN_ONCE(1, "%s: tapq rc %02x: %04x\n", __func__,
>>   				  status.response_code, apqn);
>> -			return;
>> +			return -ENODEV;
>>   		}
>>   	} while (--retry);
>>   
>> -	WARN_ONCE(1, "%s: tapq rc %02x: %04x could not clear IR bit\n",
>> -		  __func__, status.response_code, apqn);
>> +	WARN_ONCE(1, "%s: tapq rc %02x: timed out waiting for interrupts disabled for %02x.%04x\n",
>> +		  __func__, status.response_code,
>> +		  AP_QID_CARD(apqn), AP_QID_QUEUE(apqn));
>> +
>> +	return -ETIMEDOUT;
>>   }
> [Severity: High]
> Does this allow an unprivileged guest to trigger a host panic?
>
> According to KVM subsystem guidelines, guest-reachable WARN_ON or WARN_ONCE
> macros are prohibited because they can crash the host kernel on systems
> configured with panic_on_warn.
>
> If a guest forces a delay by issuing a ZAPQ reset (which can take up to
> 2000ms), and immediately follows it with a PQAP-AQIC disable instruction,
> this 100ms TAPQ polling loop (5 retries * 20ms) will time out.
>
> Could this be converted to pr_warn_once() to avoid host denial of service?

This problem has been reported and responded to for all six versions of
this patch as well as others in the series. I discovered while doing some
testing/tracing of manually injected error conditions and discovered that
this is a real possibility; so all of the code locations that are 
reachable from
a guest that log WARN messages need to be fixed by replacing them with
pr_warn_rate_limited() or dev_warn_rate_limited calls.

>
> [ ... ]
>
>> @@ -326,18 +372,47 @@ static struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q)
>>   		case AP_RESPONSE_Q_NOT_AVAIL:
>>   		case AP_RESPONSE_DECONFIGURED:
>>   		case AP_RESPONSE_CHECKSTOPPED:
>> +			/* AP not operational; no further interrupts possible */
>> +			WARN_ONCE(1, "%s: ap_aqic status %d\n", __func__,
>> +				  status.response_code);
>> +			goto end_free;
>>   		case AP_RESPONSE_INVALID_ADDRESS:
>>   		default:
>> -			/* All cases in default means AP not operational */
>> +			/*
>> +			 * The AQIC disable was rejected; IRQ is still enabled
>> +			 * and the hardware still holds the NIB address. Do not
>> +			 * free resources.
>> +			 */
>>   			WARN_ONCE(1, "%s: ap_aqic status %d\n", __func__,
>>   				  status.response_code);
>> -			goto end_free;
>> +			goto end_fail;
>>   		}
>>   	} while (retries--);
> [Severity: High]
> Does this share the same risk of triggering a host panic as the warning above?
>
> Similar to the timeout case, if the AP returns statuses like DECONFIGURED
> or CHECKSTOPPED, this guest-reachable path triggers a WARN_ONCE. Should
> these be converted to pr_warn_once() to prevent a potential host DoS?

See above

>
> [ ... ]
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does the vfio_ap driver leak
> the pinned NIB page and registered GISC when a queue is physically removed
> or becomes unavailable?
>
> Looking at vfio_ap_mdev_remove_queue(), if a queue with active interrupts is
> removed from the host AP configuration, the bit test fails and we skip
> calling vfio_ap_mdev_reset_queue(q):
>
> 	/*
> 	 * If the queue is not in the host's AP configuration, then resetting
> 	 * it will fail with response code 01, (APQN not valid); so, let's make
> 	 * sure it is in the host's config.
> 	 */
> 	if (test_bit_inv(apid, (unsigned long *)matrix_dev->info.apm) &&
> 	    test_bit_inv(apqi, (unsigned long *)matrix_dev->info.aqm)) {
> 		vfio_ap_mdev_reset_queue(q);
> 		flush_work(&q->reset_work);
> 	}
>
> done:
> 	if (matrix_mdev)
> 		vfio_ap_unlink_queue_fr_mdev(q);
>
> 	dev_set_drvdata(&apdev->device, NULL);
> 	kfree(q);
>
> Because the reset is bypassed, vfio_ap_free_aqic_resources(q) is never
> called, permanently leaking the page and KVM routing entries.
>
> Similarly, in vfio_ap_mdev_reset_queue():
>
> 	default:
> 		WARN(true,
> 		     "PQAP/ZAPQ for %02x.%04x failed with invalid rc=%u\n",
> 		     AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
> 		     status.response_code);
> 	}
>
> If ap_zapq() returns AP_RESPONSE_Q_NOT_AVAIL (0x01) because the queue is
> physically unavailable, the default case is hit. It logs a warning but fails
> to call vfio_ap_free_aqic_resources(q), leaking the resources.
>
> Could this lead to memory exhaustion and depletion of KVM ISCs over time if
> devices are dynamically reconfigured?

This analysis is correct, there is a resource leak in
vfio_ap_mdev_reset_queue() when ap_zapq() returns
AP_RESPONSE_Q_NOT_AVAIL (0x01). That response code falls through
to the default case which only fires a WARN without calling
vfio_ap_free_aqic_resources(q).

Regarding vfio_ap_mdev_remove_queue(): there are two distinct
invocation paths to consider:

Sysfs unbind: the queue is still in the host's AP
configuration, so the test_bit_inv guard passes and
vfio_ap_mdev_reset_queue() is called normally. The ZAPQ will
succeed and resources are freed through the normal reset path.

AP bus scan (queue physically removed): the queue is no
longer in the host's AP configuration, so the test_bit_inv
guard is false and the ZAPQ is intentionally skipped — issuing
it would just return rc=0x01 anyway. Since the hardware is gone
and can no longer DMA-write to the NIB, it is safe to call
vfio_ap_free_aqic_resources(q) directly without a ZAPQ.
This is the fix needed here.

Note that vfio_ap_free_aqic_resources() is safe to call
unconditionally in the AP bus scan path even when q->matrix_mdev
is NULL (i.e., the queue is bound to vfio_ap but not assigned to
any mdev). The WARN_ON guards inside the function will protect
against the unpin and unregister operations. Since interrupts can
only be enabled for a queue assigned to an mdev with an active KVM
guest, saved_iova and saved_isc will not be set in that case,
making the call a safe no-op.

The fix for vfio_ap_mdev_reset_queue() is to add
AP_RESPONSE_Q_NOT_AVAIL alongside the existing
AP_RESPONSE_DECONFIGURED and AP_RESPONSE_CHECKSTOPPED cases
so that resources are freed when the queue is not available.

>