Re: [PATCH v2 5/7] s390/pci: Add error cleanup in zpci_directed_irq_init

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] s390/pci: hardware use-after-free via dangling DMA pointer on init failure
- [Low] mm: unnecessary complex error handling for __init memory allocation
--

commit 3f6f1514864f283bcc9b6097b32ff020d51e4124
Author: Tobias Schumacher <ts@linux.ibm.com>

s390/pci: Add error cleanup in zpci_directed_irq_init

If per-CPU airq_iv allocation fails in the loop, previously allocated
vectors and arrays leak. Add proper error path to release all resources
on failure.

> diff --git a/arch/s390/pci/pci_irq.c b/arch/s390/pci/pci_irq.c
> index 81a27bf756a3a..682bceb6525e2 100644
> --- a/arch/s390/pci/pci_irq.c
> +++ b/arch/s390/pci/pci_irq.c

[ ... ]

> @@ -576,7 +576,7 @@ static int __init zpci_directed_irq_init(void)
>  
>  	zpci_dibv = kzalloc_objs(*zpci_dibv, num_possible_cpus());
>  	if (!zpci_dibv)
> -		return -ENOMEM;
> +		goto out_free_sbv;
>  
>  	for_each_possible_cpu(cpu) {
>  		/*
> @@ -589,13 +589,25 @@ static int __init zpci_directed_irq_init(void)
>  						AIRQ_IV_CACHELINE |
>  						(!cpu ? AIRQ_IV_ALLOC : 0), NULL);
>  		if (!zpci_dibv[cpu])
> -			return -ENOMEM;
> +			goto out_free_dibv;
>  	}
>  	on_each_cpu(cpu_enable_directed_irq, NULL, 1);
>  
>  	zpci_irq_chip.irq_set_affinity = zpci_set_irq_affinity;
>  
>  	return 0;
> +
> +out_free_dibv:
> +	for_each_possible_cpu(cpu) {
> +		if (zpci_dibv[cpu])
> +			airq_iv_release(zpci_dibv[cpu]);
> +	}
> +	kfree(zpci_dibv);
> +	zpci_dibv = NULL;

[Severity: Low]
Since zpci_directed_irq_init() is an __init function, is this complex error
handling for early boot memory allocation failures necessary?

Functions executed only early in the boot process generally do not need to
handle memory allocation failures gracefully, as physical memory should be
available and a failure typically means the system cannot boot anyway. This
could be unnecessary dead code.

> +out_free_sbv:
> +	airq_iv_release(zpci_sbv);
> +	zpci_sbv = NULL;
> +	return -ENOMEM;

[Severity: High]
Does this error path leave a dangling physical memory address programmed into
the hardware?

Earlier in zpci_directed_irq_init(), the hardware is programmed to use the
physical address of zpci_sbv->vector:

	iib.diib.disb_addr = virt_to_phys(zpci_sbv->vector);
	zpci_set_irq_ctrl(SIC_IRQ_MODE_DIRECT, 0, &iib);

If a subsequent allocation fails and we take this out_free_sbv path, zpci_sbv
is freed without unregistering the address from the hardware. Could a
subsequent spurious adapter interrupt cause the hardware to write to the freed
memory and corrupt it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005-s390_irq_domain_fixes-v2-0-d45b824874c0@linux.ibm.com?part=5