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

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

[xujiahao]

cdc_ncm_ndp16() and cdc_ncm_ndp32() verify that the new NDP and the
datagram fit into the NTB with an unsigned subtraction:

	if ((ctx->tx_curr_size - skb->len - reserve) < ctx->max_ndp_size)
		return NULL;

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.  A second datagram in the same NTB is therefore exactly the
case this check has to reject, and that is also the case where
skb->len + reserve exceeds tx_curr_size: the subtraction wraps around to
a huge value, the check passes, and the skb_put_zero() of max_ndp_size
bytes that follows overruns the buffer:

  skbuff: skb_over_panic: text:ffff80000db33558 len:428 put:172
  head:ffff0000c2c66100 tail:0x1ac end:0x140
  kernel BUG at net/core/skbuff.c:200
  skb_put_zero include/linux/skbuff.h:2595 [inline]
  cdc_ncm_ndp16 drivers/net/usb/cdc_ncm.c:1132 [inline]
  cdc_ncm_fill_tx_frame+0xdf0/0x30e0 drivers/net/usb/cdc_ncm.c:1309

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

The room check in the aggregation loop cannot catch this, because it is
done after the NDP has already been pushed.

Rewrite the test as an addition, which cannot underflow and gives the
same answer whenever the original subtraction did not wrap.

Reported-by: syzbot+1994e2ecf323ed90f255@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=1994e2ecf323ed90f255
Signed-off-by: xujiahao <xujiahao@cmss.chinamobile.com>
---
 drivers/net/usb/cdc_ncm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 19db5a668..e92dc7f01 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -1125,7 +1125,7 @@ static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp16(struct cdc_ncm_ctx *ctx, struct s
 		cdc_ncm_align_tail(skb, ctx->tx_ndp_modulus, 0, ctx->tx_curr_size);
 
 	/* verify that there is room for the NDP and the datagram (reserve) */
-	if ((ctx->tx_curr_size - skb->len - reserve) < ctx->max_ndp_size)
+	if (skb->len + reserve + ctx->max_ndp_size > ctx->tx_curr_size)
 		return NULL;
 
 	/* link to it */
@@ -1180,7 +1180,7 @@ static struct usb_cdc_ncm_ndp32 *cdc_ncm_ndp32(struct cdc_ncm_ctx *ctx, struct s
 		cdc_ncm_align_tail(skb, ctx->tx_ndp_modulus, 0, ctx->tx_curr_size);
 
 	/* verify that there is room for the NDP and the datagram (reserve) */
-	if ((ctx->tx_curr_size - skb->len - reserve) < ctx->max_ndp_size)
+	if (skb->len + reserve + ctx->max_ndp_size > ctx->tx_curr_size)
 		return NULL;
 
 	/* link to it */
-- 
2.43.0

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

[xujiahao]

cdc_ncm_ndp16() and cdc_ncm_ndp32() verify that the new NDP and the
datagram fit into the NTB with an unsigned subtraction:

	if ((ctx->tx_curr_size - skb->len - reserve) < ctx->max_ndp_size)
		return NULL;

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.  A second datagram in the same NTB is therefore exactly the
case this check has to reject, and that is also the case where
skb->len + reserve exceeds tx_curr_size: the subtraction wraps around to
a huge value, the check passes, and the skb_put_zero() of max_ndp_size
bytes that follows overruns the buffer:

  skbuff: skb_over_panic: text:ffff80000db33558 len:428 put:172
  head:ffff0000c2c66100 tail:0x1ac end:0x140
  kernel BUG at net/core/skbuff.c:200
  skb_put_zero include/linux/skbuff.h:2595 [inline]
  cdc_ncm_ndp16 drivers/net/usb/cdc_ncm.c:1132 [inline]
  cdc_ncm_fill_tx_frame+0xdf0/0x30e0 drivers/net/usb/cdc_ncm.c:1309

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

The room check in the aggregation loop cannot catch this, because it is
done after the NDP has already been pushed.

Rewrite the test as an addition, which cannot underflow and gives the
same answer whenever the original subtraction did not wrap.

Fixes: c78b7c586657 ("net: cdc_ncm: refactoring for tx multiplexing")
Fixes: 0fa81b304a79 ("cdc_ncm: Implement the 32-bit version of NCM Transfer Block")
Reported-by: syzbot+1994e2ecf323ed90f255@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=1994e2ecf323ed90f255
Signed-off-by: xujiahao <xujiahao@cmss.chinamobile.com>
---
v2:
 - Add the Fixes: tags reported by sashiko-bot below.  No code change.

 drivers/net/usb/cdc_ncm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 19db5a668..e92dc7f01 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -1125,7 +1125,7 @@ static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp16(struct cdc_ncm_ctx *ctx, struct s
 		cdc_ncm_align_tail(skb, ctx->tx_ndp_modulus, 0, ctx->tx_curr_size);
 
 	/* verify that there is room for the NDP and the datagram (reserve) */
-	if ((ctx->tx_curr_size - skb->len - reserve) < ctx->max_ndp_size)
+	if (skb->len + reserve + ctx->max_ndp_size > ctx->tx_curr_size)
 		return NULL;
 
 	/* link to it */
@@ -1180,7 +1180,7 @@ static struct usb_cdc_ncm_ndp32 *cdc_ncm_ndp32(struct cdc_ncm_ctx *ctx, struct s
 		cdc_ncm_align_tail(skb, ctx->tx_ndp_modulus, 0, ctx->tx_curr_size);
 
 	/* verify that there is room for the NDP and the datagram (reserve) */
-	if ((ctx->tx_curr_size - skb->len - reserve) < ctx->max_ndp_size)
+	if (skb->len + reserve + ctx->max_ndp_size > ctx->tx_curr_size)
 		return NULL;
 
 	/* link to it */
-- 
2.43.0

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 1

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.
- [Low] The patch fixes a syzbot-reported skb_over_panic() (BUG() in
  net/core/skbuff.c) in the cdc_ncm/cdc_mbim TX path but has no Fixes:…

--

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
check can then no longer wrap and let the following skb_put_zero() of
max_ndp_size bytes overrun the NTB, which was reported by syzbot as an
skb_over_panic() in cdc_ncm_fill_tx_frame().

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.  A second datagram in the same NTB is therefore exactly the
> case this check has to reject, and that is also the case where
> skb->len + reserve exceeds tx_curr_size: the subtraction wraps around to
> a huge value, the check passes, and the skb_put_zero() of max_ndp_size
> bytes that follows overruns the buffer:

[Severity: Medium]
The code change looks right, but does this paragraph describe the
trigger and the root cause accurately?

Can a second datagram in the same NTB reach this check at all? Before
the room check, cdc_ncm_ndp16() walks the NDP chain and returns any
existing NDP that has the same signature:

drivers/net/usb/cdc_ncm.c:cdc_ncm_ndp16() {
    ...
	while (ndpoffset) {
		ndp16 = (struct usb_cdc_ncm_ndp16 *)(skb->data + ndpoffset);
		if  (ndp16->dwSignature == sign)
			return ndp16;
		ndpoffset = le16_to_cpu(ndp16->wNextNdpIndex);
	}
    ...
}

cdc_ncm_tx_fixup() always passes one fixed signature
(USB_CDC_NCM_NDP16_NOCRC_SIGN or USB_CDC_NCM_NDP32_NOCRC_SIGN). On plain
cdc_ncm the check therefore only runs for the first datagram of each
NTB.

As far as I can tell, the only way to add another NDP to an NTB that
already holds datagrams is cdc_mbim_tx_fixup()->cdc_ncm_fill_tx_frame().
That path uses a different signature for each MBIM session.

The clamp also doesn't seem to guarantee room for one datagram plus one
NDP. cdc_ncm_setup() clamps tx_max before it sets the datagram size:

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);

	/* sanitize the modulus and remainder values */
	cdc_ncm_fix_modulus(dev);

	/* set max datagram size */
	cdc_ncm_set_dgram_size(dev, cdc_ncm_max_dgram_size(dev));
    ...
}

ctx is allocated with kzalloc() in cdc_ncm_bind_common(), so
ctx->max_datagram_size is still 0 when cdc_ncm_check_tx_max() computes:

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

That makes the floor only 172 + 12 = 184 bytes. def_tx is
min(CDC_NCM_NTB_DEF_SIZE_TX, dwNtbOutMaxSize), so a device that reports
dwNtbOutMaxSize = 320 gets tx_max = 320. That matches end:0x140.

With that tx_max, the reported len:428 put:172 (skb_out->len is 256 when
the NDP is pushed) could come from either of two paths:

  - an NDP for a second MBIM session on cdc_mbim

  - the first datagram on plain cdc_ncm, when the device reports
    wNdpOutAlignment = 256. cdc_ncm_fix_modulus() accepts this because
    256 < tx_max. cdc_ncm_align_tail() then pads the 12-byte NTH up to
    256 before the room check.

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

Where do the 140 and the 8 come from? sizeof(struct usb_cdc_ncm_nth16)
is 12, and 8 is sizeof(struct usb_cdc_ncm_ndp16). max_datagram_size
doesn't appear to ever be 140, and it is 0 when the tx_max floor is
computed.

Could the commit message be updated to describe the actual triggers?
Those are a device-controlled dwNtbOutMaxSize/wNdpOutAlignment, or MBIM
multi-session NDPs. It could also mention that cdc_ncm_setup() clamps
tx_max before max_datagram_size is set.

> Reported-by: syzbot+1994e2ecf323ed90f255@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=1994e2ecf323ed90f255
> Signed-off-by: xujiahao <xujiahao@cmss.chinamobile.com>

[Severity: Low]
Should this have a Fixes: tag? The patch fixes a kernel BUG that can be
reached from the cdc_ncm and cdc_mbim TX paths. netdev expects a Fixes:
tag on fixes, and stable backports rely on it.

According to blame, the subtraction-based room check in cdc_ncm_ndp16()
was added by:

  c78b7c586657 ("net: cdc_ncm: refactoring for tx multiplexing")

and its current tx_curr_size form comes from:

  e1069bbfcf3b ("net: cdc_ncm: Reduce memory use when kernel memory low")

cdc_ncm_ndp32() has the same check.

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