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

[netdev-bot+sashiko@kernel.org]

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