Re: [PATCH 5/7] s390/pci: add NULL check in zpci_msi_clear_airq()

["Tobias Schumacher" <ts@linux.ibm.com>]

On Wed Aug 19, 2026 at 11:14 AM CEST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>
> New issues:
> - [Medium] The patch introduces dead code by adding NULL checks for impossible scenarios due to a misunderstanding of the MSI irqdomain lifecycle.
>
> Pre-existing issues:
> - [Critical] Double addition of the loop index `i` in `zpci_msi_clear_airq()` causes a heap out-of-bounds write during multi-MSI teardown.
> - [High] Missing device teardown in `pci_base_init()` error path creates a Use-After-Free condition, which the added NULL checks silently evaluate.
> --
>
> --- Patch [5]: [PATCH 5/7] s390/pci: add NULL check in zpci_msi_clear_airq() ---
> commit a9e3adc4d04b20107c5f8c48da92688566d873e6
> Author: Tobias Schumacher <ts@linux.ibm.com>
> s390/pci: add NULL check in zpci_msi_clear_airq()
>
> This commit adds null checks for the interrupt vector pointers in
> zpci_msi_clear_airq() for both DIRECTED and FLOATING modes. It attempts
> to prevent crashes during error cleanup and shutdown paths if the pointers
> are already freed.
>
>> In FLOATING mode, zdev->aibv can be NULL if:
>> - zpci_msi_prepare() fails after __alloc_airq() but before setting
>>   up the device's AIBV, and zpci_msi_domain_free() is called during
>>   error cleanup
>> - The device is being torn down and zpci_msi_teardown_floating()
>>   has already released the AIBV
>
> [Severity: Medium]
> Is this description accurate?
>
> If the .msi_prepare callback fails, it returns an error and the MSI core
> halts allocation without invoking .alloc. Consequently, the .free callback
> is never invoked for this device, so zpci_msi_clear_airq() wouldn't
> execute on this error path.
>
> During normal device teardown, the MSI core guarantees that .free is invoked
> per-IRQ strictly before the domain is destroyed and .msi_teardown is invoked.
> Since zpci_msi_teardown_floating() is the only function that sets zdev->aibv
> to null, zdev->aibv should be valid whenever zpci_msi_clear_airq() executes.

Right, will fix this.

>> diff --git a/arch/s390/pci/pci_irq.c b/arch/s390/pci/pci_irq.c
>> index e9eda846cb2d2..1515d8d7460ea 100644
>> --- a/arch/s390/pci/pci_irq.c
>> +++ b/arch/s390/pci/pci_irq.c
>> @@ -465,12 +465,16 @@ static void zpci_msi_clear_airq(struct irq_data *d, int i)
>>
>>  	if (irq_delivery == DIRECTED) {
>>  		for_each_possible_cpu(cpu) {
>> -			airq_iv_set_ptr(zpci_ibv[cpu], bit + i, 0);
>> -			airq_iv_set_data(zpci_ibv[cpu], bit + i, 0);
>> +			if (zpci_ibv[cpu]) {
>
> [Severity: High]
> This is a pre-existing issue, but does checking zpci_ibv[cpu] here lead to a
> use-after-free read?
>
> In pci_base_init(), if zpci_fw_sysfs_init() fails, it jumps to out_find and
> calls zpci_irq_exit(). This frees the zpci_ibv array via kfree(), but leaves
> active PCI devices registered.
>
> When drivers later bind to these devices and tear them down,
> zpci_msi_clear_airq() will execute. Checking the freed zpci_ibv array pointer
> will likely evaluate to true, causing a use-after-free read and subsequent
> write when it is dereferenced.

Right, will fix that.

>> +				airq_iv_set_ptr(zpci_ibv[cpu], bit + i, 0);
>> +				airq_iv_set_data(zpci_ibv[cpu], bit + i, 0);
>
> [Severity: Critical]
> This is a pre-existing issue, but does this code write out of bounds during a
> multi-MSI teardown?

Discussed before, correct and will be fixed in follow-up series.