[PATCH net] net: wwan: iosm: fix NULL pointer dereference when removing device

[PATCH net] net: wwan: iosm: fix NULL pointer dereference when removing device

[m.chetan.kumar]

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>
---
 drivers/net/wwan/iosm/iosm_ipc_imem.c     | 29 ++++++++++++++++++-----
 drivers/net/wwan/iosm/iosm_ipc_imem_ops.c | 12 ++++++----
 drivers/net/wwan/iosm/iosm_ipc_imem_ops.h |  6 +++--
 3 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wwan/iosm/iosm_ipc_imem.c b/drivers/net/wwan/iosm/iosm_ipc_imem.c
index c066b0040a3f..0b4efd2571dd 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_cp_phase;
 	}
 
 	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_cap_init;
+
+	ipc_imem->mux = ipc_mux_init(&mux_cfg, ipc_imem);
+	if (!ipc_imem->mux)
+		goto err_mux_init;
+
+	ret = ipc_imem_wwan_channel_init(ipc_imem, mux_cfg.protocol);
+	if (ret < 0)
+		goto err_channel_init;
 
-	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,15 @@ static void ipc_imem_run_state_worker(struct work_struct *instance)
 
 	/* Complete all memory stores after setting bit */
 	smp_mb__after_atomic();
+
+	return;
+
+err_channel_init:
+	ipc_mux_deinit(ipc_imem->mux);
+err_mux_init:
+err_cap_init:
+err_cp_phase:
+	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
-- 
2.34.1

Re: [PATCH net] net: wwan: iosm: fix NULL pointer dereference when removing device

[Simon Horman]

On Thu, May 11, 2023 at 03:34:44PM +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>

...

> @@ -622,6 +630,15 @@ static void ipc_imem_run_state_worker(struct work_struct *instance)
>  
>  	/* Complete all memory stores after setting bit */
>  	smp_mb__after_atomic();
> +
> +	return;
> +
> +err_channel_init:
> +	ipc_mux_deinit(ipc_imem->mux);
> +err_mux_init:
> +err_cap_init:
> +err_cp_phase:
> +	ipc_uevent_send(ipc_imem->dev, UEVENT_CD_READY_LINK_DOWN);

Hi Chentan,

Thanks for your patch.
a nit from my side.

I think it is preferable for labels to be named after what they do
rather than where they are called from.

Something like:

err_mux_deinit:
	ipc_mux_deinit(ipc_imem->mux);
err_out:
	ipc_uevent_send(ipc_imem->dev, UEVENT_CD_READY_LINK_DOWN);

I also note that if ipc_mux_deinit() checked for a NULL (and did nothing),
then this could become:

err_out:
	ipc_mux_deinit(ipc_imem->mux);
	ipc_uevent_send(ipc_imem->dev, UEVENT_CD_READY_LINK_DOWN);

>  }
>  

...

Re: [PATCH net] net: wwan: iosm: fix NULL pointer dereference when removing device

[Kumar, M Chetan]

On 5/12/2023 10:35 PM, Simon Horman wrote:
> On Thu, May 11, 2023 at 03:34:44PM +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>
> 

<skip>

> err_out:
> 	ipc_mux_deinit(ipc_imem->mux);
> 	ipc_uevent_send(ipc_imem->dev, UEVENT_CD_READY_LINK_DOWN);
> 
>>   }

Thank you for reviewing the patch and sharing the feedback.
I will post the v2 with the above suggested changes.