Re: [PATCH v7 1/6] s390/vfio-ap: Fix leaks of pinned NIB and registered GISC

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

On 9/4/26 6:51 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Critical] Eagerly freeing AQIC resources when ap_zapq() or ap_tapq() returns an unrecognized response allows wild DMA writes to freed host memory.
> --
>
> commit 6c73d9ea4d311d5d68e8f5501374842cccbe2c90
> Author: Anthony Krowiak <akrowiak@linux.ibm.com>
>
> s390/vfio-ap: Fix leaks of pinned NIB and registered GISC
>
> Several code paths in the vfio_ap driver failed to free the AQIC
> resources — the pinned guest NIB page and the registered guest ISC
> used to enable interrupts for a queue — when a queue became unavailable
> or when unexpected response codes were returned. This could cause memory
> exhaustion and depletion of KVM interrupt subclass registrations over time
> with repeated dynamic AP reconfiguration.
>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 940c0ff668bee..9d4f5b9e3301f 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> [ ... ]
>> @@ -1959,8 +2087,16 @@ static void apq_reset_check(struct work_struct *reset_work)
>>   		elapsed += AP_RESET_INTERVAL;
>>   		status = ap_tapq(q->apqn, NULL);
>>   		ret = apq_status_check(q->apqn, &status);
>> -		if (ret == -EIO)
>> +		if (ret == -EIO) {
>> +			/*
>> +			 * TAPQ returned an invalid response code. This
>> +			 * indicates a hardware or firmware bug; the queue
>> +			 * cannot generate AP interrupts or DMA-write to the
>> +			 * NIB, so free the AQIC resources rather than leak them.
>> +			 */
>> +			vfio_ap_free_aqic_resources(q);
>>   			return;
>> +		}
> [Severity: Critical]
> If apq_status_check() returns -EIO due to an unrecognized TAPQ response,
> is it safe to eagerly free the AQIC resources?
>
> If the hardware is in an unknown state (due to a bug or unrecognized code),
> could it still be actively DMA-writing to the pinned Notification Indicator
> Byte (NIB) page?
>
> If vfio_ap_free_aqic_resources() unpins the page and returns it to the
> system allocator, could a subsequent wild DMA write from the malfunctioning
> AP queue corrupt memory belonging to a new owner, potentially causing a host
> panic?

Since we don't know the state of the queue, the AQIC resources should be
intentionally leaked here lest the NIB page be re-allocated to another
process potentially resulting in a wild DMA-write.

>
> [ ... ]
>> @@ -1995,22 +2135,30 @@ static void vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q)
>>   	switch (status.response_code) {
>>   	case AP_RESPONSE_NORMAL:
>>   	case AP_RESPONSE_RESET_IN_PROGRESS:
>> -	case AP_RESPONSE_BUSY:
>>   	case AP_RESPONSE_STATE_CHANGE_IN_PROGRESS:
>>   		/*
>>   		 * Let's verify whether the ZAPQ completed successfully on a work queue.
>>   		 */
>>   		queue_work(system_long_wq, &q->reset_work);
>>   		break;
>> +	case AP_RESPONSE_Q_NOT_AVAIL:
>>   	case AP_RESPONSE_DECONFIGURED:
>>   	case AP_RESPONSE_CHECKSTOPPED:
>>   		vfio_ap_free_aqic_resources(q);
>>   		break;
>>   	default:
>> +		/*
>> +		 * The architecture defines only the response codes above as
>> +		 * valid for ZAPQ. Any other response code indicates a hardware
>> +		 * or firmware bug. Since a malfunctioning queue cannot generate
>> +		 * AP interrupts or DMA-write to the NIB, free the AQIC resources
>> +		 * rather than leak them.
>> +		 */
>>   		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);
>> +		vfio_ap_free_aqic_resources(q);
>>   	}
>>   }
> [Severity: Critical]
> Similarly, if ZAPQ fails with an unrecognized response code in
> vfio_ap_mdev_reset_queue(), does freeing the AQIC resources here present the
> same wild DMA risk?
>
> Even if a later patch in this series adds a retry loop that explicitly
> handles AP_RESPONSE_BUSY, does this default case still expose the host to
> memory corruption if any other unexpected response code is returned?

Same as the above response

>