[PATCH net] net: wwan: iosm: fix potential use after free in ipc_imem_cleanup()

[PATCH net] net: wwan: iosm: fix potential use after free in ipc_imem_cleanup()

[Abdun Nihaal]

During cleanup, the ipc_protocol_deinit() is called before the tasklets
are cleaned up. The tasklets may concurrently access the memory
allocated for ipc_protocol and so it could result in a use-after-free.
Fix that by moving ipc_protocol_deinit() after ipc_task_deinit().

Fixes: 3670970dd8c6 ("net: iosm: shared memory IPC interface")
Cc: stable@vger.kernel.org
Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>
---
Compile tested only. Not tested on hardware.

 drivers/net/wwan/iosm/iosm_ipc_imem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wwan/iosm/iosm_ipc_imem.c b/drivers/net/wwan/iosm/iosm_ipc_imem.c
index 4405c8531888..939364daf5c7 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_imem.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_imem.c
@@ -1256,8 +1256,8 @@ void ipc_imem_cleanup(struct iosm_imem *ipc_imem)
 	ipc_imem_device_ipc_uninit(ipc_imem);
 	ipc_imem_channel_reset(ipc_imem);
 
-	ipc_protocol_deinit(ipc_imem->ipc_protocol);
 	ipc_task_deinit(ipc_imem->ipc_task);
+	ipc_protocol_deinit(ipc_imem->ipc_protocol);
 
 	kfree(ipc_imem->ipc_task);
 	kfree(ipc_imem->mmio);
-- 
2.43.0

Re: [PATCH net] net: wwan: iosm: fix potential use after free in ipc_imem_cleanup()

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: wwan: iosm: fix potential use after free in ipc_imem_cleanup()

This commit reorders ipc_imem_cleanup() so that ipc_task_deinit() runs
before ipc_protocol_deinit(), so a tasklet observed mid-flight does not
dereference protocol memory that has already been freed.

> diff --git a/drivers/net/wwan/iosm/iosm_ipc_imem.c b/drivers/net/wwan/iosm/iosm_ipc_imem.c
> index 1b7bc7d63a2e..a3353af6960c 100644
> --- a/drivers/net/wwan/iosm/iosm_ipc_imem.c
> +++ b/drivers/net/wwan/iosm/iosm_ipc_imem.c
> @@ -1256,8 +1256,8 @@ void ipc_imem_cleanup(struct iosm_imem *ipc_imem)
>  	ipc_imem_device_ipc_uninit(ipc_imem);
>  	ipc_imem_channel_reset(ipc_imem);
>  
> -	ipc_protocol_deinit(ipc_imem->ipc_protocol);
>  	ipc_task_deinit(ipc_imem->ipc_task);
> +	ipc_protocol_deinit(ipc_imem->ipc_protocol);

[High]
Does this reorder fully close the cleanup race, or only the narrow window
between the tasklet and ipc_protocol?

The MSI handler ipc_msi_interrupt() remains registered throughout
ipc_imem_cleanup(); ipc_release_irq() / free_irq() is only invoked later,
from ipc_pcie_resources_release() after ipc_imem_cleanup() has returned.

Once ipc_task_deinit() returns, ipc_imem->ipc_task->ipc_tasklet has been
kfree()'d:

void ipc_task_deinit(struct ipc_task *ipc_task)
{
	tasklet_kill(ipc_task->ipc_tasklet);

	kfree(ipc_task->ipc_tasklet);
	...
}

If the modem fires an MSI in the window between ipc_task_deinit() and
ipc_release_irq(), can the path

  ipc_msi_interrupt()
    -> ipc_imem_irq_process()
      -> ipc_task_queue_send_task()
        -> ipc_task_queue_add_task()

still dereference ipc_imem->ipc_task->ipc_tasklet (now freed) and call
tasklet_schedule() on it? Once kfree(ipc_imem->ipc_task) executes, the
q_lock and queue array are also freed.

The only attempt to quiesce the IRQ producer before this point is
ipc_imem_device_ipc_uninit(), which is best-effort: it is skipped when
ipc_pcie_check_data_link_active() returns false, has roughly a 30 ms
timeout, and does not call synchronize_irq().

This race appears to predate the patch (the freed-tasklet use-after-free
after ipc_task_deinit() was already reachable from the live IRQ handler),
so it is not introduced by the reorder. Would it be worth calling
ipc_release_irq() (or free_irq() / synchronize_irq()) before
ipc_task_deinit() / ipc_protocol_deinit() so that no MSI can schedule new
work against freed resources?

>  
>  	kfree(ipc_imem->ipc_task);
>  	kfree(ipc_imem->mmio);
-- 
pw-bot: cr