| Message ID | 6d9e4d90ec89d8ac026e149381ca0c8243a11a19.1684250558.git.m.chetan.kumar@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 60829145f1e2650b31ebe6a0ec70a9725b38fa2c |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v3,net] net: wwan: iosm: fix NULL pointer dereference when removing device | expand |
On Tue, May 16, 2023 at 09:09:46PM +0530, m.chetan.kumar@linux.intel.com wrote: > From: M Chetan Kumar <m.chetan.kumar@linux.intel.com> > > In suspend and resume cycle, the removal and rescan of device ends > up in NULL pointer dereference. > > During driver initialization, if the ipc_imem_wwan_channel_init() > fails to get the valid device capabilities it returns an error and > further no resource (wwan struct) will be allocated. Now in this > situation if driver removal procedure is initiated it would result > in NULL pointer exception since unallocated wwan struct is dereferenced > inside ipc_wwan_deinit(). > > ipc_imem_run_state_worker() to handle the called functions return value > and to release the resource in failure case. It also reports the link > down event in failure cases. The user space application can handle this > event to do a device reset for restoring the device communication. > > Fixes: 3670970dd8c6 ("net: iosm: shared memory IPC interface") > Reported-by: Samuel Wein PhD <sam@samwein.com> > Closes: https://lore.kernel.org/netdev/20230427140819.1310f4bd@kernel.org/T/ > Signed-off-by: M Chetan Kumar <m.chetan.kumar@linux.intel.com> > -- > v2 -> v3: > * Fix review comments given by Simon Horman. > - use second label for ipc_mux_deinit() since mux will be uninitalized in > other failure cases. > v1 -> v2: > * Fix review comments given by Simon Horman. > - goto labes renamed to reflect after usage instead where they are > called from. > - ipc_mux_deinit() checks for initalization state so is safe to keep > under common err_out. Reviewed-by: Simon Horman <simon.horman@corigine.com>
Hello: This patch was applied to netdev/net.git (main) by David S. Miller <davem@davemloft.net>: On Tue, 16 May 2023 21:09:46 +0530 you wrote: > From: M Chetan Kumar <m.chetan.kumar@linux.intel.com> > > In suspend and resume cycle, the removal and rescan of device ends > up in NULL pointer dereference. > > During driver initialization, if the ipc_imem_wwan_channel_init() > fails to get the valid device capabilities it returns an error and > further no resource (wwan struct) will be allocated. Now in this > situation if driver removal procedure is initiated it would result > in NULL pointer exception since unallocated wwan struct is dereferenced > inside ipc_wwan_deinit(). > > [...] Here is the summary with links: - [v3,net] net: wwan: iosm: fix NULL pointer dereference when removing device https://git.kernel.org/netdev/net/c/60829145f1e2 You are awesome, thank you!
diff --git a/drivers/net/wwan/iosm/iosm_ipc_imem.c b/drivers/net/wwan/iosm/iosm_ipc_imem.c index c066b0040a3f..829515a601b3 100644 --- a/drivers/net/wwan/iosm/iosm_ipc_imem.c +++ b/drivers/net/wwan/iosm/iosm_ipc_imem.c @@ -565,24 +565,32 @@ static void ipc_imem_run_state_worker(struct work_struct *instance) struct ipc_mux_config mux_cfg; struct iosm_imem *ipc_imem; u8 ctrl_chl_idx = 0; + int ret; ipc_imem = container_of(instance, struct iosm_imem, run_state_worker); if (ipc_imem->phase != IPC_P_RUN) { dev_err(ipc_imem->dev, "Modem link down. Exit run state worker."); - return; + goto err_out; } if (test_and_clear_bit(IOSM_DEVLINK_INIT, &ipc_imem->flag)) ipc_devlink_deinit(ipc_imem->ipc_devlink); - if (!ipc_imem_setup_cp_mux_cap_init(ipc_imem, &mux_cfg)) - ipc_imem->mux = ipc_mux_init(&mux_cfg, ipc_imem); + ret = ipc_imem_setup_cp_mux_cap_init(ipc_imem, &mux_cfg); + if (ret < 0) + goto err_out; + + ipc_imem->mux = ipc_mux_init(&mux_cfg, ipc_imem); + if (!ipc_imem->mux) + goto err_out; + + ret = ipc_imem_wwan_channel_init(ipc_imem, mux_cfg.protocol); + if (ret < 0) + goto err_ipc_mux_deinit; - ipc_imem_wwan_channel_init(ipc_imem, mux_cfg.protocol); - if (ipc_imem->mux) - ipc_imem->mux->wwan = ipc_imem->wwan; + ipc_imem->mux->wwan = ipc_imem->wwan; while (ctrl_chl_idx < IPC_MEM_MAX_CHANNELS) { if (!ipc_chnl_cfg_get(&chnl_cfg_port, ctrl_chl_idx)) { @@ -622,6 +630,13 @@ static void ipc_imem_run_state_worker(struct work_struct *instance) /* Complete all memory stores after setting bit */ smp_mb__after_atomic(); + + return; + +err_ipc_mux_deinit: + ipc_mux_deinit(ipc_imem->mux); +err_out: + ipc_uevent_send(ipc_imem->dev, UEVENT_CD_READY_LINK_DOWN); } static void ipc_imem_handle_irq(struct iosm_imem *ipc_imem, int irq) diff --git a/drivers/net/wwan/iosm/iosm_ipc_imem_ops.c b/drivers/net/wwan/iosm/iosm_ipc_imem_ops.c index 66b90cc4c346..109cf8930488 100644 --- a/drivers/net/wwan/iosm/iosm_ipc_imem_ops.c +++ b/drivers/net/wwan/iosm/iosm_ipc_imem_ops.c @@ -77,8 +77,8 @@ int ipc_imem_sys_wwan_transmit(struct iosm_imem *ipc_imem, } /* Initialize wwan channel */ -void ipc_imem_wwan_channel_init(struct iosm_imem *ipc_imem, - enum ipc_mux_protocol mux_type) +int ipc_imem_wwan_channel_init(struct iosm_imem *ipc_imem, + enum ipc_mux_protocol mux_type) { struct ipc_chnl_cfg chnl_cfg = { 0 }; @@ -87,7 +87,7 @@ void ipc_imem_wwan_channel_init(struct iosm_imem *ipc_imem, /* If modem version is invalid (0xffffffff), do not initialize WWAN. */ if (ipc_imem->cp_version == -1) { dev_err(ipc_imem->dev, "invalid CP version"); - return; + return -EIO; } ipc_chnl_cfg_get(&chnl_cfg, ipc_imem->nr_of_channels); @@ -104,9 +104,13 @@ void ipc_imem_wwan_channel_init(struct iosm_imem *ipc_imem, /* WWAN registration. */ ipc_imem->wwan = ipc_wwan_init(ipc_imem, ipc_imem->dev); - if (!ipc_imem->wwan) + if (!ipc_imem->wwan) { dev_err(ipc_imem->dev, "failed to register the ipc_wwan interfaces"); + return -ENOMEM; + } + + return 0; } /* Map SKB to DMA for transfer */ diff --git a/drivers/net/wwan/iosm/iosm_ipc_imem_ops.h b/drivers/net/wwan/iosm/iosm_ipc_imem_ops.h index f8afb217d9e2..026c5bd0f999 100644 --- a/drivers/net/wwan/iosm/iosm_ipc_imem_ops.h +++ b/drivers/net/wwan/iosm/iosm_ipc_imem_ops.h @@ -91,9 +91,11 @@ int ipc_imem_sys_wwan_transmit(struct iosm_imem *ipc_imem, int if_id, * MUX. * @ipc_imem: Pointer to iosm_imem struct. * @mux_type: Type of mux protocol. + * + * Return: 0 on success and failure value on error */ -void ipc_imem_wwan_channel_init(struct iosm_imem *ipc_imem, - enum ipc_mux_protocol mux_type); +int ipc_imem_wwan_channel_init(struct iosm_imem *ipc_imem, + enum ipc_mux_protocol mux_type); /** * ipc_imem_sys_devlink_open - Open a Flash/CD Channel link to CP