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

On 06.08.26 15:11, [email protected] 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 <[email protected]>
> 
> 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.