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

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

[Alexandra Winter]

A ism interrupt handler can be active in parallel with ism_dev_exit(),
accessing freed data structures.

No new interrupts will be generated after unregister_ieq(). Drain ongoing
interrupt handlers by free_irq(), before freeing ism data structures.

Fixes: 684b89bc39ce ("s390/ism: add device driver for internal shared memory")
Signed-off-by: Alexandra Winter <wintera@linux.ibm.com>
---
 drivers/s390/net/ism_drv.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/s390/net/ism_drv.c b/drivers/s390/net/ism_drv.c
index d99c588d3e00..242da20f27e0 100644
--- 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;
 }
 
 static int unregister_ieq(struct ism_dev *ism)
@@ -168,13 +171,16 @@ static int unregister_ieq(struct ism_dev *ism)
 	if (ret && ret != ISM_ERROR)
 		return -EIO;
 
+	return 0;
+}
+
+static void ism_free_ieq(struct ism_dev *ism)
+{
 	dma_free_coherent(&ism->pdev->dev, PAGE_SIZE,
 			  ism->ieq, ism->ieq_dma_addr);
 
 	ism->ieq = NULL;
 	ism->ieq_dma_addr = 0;
-
-	return 0;
 }
 
 static int ism_read_local_gid(struct dibs_dev *dibs)
@@ -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);
 free_vectors:
@@ -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);
+	/* 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);
 }
 
-- 
2.53.0

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

[sashiko-bot]

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

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

[Alexandra Winter]

On 06.08.26 15:11, sashiko-bot@kernel.org wrote:
> 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?
> 


Cases where a bad return code from HW/FW can happen:
- Device is configured off or unplugged -> ISM_ERROR -> ok to free
- Still DMBs registered -> does not happen with today's kernel code, afaict
  I plan to make this more robust for future dibs clients.
- Broken FW

=> We cannot create a scenario today where -EIO will be returned.



>> +	/* 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?


No interrupts are generated before SBA AND IEQ are successfully registered.
So this scenario is not possible for this error path.


> 
> 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?
> 

No interrupts are generated before SBA AND IEQ are successfully registered.

I will add comments in a cleanup patch.

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed,  5 Aug 2026 15:10:43 +0200 you wrote:
> A ism interrupt handler can be active in parallel with ism_dev_exit(),
> accessing freed data structures.
> 
> No new interrupts will be generated after unregister_ieq(). Drain ongoing
> interrupt handlers by free_irq(), before freeing ism data structures.
> 
> Fixes: 684b89bc39ce ("s390/ism: add device driver for internal shared memory")
> Signed-off-by: Alexandra Winter <wintera@linux.ibm.com>
> 
> [...]

Here is the summary with links:
  - [net] s390/ism: Fix UAF of sba and ieq during ism_dev_exit()
    https://git.kernel.org/netdev/net/c/b1896543ce59

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html