Re: [PATCH 2/7] s390/pci: fix use-after-free race in zpci floating interrupt cleanup

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Teardown order creates a race condition that can overwrite a concurrently allocated IRQ vector pointer with NULL, leading to permanently lost interrupts.
- [Medium] Missing RCU publication mechanism (`rcu_assign_pointer`) for `zpci_ibv` writes creates a potential data race where the IRQ handler could read uninitialized adapter interrupt vectors.
--

--- Patch [2]: [PATCH 2/7] s390/pci: fix use-after-free race in zpci floating interrupt cleanup ---
commit 2b99a8a1009855b33744de14f0b8b1c8afadae39
Author: Tobias Schumacher <[email protected]>

s390/pci: fix use-after-free race in zpci floating interrupt cleanup

The interrupt handler reads zpci_ibv[si] without synchronization while
concurrent teardown can release this memory, creating a race. The fix
protects the array access with RCU synchronization.

> diff --git a/arch/s390/pci/pci_irq.c b/arch/s390/pci/pci_irq.c
> index c9520a16ca753..94b03d16006ba 100644
> --- a/arch/s390/pci/pci_irq.c
> +++ b/arch/s390/pci/pci_irq.c
> @@ -278,8 +278,14 @@ static void zpci_floating_irq_handler(struct airq_struct *airq,
>  			continue;
>  		}
>  
> +		rcu_read_lock();
> +
>  		/* Scan the adapter interrupt vector for this device. */
> -		aibv = zpci_ibv[si];
> +		aibv = rcu_dereference(zpci_ibv[si]);

[Severity: Medium]
I see that rcu_dereference() is used here to read from the array.

Does the write side in __alloc_airq() also need to be updated to use
rcu_assign_pointer()?

arch/s390/pci/pci_irq.c:__alloc_airq() {
    ...
		/* Wire up shortcut pointer */
		zpci_ibv[*bit] = zdev->aibv;
    ...
}

Without rcu_assign_pointer(), the required memory release barrier is missing
when the interrupt vector is published to the RCU-protected array.

> +		if (!aibv) {
> +			rcu_read_unlock();
> +			continue;
> +		}

[ ... ]

> @@ -346,9 +353,12 @@ static void zpci_msi_teardown_directed(struct zpci_dev *zdev)
>  
>  static void zpci_msi_teardown_floating(struct zpci_dev *zdev)
>  {
> +	airq_iv_free_bit(zpci_sbv, zdev->aisb);
> +	zpci_ibv[zdev->aisb] = NULL;
> +	synchronize_rcu();

[Severity: High]
Does freeing the bit before clearing the array pointer create a race condition?

If Thread A (teardown) frees the bit, Thread B (initializing a new device)
could allocate the exact same bit from zpci_sbv. Thread B would then allocate
its vector and assign it to zpci_ibv[bit]. When Thread A continues, it clears
zpci_ibv[zdev->aisb], overwriting Thread B's valid pointer with NULL.

Could this overwrite lead to permanently lost interrupts and a memory leak for
the new device?

> +
>  	airq_iv_release(zdev->aibv);
>  	zdev->aibv = NULL;
> -	airq_iv_free_bit(zpci_sbv, zdev->aisb);
>  	zdev->aisb = -1UL;
>  	zdev->msi_first_bit = -1U;
>  	zdev->msi_nr_irqs = 0;

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