Re: [PATCH net] s390/ism: Fix UAF of sba and ieq during ism_dev_exit()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Critical] The patch unconditionally frees DMA memory even if the firmware command to unregister the structures fails, potentially causing the hardware to perform DMA into freed pages.

Pre-existing issues:
- [High] Incomplete fix: The `ism_dev_init()` error path frees the SBA data structure before unregistering the interrupt handler, preserving the Use-After-Free/NULL pointer dereference race condition.
- [High] The interrupt handler is registered before the DMA structures it depends on are fully initialized, leading to a possible NULL pointer dereference on early/spurious interrupts.
--

--- Patch [1]: [PATCH net] s390/ism: Fix UAF of sba and ieq during ism_dev_exit() ---
commit b7d428b60205e305c1f2fbb4f8ba1ff2da436f6f
Author: Alexandra Winter <wintera@linux.ibm.com>

s390/ism: Fix UAF of sba and ieq during ism_dev_exit()

This commit fixes a use-after-free race condition in the device teardown
path by draining ongoing interrupt handlers before freeing the internal
shared memory data structures.

> diff --git a/drivers/s390/net/ism_drv.c b/drivers/s390/net/ism_drv.c
> --- a/drivers/s390/net/ism_drv.c
> +++ b/drivers/s390/net/ism_drv.c
> @@ -148,13 +148,16 @@ static int unregister_sba(struct ism_dev *ism)
>  	if (ret && ret != ISM_ERROR)
>  		return -EIO;
>  
> +	return 0;
> +}
> +
> +static void ism_free_sba(struct ism_dev *ism)
> +{
>  	dma_free_coherent(&ism->pdev->dev, PAGE_SIZE,
>  			  ism->sba, ism->sba_dma_addr);
>  
>  	ism->sba = NULL;
>  	ism->sba_dma_addr = 0;
> -
> -	return 0;
>  }

[ ... ]

> @@ -585,9 +592,13 @@ static void ism_dev_exit(struct ism_dev *ism)
>  {
>  	struct pci_dev *pdev = ism->pdev;
>  
> +	/* ism will only generate new IRQs while ieq & sba are registered */
>  	unregister_ieq(ism);
>  	unregister_sba(ism);

[Severity: Critical]
If unregister_ieq() or unregister_sba() fails (returning -EIO) due to a
hardware or firmware error, the device may still be active and performing
DMA to the SBA or IEQ memory regions.

By separating the free operations and calling them unconditionally here,
could we be returning active DMA pages to the kernel allocator, leading to
memory corruption?

> +	/* drain ongoing irpt handlers */
>  	free_irq(pci_irq_vector(pdev, 0), ism);
> +	ism_free_ieq(ism);
> +	ism_free_sba(ism);
>  	pci_free_irq_vectors(pdev);
>  }

[ ... ]

> @@ -573,6 +579,7 @@ static int ism_dev_init(struct ism_dev *ism)
>  
>  unreg_sba:
>  	unregister_sba(ism);
> +	ism_free_sba(ism);
>  free_irq:
>  	free_irq(pci_irq_vector(pdev, 0), ism);

[Severity: High]
This is a pre-existing issue, but I noticed the error path here frees the
SBA data structure before calling free_irq().

If register_ieq() fails during initialization, could a concurrent interrupt
fire after ism_free_sba() but before free_irq(), causing ism_handle_irq()
to dereference a NULL ism->sba pointer?

Additionally, this is another pre-existing issue, but looking earlier in
ism_dev_init():

ism_dev_init() {
    ...
    ret = request_irq(pci_irq_vector(pdev, 0), ism_handle_irq, 0,
                      pci_name(pdev), ism);
    if (ret)
        goto free_vectors;

    ret = register_sba(ism);
    ...
}

[Severity: High]
Since request_irq() enables the interrupt handler before register_sba()
allocates and assigns the data structures, what happens if an early or
spurious MSI interrupt fires during this window?

Will ism_handle_irq() unconditionally dereference the uninitialized
ism->sba pointer upon entry?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805131043.954639-1-wintera@linux.ibm.com?part=1