[PATCH] net: wwan: iosm: fix potential skb leak in MUX DL decode

[PATCH] net: wwan: iosm: fix potential skb leak in MUX DL decode

[Fang Xieyan]

ipc_mux_dl_decode() owns the received skb and frees it after
decoding. However, the early return when skb->data is NULL
bypasses the skb cleanup.

Free the skb through ipc_pcie_kfree_skb() before returning
to ensure that both the DMA mapping and skb are released.

Fixes: 3670970dd8c6 ("net: iosm: shared memory IPC interface")
Reviewed-by: Guanglei Zhu <zhugl3@xiaopeng.com>
Signed-off-by: Fang Xieyan <fangxy@xiaopeng.com>
---
 drivers/net/wwan/iosm/iosm_ipc_mux_codec.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wwan/iosm/iosm_ipc_mux_codec.c b/drivers/net/wwan/iosm/iosm_ipc_mux_codec.c
index 0bbd41263cc2..61700688eac9 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_mux_codec.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_mux_codec.c
@@ -693,8 +693,10 @@ void ipc_mux_dl_decode(struct iosm_mux *ipc_mux, struct sk_buff *skb)
 {
 	u32 signature;
 
-	if (!skb->data)
+	if (!skb->data) {
+		ipc_pcie_kfree_skb(ipc_mux->pcie, skb);
 		return;
+	}
 
 	/* Decode the MUX header type. */
 	signature = le32_to_cpup((__le32 *)skb->data);
-- 
2.50.1

Re: [PATCH] net: wwan: iosm: fix potential skb leak in MUX DL decode

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH] net: wwan: iosm: fix potential skb leak in MUX DL decode

[Loic Poulain]

Hi Fang,

On Mon, Nov 2, 2026 at 9:19 AM Fang Xieyan <fangxy@xiaopeng.com> wrote:
>
> ipc_mux_dl_decode() owns the received skb and frees it after
> decoding. However, the early return when skb->data is NULL
> bypasses the skb cleanup.
>
> Free the skb through ipc_pcie_kfree_skb() before returning
> to ensure that both the DMA mapping and skb are released.
>
> Fixes: 3670970dd8c6 ("net: iosm: shared memory IPC interface")
> Reviewed-by: Guanglei Zhu <zhugl3@xiaopeng.com>
> Signed-off-by: Fang Xieyan <fangxy@xiaopeng.com>
> ---
>  drivers/net/wwan/iosm/iosm_ipc_mux_codec.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wwan/iosm/iosm_ipc_mux_codec.c b/drivers/net/wwan/iosm/iosm_ipc_mux_codec.c
> index 0bbd41263cc2..61700688eac9 100644
> --- a/drivers/net/wwan/iosm/iosm_ipc_mux_codec.c
> +++ b/drivers/net/wwan/iosm/iosm_ipc_mux_codec.c
> @@ -693,8 +693,10 @@ void ipc_mux_dl_decode(struct iosm_mux *ipc_mux, struct sk_buff *skb)
>  {
>         u32 signature;
>
> -       if (!skb->data)
> +       if (!skb->data) {

Can skb->data ever be NULL in the first place? This looks rather
unusual. If it can happen, please describe what leads to that
situation. Otherwise, the check should simply be removed.

> +               ipc_pcie_kfree_skb(ipc_mux->pcie, skb);
>                 return;
> +       }
>
>         /* Decode the MUX header type. */
>         signature = le32_to_cpup((__le32 *)skb->data);
> --
> 2.50.1
>

Re: [PATCH] net: wwan: iosm: fix potential skb leak in MUX DL decode

[Fang Xieyan]

Hi Loic,

You're right, skb->data cannot be NULL in this path. I traced both the
allocation side and the RX path that feeds ipc_mux_dl_decode(), and the
check can never trigger.

Allocation chain:

  ipc_pcie_alloc_skb()
    +-- ipc_pcie_alloc_local_skb()
          +-- __netdev_alloc_skb()

On success skb->data points into a valid linear buffer; skb_reserve()
only advances skb->data within that buffer and never clears the
pointer. Allocation or DMA mapping failure returns a NULL skb.

The RX path is:

  ipc_imem_dl_pipe_process()
    +-- ipc_protocol_dl_td_process()
    +-- ipc_imem_dl_skb_process()
          +-- if (!skb) return;
          +-- ipc_mux_dl_decode()

The skb is allocated and queued on the DL pipe by
ipc_protocol_dl_td_prepare(). It stores the skb in skbr_start[] only
after a successful allocation; on failure it returns without advancing
the head or publishing the descriptor.

On the RX path, ipc_protocol_dl_td_process() either returns a valid skb
or frees it and returns NULL. ipc_imem_dl_skb_process() checks for NULL
before calling ipc_mux_dl_decode().

The later skb_put() only updates skb->len and the tail; it does not
touch skb->data. DMA writes to the mapped data buffer, not to the skb
metadata.

I couldn't find any code in the IOSM driver that assigns skb->data to
NULL either.

So the check is unreachable: the skb that reaches ipc_mux_dl_decode()
always has a valid data pointer. I'll send a cleanup patch that removes
the redundant check instead.

Thanks.

Fang Xieyan

Re: [PATCH] net: wwan: iosm: fix potential skb leak in MUX DL decode

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 (1) · Medium: 1 (2) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message claims to fix a 'potential skb leak' in
  ipc_mux_dl_decode()…

Pre-existing issues:
- [High] ipc_mux_dl_acb_decode()
  (drivers/net/wwan/iosm/iosm_ipc_mux_codec.c:518-545) follows a chain…
- [Medium] ipc_mux_dl_decode() reads a 4-byte signature at line 702
  without checking skb->len >= 4.
- [Medium] On the MUX (IPC_CTYPE_WWAN) downlink path, the CPU parses the
  receive buffer and hands skb_clone()s of it to the network stack while…

--

net: wwan: iosm: fix potential skb leak in MUX DL decode

This patch adds an ipc_pcie_kfree_skb() call to the early return in
ipc_mux_dl_decode() that is taken when skb->data is NULL. The intent is to
release both the DMA mapping and the skb. It carries a Fixes: tag for
commit 3670970dd8c6.

> diff --git a/drivers/net/wwan/iosm/iosm_ipc_mux_codec.c b/drivers/net/wwan/iosm/iosm_ipc_mux_codec.c
> index 0bbd41263cc2f..61700688eac96 100644
> --- a/drivers/net/wwan/iosm/iosm_ipc_mux_codec.c
> +++ b/drivers/net/wwan/iosm/iosm_ipc_mux_codec.c
> @@ -693,8 +693,10 @@ void ipc_mux_dl_decode(struct iosm_mux *ipc_mux, struct sk_buff *skb)
>  {
>  	u32 signature;
>  
> -	if (!skb->data)
> +	if (!skb->data) {
> +		ipc_pcie_kfree_skb(ipc_mux->pcie, skb);
>  		return;
> +	}

[Severity: Medium]
Can skb->data actually be NULL here? The commit message says:

  However, the early return when skb->data is NULL
  bypasses the skb cleanup.

The only caller is ipc_imem_dl_skb_process(), which already returns early
when skb is NULL.

DL ring skbs come only from
ipc_protocol_dl_td_prepare()->ipc_pcie_alloc_skb()->
ipc_pcie_alloc_local_skb()->__netdev_alloc_skb(). When the allocation
succeeds, skb->data points at the head buffer. When it fails, NULL is
returned and caught before anything is put on the ring:

drivers/net/wwan/iosm/iosm_ipc_protocol_ops.c:ipc_protocol_dl_td_prepare() {
    ...
	skb = ipc_pcie_alloc_skb(ipc_protocol->pcie, pipe->buf_size, GFP_ATOMIC,
				 &mapping, DMA_FROM_DEVICE,
				 IPC_MEM_DL_ETH_OFFSET);
	if (!skb)
		return false;
    ...
}

ipc_pcie_alloc_skb() also passes skb->data to ipc_pcie_addr_map() and
frees the skb if the mapping fails. Nothing between there and the decoder
(skb_reserve(), skb_put()) can set skb->data to NULL.

If that is right, the new ipc_pcie_kfree_skb() call is dead code and no
reachable leak gets fixed. Should this be described as hardening and sent
without the Fixes: tag? Or should the impossible check simply be removed,
so stable does not pick it up as a leak fix?

Separately, ipc_imem_dl_skb_process() has two branches that drop the skb
without freeing it: the IPC_CTYPE_WWAN case when if_id is not
IPC_MEM_MUX_IP_CH_IF_ID, and the default case. Both also look unreachable
with the current single WWAN channel (if_id 0).

>  
>  	/* Decode the MUX header type. */
>  	signature = le32_to_cpup((__le32 *)skb->data);

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. Is
skb->len checked anywhere before the signature and the headers after it
are parsed?

skb->len comes from the device-written p_td->scs in
ipc_protocol_dl_td_process(). It is only bounded by pipe->buf_size, so it
can be 0.

None of the decoders compare skb->len with their header sizes:
ipc_mux_dl_cmd_decode() (struct mux_lite_cmdh), ipc_mux_dl_adgh_decode()
(struct mux_adgh) and ipc_mux_dl_acb_decode(). ipc_mux_dl_fcth_decode()
gets only skb->data, so it cannot check the length at all. With a short
transfer, these decoders read stale or uninitialized bytes that lie inside
the buf_size allocation but beyond the received data.

In ipc_mux_dl_adgh_decode(), the u32 subtraction can underflow when the
device-supplied length is smaller than packet_offset:

	adgh_len = le16_to_cpu(adgh->length);

	/* Pass the packet to the netif layer */
	rc = ipc_mux_net_receive(ipc_mux, if_id, wwan, packet_offset,
				 adgh->service_class, skb,
				 adgh_len - packet_offset);

ipc_mux_net_receive() ignores a failed skb_pull(), and skb_trim() with a
huge length does nothing. A wrongly framed clone then reaches
ipc_wwan_receive().

[Severity: High]
This is also a pre-existing issue that this patch did not introduce.
ipc_mux_dl_decode() dispatches IOSM_AGGR_MUX_SIG_ACBH blocks to
ipc_mux_dl_acb_decode(), which walks the command chain using indices
written by the modem:

drivers/net/wwan/iosm/iosm_ipc_mux_codec.c:ipc_mux_dl_acb_decode() {
    ...
	next_cmd_index = le32_to_cpu(acbh->first_cmd_index);
	next_cmd_index = array_index_nospec(next_cmd_index,
					    sizeof(struct mux_cmdh));

	while (next_cmd_index != 0) {
		cmdh = (struct mux_cmdh *)&block[next_cmd_index];
		next_cmd_index = le32_to_cpu(cmdh->next_cmd_index);
    ...
}

Only the first index is clamped. Is each later next_cmd_index used as an
offset into skb->data without any check against skb->len? That would let
the device make the kernel read struct mux_cmdh fields up to 4 GiB past
skb->data, which page-faults in the DL tasklet path.

The loop also has no forward-progress check, unlike the adth loop in
mux_dl_adb_decode(). A command whose next_cmd_index points back to itself
(for example first_cmd_index = 16 with the u32 at offset 24 also set to
16) looks like it would spin forever. On each pass it could call
ipc_mux_dl_acb_send_cmds() again.

The out-of-bounds write through link_status_resp.response in
ipc_mux_dl_acbcmd_decode() does not look reachable.
ipc_mux_dl_cmds_decode_process() returns -EINVAL for
IOSM_AGGR_MUX_CMD_LINK_STATUS_REPORT_RESP, so the concern is limited to
the out-of-bounds read and the endless loop.

[Severity: Medium]
This is another pre-existing issue that this patch did not introduce. Is
the CPU reading this DMA_FROM_DEVICE buffer while it is still mapped?

In ipc_imem_dl_skb_process(), only the CTRL case unmaps before using the
buffer:

	case IPC_CTYPE_CTRL:
		...
		ipc_pcie_addr_unmap(ipc_imem->pcie, IPC_CB(skb)->len,
				    IPC_CB(skb)->mapping,
				    IPC_CB(skb)->direction);
	...
	case IPC_CTYPE_WWAN:
		if (pipe->channel->if_id == IPC_MEM_MUX_IP_CH_IF_ID)
			ipc_mux_dl_decode(ipc_imem->mux, skb);

On the WWAN path, ipc_mux_dl_decode() reads the signature and runs the
ADB/ADGH/FCTH/ACB/CMD decoders. ipc_mux_net_receive() then hands
skb_clone()s to ipc_wwan_receive(). The unmap only happens in the final
ipc_pcie_kfree_skb() call. There is no dma_sync_single_for_cpu() anywhere
in drivers/net/wwan/iosm/.

With SWIOTLB bouncing, or on platforms without cache-coherent DMA, the
decoder and the network stack would see stale data. The unmap would then
copy the device data into a buffer that clones already in the stack may
still be reading. The dead-code exit added by this patch follows the same
late-unmap pattern.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102081920.50288-1-fangxy%40xiaopeng.com

[PATCH v2] net: wwan: iosm: drop redundant skb->data check in MUX DL decode

[Fang Xieyan]

The skb passed to ipc_mux_dl_decode() through the current RX path
always has a valid data pointer.

The only caller, ipc_imem_dl_skb_process(), checks for a NULL skb
before calling ipc_mux_dl_decode(). The skb is allocated through
ipc_pcie_alloc_skb() -> ipc_pcie_alloc_local_skb() ->
__netdev_alloc_skb(), which provides a valid data buffer on
successful allocation. Allocation or DMA mapping failures return
a NULL skb instead.

Remove the redundant skb->data check.

Suggested-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
Reviewed-by: Guanglei Zhu <zhugl3@xiaopeng.com>
Signed-off-by: Fang Xieyan <fangxy@xiaopeng.com>
---
Changes in v2:
- Drop the "fix potential skb leak" approach. The skb->data NULL
  condition is unreachable through the current RX path, so remove
  the redundant check instead of adding an ipc_pcie_kfree_skb() call.
- Drop the Fixes: tag because this patch does not fix a reachable bug.

 drivers/net/wwan/iosm/iosm_ipc_mux_codec.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/wwan/iosm/iosm_ipc_mux_codec.c b/drivers/net/wwan/iosm/iosm_ipc_mux_codec.c
index 0bbd41263cc2..2b8a22980c87 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_mux_codec.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_mux_codec.c
@@ -693,9 +693,6 @@ void ipc_mux_dl_decode(struct iosm_mux *ipc_mux, struct sk_buff *skb)
 {
 	u32 signature;
 
-	if (!skb->data)
-		return;
-
 	/* Decode the MUX header type. */
 	signature = le32_to_cpup((__le32 *)skb->data);
 
-- 
2.50.1