[PATCH net] net: wwan: iosm: fix potential memory leaks in ipc_imem_init()

[PATCH net] net: wwan: iosm: fix potential memory leaks in ipc_imem_init()

[Abdun Nihaal]

The memory allocated in ipc_protocol_init() is not freed on the error
paths that follow in ipc_imem_init(). Fix that by calling the
corresponding release function ipc_protocol_deinit() in the error path.

Fixes: 3670970dd8c6 ("net: iosm: shared memory IPC interface")
Cc: stable@vger.kernel.org
Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>
---
Compile tested only. Issue found using static analysis.

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

diff --git a/drivers/net/wwan/iosm/iosm_ipc_imem.c b/drivers/net/wwan/iosm/iosm_ipc_imem.c
index 1b7bc7d63a2e..f4edb277efd9 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_imem.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_imem.c
@@ -1422,6 +1422,7 @@ struct iosm_imem *ipc_imem_init(struct iosm_pcie *pcie, unsigned int device_id,
 	hrtimer_cancel(&ipc_imem->fast_update_timer);
 	hrtimer_cancel(&ipc_imem->tdupdate_timer);
 	hrtimer_cancel(&ipc_imem->startup_timer);
+	ipc_protocol_deinit(ipc_imem->ipc_protocol);
 protocol_init_fail:
 	cancel_work_sync(&ipc_imem->run_state_worker);
 	ipc_task_deinit(ipc_imem->ipc_task);
-- 
2.43.0

Re: [PATCH net] net: wwan: iosm: fix potential memory leaks in ipc_imem_init()

[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 memory leaks in ipc_imem_init()

The memory allocated in ipc_protocol_init() is not freed on the error
paths that follow in ipc_imem_init(). Fix that by calling the
corresponding release function ipc_protocol_deinit() in the error path.

> diff --git a/drivers/net/wwan/iosm/iosm_ipc_imem.c b/drivers/net/wwan/iosm/iosm_ipc_imem.c
> index 1b7bc7d63a2e8..f4edb277efd92 100644
> --- a/drivers/net/wwan/iosm/iosm_ipc_imem.c
> +++ b/drivers/net/wwan/iosm/iosm_ipc_imem.c
> @@ -1422,6 +1422,7 @@ struct iosm_imem *ipc_imem_init(struct iosm_pcie *pcie, unsigned int device_id,
>  	hrtimer_cancel(&ipc_imem->fast_update_timer);
>  	hrtimer_cancel(&ipc_imem->tdupdate_timer);
>  	hrtimer_cancel(&ipc_imem->startup_timer);
> +	ipc_protocol_deinit(ipc_imem->ipc_protocol);
>  protocol_init_fail:
>  	cancel_work_sync(&ipc_imem->run_state_worker);
>  	ipc_task_deinit(ipc_imem->ipc_task);

Could this introduce a use-after-free by destroying the protocol before
canceling the work and tasklet?

If an error occurs after the timers are started (for example, if devlink
registration fails), the hrtimer cancellation stops new timer events.
However, if the startup timer has already fired, it might have queued a
task into the ipc_task queue.

Calling ipc_protocol_deinit() here frees the ipc_protocol structure.
Since ipc_task_deinit() has not yet been called to flush the queue and
kill the tasklet, any pending tasklet may still execute.

If a queued task such as ipc_imem_tq_startup_timer_cb() runs, does it
access the freed ipc_protocol?

ipc_imem_tq_startup_timer_cb()
  ipc_imem_phase_update()
    ipc_imem_get_exec_stage_buffered()
      ipc_protocol_get_ap_exec_stage(ipc_imem->ipc_protocol)

Would it be safer to place the ipc_protocol_deinit() call after the
tasklet and worker are fully destroyed?
-- 
pw-bot: cr