Re: [PATCH] net: wwan: mhi_wwan_mbim: validate datagram bounds before copy

Re: [PATCH] net: wwan: mhi_wwan_mbim: validate datagram bounds before copy

[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 · Low: 0

- [Medium] Commit message understates the severity of the bug being
  fixed.

Pre-existing issues:
- [High] The outer `while (1)` NDP walk in mhi_mbim_rx()
  (drivers/net/wwan/mhi_wwan_mbim.c) has no iteration bound and no…

--

net: wwan: mhi_wwan_mbim: validate datagram bounds before copy

This adds a bounds check in mhi_mbim_rx() so that an NDP16 datagram entry
whose offset plus length falls outside the received skb is skipped instead
of being copied.  The check mirrors the one in cdc_mbim.c.

A couple of questions below, one about the commit message wording and one
about the loop that encloses the changed lines.

> mhi_mbim_rx() copies datagrams from the NTB using skb_copy_bits() but
> never validates that the datagram offset and length from the NDP entry
> actually lie within the source skb. If a malicious or buggy modem sends
> an NDP entry with dgram_offset + dgram_len > skb->len, skb_copy_bits()
> returns -EFAULT and the destination skb is delivered with partially
> uninitialized data.

[Severity: Medium]
Is "partially uninitialized data" the right description of the pre-patch
behaviour?  skb_copy_bits() does the range test as its very first
statement:

net/core/skbuff.c:skb_copy_bits() {
	if (offset > (int)skb->len - len)
		goto fault;
	...
}

so on the out-of-range case nothing at all is copied into the destination
buffer.

In mhi_mbim_rx() the destination skb has already been allocated and
extended before the copy, and the copy's return value is discarded:

	skbn = netdev_alloc_skb(link->ndev, dgram_len);
	if (!skbn)
		continue;

	skb_put(skbn, dgram_len);
	skb_copy_bits(skb, dgram_offset, skbn->data, dgram_len);

netdev_alloc_skb() does not zero the payload area, so wouldn't every one
of the up-to-65535 bytes handed to netif_rx() be stale slab or page-frag
content rather than a partially filled datagram?  The protocol
classification that follows, skbn->data[0] & 0xf0, would also be reading
that stale content.

Would it be worth restating this as a full uninitialized kernel memory
disclosure driven by the modem-supplied wDatagramIndex/wDatagramLength
fields, so that stable and security triage see the real impact?

> diff --git a/drivers/net/wwan/mhi_wwan_mbim.c b/drivers/net/wwan/mhi_wwan_mbim.c
> index a949987125976..acdcaceebc488 100644
> --- a/drivers/net/wwan/mhi_wwan_mbim.c
> +++ b/drivers/net/wwan/mhi_wwan_mbim.c
> @@ -315,6 +315,9 @@ static void mhi_mbim_rx(struct mhi_mbim_context *mbim, struct sk_buff *skb)
>  			if (!dgram_offset || !dgram_len)
>  				break; /* null terminator */
>  
> +			if (dgram_offset + dgram_len > skb->len)
> +				continue;
> +
>  			skbn = netdev_alloc_skb(link->ndev, dgram_len);
>  			if (!skbn)
>  				continue;

[Severity: High]
This is a pre-existing issue and not introduced by this patch, but since
the change hardens the inner DPE loop against the same untrusted NDP data,
can the outer NDP walk in mhi_mbim_rx() still be spun forever by the
modem?

The loop back-edge takes wNextNdpIndex verbatim and zero is the only exit
condition:

drivers/net/wwan/mhi_wwan_mbim.c:mhi_mbim_rx() {
	while (1) {
		...
unlock:
		rcu_read_unlock();
next_ndp:
		/* Other NDP to process? */
		ndpoffset = (int)le16_to_cpu(ndp16.wNextNdpIndex);
		if (!ndpoffset)
			break;
	}
}

mbim_rx_verify_nth16() returns le16_to_cpu(nth16->wNdpIndex) unchanged,
and mbim_rx_verify_ndp16() only checks wLength against
USB_CDC_NCM_NDP16_LENGTH_MIN and that the DPE array fits in skb->len.
Neither places any constraint on wNextNdpIndex.

So for an NDP16 with wLength == 0x10 (nframes == 1) followed by a
{index=0,len=0} DPE, the inner for loop breaks immediately, and if
wNextNdpIndex points back at that same NDP the loop re-reads identical
bytes with no per-iteration state change (rx_seq is updated before the
loop, the skb is not modified), so is the !ndpoffset exit ever reached?

The function this patch takes its check from guards exactly this:

drivers/net/usb/cdc_mbim.c:cdc_mbim_rx_fixup() {
	int loopcount = 50; /* arbitrary max preventing infinite loop */
	...
	ndpoffset = le16_to_cpu(ndp16->wNextNdpIndex);
	if (ndpoffset && loopcount--)
		goto next_ndp;
}

MHI downlink callbacks reach mhi_mbim_rx() from a tasklet
(tasklet_schedule(&mhi_event->task) in drivers/bus/mhi/host/main.c), so
would an unterminated walk here wedge softirq processing on that CPU and
trip the soft lockup or RCU stall detectors?

Would adding an equivalent loopcount guard, or requiring wNextNdpIndex to
advance past the current NDP, be appropriate alongside this fix?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/AS8P251MB0001E25A247262552A62241BC8B22%40AS8P251MB0001.EURP251.PROD.OUTLOOK.COM

[PATCH] net: wwan: mhi_wwan_mbim: validate datagram bounds before copy

[Aamir Ahmed]

mhi_mbim_rx() copies datagrams from the NTB using skb_copy_bits() but
never validates that the datagram offset and length from the NDP entry
actually lie within the source skb. If a malicious or buggy modem sends
an NDP entry with dgram_offset + dgram_len > skb->len, skb_copy_bits()
returns -EFAULT and the destination skb is delivered with partially
uninitialized data.

Add a bounds check matching the one in cdc_mbim.c to skip datagrams
that would read past the end of the transfer block.

Fixes: aab8d56c11be ("net: Add Qualcomm MHI MBIM network driver")
Cc: stable@vger.kernel.org
Signed-off-by: Aamir Ahmed <elb12345@hotmail.co.uk>
---
 drivers/net/wwan/mhi_wwan_mbim.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/wwan/mhi_wwan_mbim.c b/drivers/net/wwan/mhi_wwan_mbim.c
index a94998712597..acdcaceebc48 100644
--- a/drivers/net/wwan/mhi_wwan_mbim.c
+++ b/drivers/net/wwan/mhi_wwan_mbim.c
@@ -315,6 +315,9 @@ static void mhi_mbim_rx(struct mhi_mbim_context *mbim, struct sk_buff *skb)
 			if (!dgram_offset || !dgram_len)
 				break; /* null terminator */
 
+			if (dgram_offset + dgram_len > skb->len)
+				continue;
+
 			skbn = netdev_alloc_skb(link->ndev, dgram_len);
 			if (!skbn)
 				continue;
-- 
2.43.0

Re: [PATCH] net: wwan: mhi_wwan_mbim: validate datagram bounds before copy

[Loic Poulain]

Hi Aamir,

Please review AI-generated fixes carefully before submitting them, it
will save reviewers time...

You've submitted a lot of patches in the last two days. I'd suggest
starting with just a few, if not one, to first validate that your
process is solid and that your AI-assisted fixes are reliable and
double checked, before moving to a bulk submission.

On Mon, Sep 7, 2026 at 3:49 AM Aamir Ahmed <elb12345@hotmail.co.uk> wrote:
>
> mhi_mbim_rx() copies datagrams from the NTB using skb_copy_bits() but
> never validates that the datagram offset and length from the NDP entry
> actually lie within the source skb. If a malicious or buggy modem sends
> an NDP entry with dgram_offset + dgram_len > skb->len, skb_copy_bits()
> returns -EFAULT and the destination skb is delivered with partially
> uninitialized data.
>
> Add a bounds check matching the one in cdc_mbim.c to skip datagrams
> that would read past the end of the transfer block.
>
> Fixes: aab8d56c11be ("net: Add Qualcomm MHI MBIM network driver")

That hash doesn't exist.

> Cc: stable@vger.kernel.org
> Signed-off-by: Aamir Ahmed <elb12345@hotmail.co.uk>
> ---
>  drivers/net/wwan/mhi_wwan_mbim.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/net/wwan/mhi_wwan_mbim.c b/drivers/net/wwan/mhi_wwan_mbim.c
> index a94998712597..acdcaceebc48 100644
> --- a/drivers/net/wwan/mhi_wwan_mbim.c
> +++ b/drivers/net/wwan/mhi_wwan_mbim.c
> @@ -315,6 +315,9 @@ static void mhi_mbim_rx(struct mhi_mbim_context *mbim, struct sk_buff *skb)
>                         if (!dgram_offset || !dgram_len)
>                                 break; /* null terminator */
>
> +                       if (dgram_offset + dgram_len > skb->len)
> +                               continue;

This seems redundant with the existing check in skb_copy_bits().
What would make more sense, however, is to check the return value of
skb_copy_bits().

Regards,
Loic


> +
>                         skbn = netdev_alloc_skb(link->ndev, dgram_len);
>                         if (!skbn)
>                                 continue;
> --
> 2.43.0
>

Re: [PATCH] net: wwan: mhi_wwan_mbim: validate datagram bounds before copy

[Aamir Ahmed]

On Mon, Sep 07, 2026 at 09:57:09AM +0200, Loic Poulain wrote:
> This seems redundant with the existing check in skb_copy_bits().
> What would make more sense, however, is to check the return value of
> skb_copy_bits().

Yes, this makes more sense than having to create a new guard - the
check already exists, the driver just ignores the return value.
Nothing is copied on failure, so the skb is delivered entirely
uninitialised, not partially as I first thought.

I will prepare v2 to check the return, free skbn, count an rx error
and break, as the copies above and the unknown-protocol path already
do. Will also drop the cdc_mbim rationale, which does not apply here.

> That hash doesn't exist.

Tag will be corrected in v2, thank you:

  Fixes: aa730a9905b7 ("net: wwan: Add MHI MBIM network driver")

> Please review AI-generated fixes carefully before submitting them, it
> will save reviewers time...

Yes, many apologies for this! This was a clear mistake on my part. For
the future I will be more selective and thorough in my patch
submission process.

Kind Regards

Aamir A.

Re: [PATCH] net: wwan: mhi_wwan_mbim: validate datagram bounds before copy

[Aamir Ahmed]

On Tue, Sep 08, 2026 at 01:37:32AM +0100, Aamir Ahmed wrote:
> I will prepare v2 to check the return, free skbn, count an rx error
> and break, as the copies above and the unknown-protocol path already
> do.

A v2 has been produced here by Guanglei Zhu with the above change.
This can be withdrawn. Thanks.

  net: wwan: mhi_wwan_mbim: check skb_copy_bits() return value (v2 2/3)
  https://lore.kernel.org/r/20260911021734.1396599-2-zhugl3@xiaopeng.com

pw-bot: rejected