Re: [PATCH 6/7] s390/pci: add error cleanup in zpci_directed_irq_init

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The newly added error cleanup path in `zpci_directed_irq_init` introduces a DMA use-after-free vulnerability.
--

--- Patch [6]: [PATCH 6/7] s390/pci: add error cleanup in zpci_directed_irq_init ---
commit 5a0360870b410d13391fe09d0a8840ffaa9cb9be
Author: Tobias Schumacher <[email protected]>

s390/pci: add error cleanup in zpci_directed_irq_init

This commit adds an error cleanup path to zpci_directed_irq_init to release
airq_iv resources if per-CPU allocation fails, preventing a memory leak.

> diff --git a/arch/s390/pci/pci_irq.c b/arch/s390/pci/pci_irq.c
> index 1515d8d7460ea..1ddf6b3625a26 100644
> --- a/arch/s390/pci/pci_irq.c
> +++ b/arch/s390/pci/pci_irq.c
> @@ -586,7 +586,7 @@ static int __init zpci_directed_irq_init(void)
>  
>  	zpci_ibv = kzalloc_objs(*zpci_ibv, num_possible_cpus());
>  	if (!zpci_ibv)
> -		return -ENOMEM;
> +		goto out_free_sbv;
>  
>  	for_each_possible_cpu(cpu) {

[ ... ]

> @@ -599,13 +599,25 @@ static int __init zpci_directed_irq_init(void)
>  					       AIRQ_IV_CACHELINE |
>  					       (!cpu ? AIRQ_IV_ALLOC : 0), NULL);
>  		if (!zpci_ibv[cpu])
> -			return -ENOMEM;
> +			goto out_free_ibv;
>  	}
>  	on_each_cpu(cpu_enable_directed_irq, NULL, 1);
>  
>  	zpci_irq_chip.irq_set_affinity = zpci_set_irq_affinity;
>  
>  	return 0;
> +
> +out_free_ibv:
> +	for_each_possible_cpu(cpu) {
> +		if (zpci_ibv[cpu])
> +			airq_iv_release(zpci_ibv[cpu]);
> +	}
> +	kfree(zpci_ibv);
> +	zpci_ibv = NULL;
> +out_free_sbv:
> +	airq_iv_release(zpci_sbv);

[Severity: High]
Does this new error path introduce a potential DMA use-after-free?

Just before the allocation of zpci_ibv, the hardware is configured with the
physical address of zpci_sbv->vector:

arch/s390/pci/pci_irq.c:zpci_directed_irq_init() {
    ...
    iib.diib.disb_addr = virt_to_phys(zpci_sbv->vector);
    zpci_set_irq_ctrl(SIC_IRQ_MODE_DIRECT, 0, &iib);

    zpci_ibv = kzalloc_objs(*zpci_ibv, num_possible_cpus());
    if (!zpci_ibv)
        goto out_free_sbv;
    ...
}

If we jump to out_free_sbv and free zpci_sbv via airq_iv_release(zpci_sbv)
without resetting the hardware configuration first, could the adapter
corrupt memory by writing to the freed physical page during a spurious
interrupt?

Would it make sense to either defer the zpci_set_irq_ctrl() hardware
configuration until all software allocations succeed, or unregister the
hardware in the out_free_sbv error path?

> +	zpci_sbv = NULL;
> +	return -ENOMEM;
>  }

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