Re: [PATCH net v2] net: usb: cdc_ncm: don't underflow the NDP room check

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · 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 code change is correct, but the commit message explains
  the bug wrongly, so maintainers and backporters get a false picture of…

--

net: usb: cdc_ncm: don't underflow the NDP room check

This patch rewrites the NDP room check in cdc_ncm_ndp16() and
cdc_ncm_ndp32() as an addition instead of an unsigned subtraction. The
old form could wrap when skb->len + reserve exceeded tx_curr_size, and
then skb_put_zero() of max_ndp_size bytes could overrun the skb.

Closes: https://syzkaller.appspot.com/bug?extid=1994e2ecf323ed90f255

> tx_max is only clamped to max_datagram_size + max_ndp_size + the NTH
> header size, so tx_curr_size is just big enough for one datagram and
> one NDP.

[Severity: Medium]
The code change looks correct. Is this description of the clamp accurate
at bind time, though?

cdc_ncm_setup() clamps tx_max before max_datagram_size is set:

drivers/net/usb/cdc_ncm.c:cdc_ncm_setup() {
    ...
	/* clamp rx_max and tx_max and inform device */
	cdc_ncm_update_rxtx_max(dev, def_rx, def_tx);
    ...
	/* set max datagram size */
	cdc_ncm_set_dgram_size(dev, cdc_ncm_max_dgram_size(dev));
    ...
}

ctx is zero-allocated. So when
cdc_ncm_update_rxtx_max()->cdc_ncm_check_tx_max() computes:

	min = ctx->max_datagram_size + ctx->max_ndp_size + sizeof(struct usb_cdc_ncm_nth16);

max_datagram_size is still 0, and the lower bound is just max_ndp_size
plus the NTH16 header.

If a device advertises a small dwNtbOutMaxSize, can tx_max end up well
below one datagram plus one NDP?

> Here end is 0x140 = 320 = 140 + 172 + 8, i.e. the one-datagram minimum.

Can max_datagram_size really be 140 here? cdc_ncm_set_dgram_size() clamps
it to at least cdc_ncm_min_dgram_size(). That is CDC_NCM_MIN_DATAGRAM_SIZE
(1514) or CDC_MBIM_MIN_DATAGRAM_SIZE (2048).

Also, sizeof(struct usb_cdc_ncm_nth16) is 12 bytes, not 8.

Could end = 0x140 instead come from alloc_skb() rounding a small
tx_curr_size up to its allocation size, rather than from a tx_max minimum?

> A second datagram in the same NTB is therefore exactly the
> case this check has to reject

Is this the right case? The room check only runs when there is no NDP
with a matching dwSignature yet, because the chain walk in
cdc_ncm_ndp16() returns early on a match:

	/* follow the chain of NDPs, looking for a match */
	while (ndpoffset) {
		ndp16 = (struct usb_cdc_ncm_ndp16 *)(skb->data + ndpoffset);
		if  (ndp16->dwSignature == sign)
			return ndp16;
		...
	}

cdc_ncm_tx_fixup() always passes the same NOCRC signature, so a second
datagram with that signature never reaches the check.

Wouldn't the triggering cases be the first datagram of an NTB, or a
datagram with a different signature? One example is cdc_mbim_tx_fixup()
with several MBIM sessions.

Could the commit message be updated to describe how tx_curr_size ends up
smaller than one datagram plus one NDP?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110151624.696101-1-xujiahao%40cmss.chinamobile.com