[PATCH net v2] net: usb: cdc_ncm: reject short GET_NTB_PARAMETERS responses

[PATCH net v2] net: usb: cdc_ncm: reject short GET_NTB_PARAMETERS responses

[Xinsheng Zhu]

usbnet_read_cmd() returns the actual transfer length, but cdc_ncm_init()
only checks for negative errors when reading the NTB parameters. A short
response therefore leaves the missing bytes zero-filled, and
initialization continues with parameters that were not supplied by the
device.

CDC NCM 1.1, section 7.2.1, Table 7-3 defines a 28-byte NTB
parameter structure. Require GET_NTB_PARAMETERS to return the
complete structure before continuing initialization.

Return -EIO for short responses, including zero-length responses, and
preserve negative errors from usbnet_read_cmd(). This ensures that a
zero-byte response cannot be reported as successful initialization when
rejecting an incomplete response.

The issue is theoretical and was found by manual inspection of
usbnet_read_cmd() callers with LLM assistance.

Fixes: 36c35416a94f ("cdc_ncm: fix endianness problem.")
Signed-off-by: Xinsheng Zhu <xinsheng.zhu.ny@gmail.com>
---
Changes in v2:
- Return -EIO for short responses, including zero-length responses,
  while preserving negative errors from usbnet_read_cmd().
- Add the missing Fixes tag.
- Changes of description in the commit message.

v1: https://lore.kernel.org/netdev/20261005041525.49111-1-xinsheng.zhu.ny@gmail.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 35db38cb3e4a..79893d2342ca 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -524,9 +524,9 @@ static int cdc_ncm_init(struct usbnet *dev)
 			      |USB_RECIP_INTERFACE,
 			      0, iface_no, &ctx->ncm_parm,
 			      sizeof(ctx->ncm_parm));
-	if (err < 0) {
+	if (err != sizeof(ctx->ncm_parm)) {
 		dev_err(&dev->intf->dev, "failed GET_NTB_PARAMETERS\n");
-		return err; /* GET_NTB_PARAMETERS is required */
+		return err < 0 ? err : -EIO; /* GET_NTB_PARAMETERS is required */
 	}
 
 	/* set CRC Mode */

base-commit: 6dc989ea46b96ce170840174b4a38c4a387fb005
-- 
2.54.0 (Apple Git-157)

Re: [PATCH net v2] net: usb: cdc_ncm: reject short GET_NTB_PARAMETERS responses

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usbnet: cdc_ncm: strict 28-byte length check breaks pre-errata 26-byte hardware
--

commit 21684fc876b53044a42f2941b653d84cd5d8ed14
Author: Xinsheng Zhu <xinsheng.zhu.ny@gmail.com>
net: usb: cdc_ncm: reject short GET_NTB_PARAMETERS responses

This patch updates the initialization sequence to strictly validate the
length of the GET_NTB_PARAMETERS response. Instead of only checking for
negative errors, it requires the response to exactly match the 28-byte
size of the ncm_parm structure to avoid using zero-filled parameters.

Link: https://lore.kernel.org/netdev/20261005041525.49111-1-xinsheng.zhu.ny@gmail.com/

> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -524,9 +524,9 @@ static int cdc_ncm_init(struct usbnet *dev)
>  			      |USB_RECIP_INTERFACE,
>  			      0, iface_no, &ctx->ncm_parm,
>  			      sizeof(ctx->ncm_parm));
> -	if (err < 0) {
> +	if (err != sizeof(ctx->ncm_parm)) {

[Severity: High]
Could this exact length check unconditionally fail on pre-errata CDC NCM 1.0
devices?

According to the driver's fallback logic further down in cdc_ncm_init(),
"devices prior to NCM Errata shall set this field to zero" when referring to
wNtbOutMaxDatagrams. Pre-errata devices typically return a 26-byte structure
instead of the full 28-byte structure.

Previously, with the leniency of checking if err is negative, a 26-byte read
would succeed and the kzalloc-backed ctx would safely leave the trailing
missing fields as zero. By requiring exactly 28 bytes, does this patch
unintentionally reject valid 26-byte responses from legacy hardware with -EIO,
preventing probe entirely on those devices?

>  		dev_err(&dev->intf->dev, "failed GET_NTB_PARAMETERS\n");
> -		return err; /* GET_NTB_PARAMETERS is required */
> +		return err < 0 ? err : -EIO; /* GET_NTB_PARAMETERS is required */
>  	}
>  
>  	/* set CRC Mode */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011035150.53315-1-xinsheng.zhu.ny@gmail.com?part=1

Re: [PATCH net v2] net: usb: cdc_ncm: reject short GET_NTB_PARAMETERS responses

[Xinsheng Zhu]

Thanks for the review.

> Pre-errata devices typically return a 26-byte structure
> instead of the full 28-byte structure.

The NTB parameter structure was already 28 bytes before the CDC
NCM 1.0 Errata. Issue K of CDC NCM 1.0 Errata [1] repurposed the
existing two-byte reserved field at offset 26 as wNtbOutMaxDatagrams;
it did not extend the structure from 26 to 28 bytes.

The driver comment says that "devices prior to NCM Errata shall set
this field to zero", not omit it from the response. A complete 28-byte
response with the last two bytes set to zero still passes the new length
check to avoid short read and uses the existing tx_max_datagrams
fallback later.

The patch does reject an actual 26-byte response, so compatibility
would be a concern if devices previously supported by the driver
return exactly that length of 26 bytes. However, the quoted
comment does not establish that such a response is the pre-errata
format.

This response was assisted by LLM.

[1] CDC NCM 1.0 Errata, Issue K:
https://www.usb.org/sites/default/files/NCM10_012011.zip

Thanks,
Xinsheng

On Sun, Oct 11, 2026 at 11:51 PM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] usbnet: cdc_ncm: strict 28-byte length check breaks pre-errata 26-byte hardware
> --
>
> commit 21684fc876b53044a42f2941b653d84cd5d8ed14
> Author: Xinsheng Zhu <xinsheng.zhu.ny@gmail.com>
> net: usb: cdc_ncm: reject short GET_NTB_PARAMETERS responses
>
> This patch updates the initialization sequence to strictly validate the
> length of the GET_NTB_PARAMETERS response. Instead of only checking for
> negative errors, it requires the response to exactly match the 28-byte
> size of the ncm_parm structure to avoid using zero-filled parameters.
>
> Link: https://lore.kernel.org/netdev/20261005041525.49111-1-xinsheng.zhu.ny@gmail.com/
>
> > diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> > --- a/drivers/net/usb/cdc_ncm.c
> > +++ b/drivers/net/usb/cdc_ncm.c
> > @@ -524,9 +524,9 @@ static int cdc_ncm_init(struct usbnet *dev)
> >                             |USB_RECIP_INTERFACE,
> >                             0, iface_no, &ctx->ncm_parm,
> >                             sizeof(ctx->ncm_parm));
> > -     if (err < 0) {
> > +     if (err != sizeof(ctx->ncm_parm)) {
>
> [Severity: High]
> Could this exact length check unconditionally fail on pre-errata CDC NCM 1.0
> devices?
>
> According to the driver's fallback logic further down in cdc_ncm_init(),
> "devices prior to NCM Errata shall set this field to zero" when referring to
> wNtbOutMaxDatagrams. Pre-errata devices typically return a 26-byte structure
> instead of the full 28-byte structure.
>
> Previously, with the leniency of checking if err is negative, a 26-byte read
> would succeed and the kzalloc-backed ctx would safely leave the trailing
> missing fields as zero. By requiring exactly 28 bytes, does this patch
> unintentionally reject valid 26-byte responses from legacy hardware with -EIO,
> preventing probe entirely on those devices?
>
> >               dev_err(&dev->intf->dev, "failed GET_NTB_PARAMETERS\n");
> > -             return err; /* GET_NTB_PARAMETERS is required */
> > +             return err < 0 ? err : -EIO; /* GET_NTB_PARAMETERS is required */
> >       }
> >
> >       /* set CRC Mode */
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20261011035150.53315-1-xinsheng.zhu.ny@gmail.com?part=1

Re: [PATCH net v2] net: usb: cdc_ncm: reject short GET_NTB_PARAMETERS responses

[netdev-bot+sashiko]

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 patch makes binding stricter for every CDC NCM, MBIM and
  Huawei NCM device, but the commit message does not say so.

--

net: usb: cdc_ncm: reject short GET_NTB_PARAMETERS responses

This patch changes cdc_ncm_init() so that GET_NTB_PARAMETERS must return
the full 28-byte NTB parameter structure. Before, only negative errors from
usbnet_read_cmd() were rejected. Short and zero-length responses now fail
with -EIO, and negative errors are passed through unchanged.

> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> index 35db38cb3e4a2..79893d2342ca1 100644
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -524,9 +524,9 @@ static int cdc_ncm_init(struct usbnet *dev)
>  			      |USB_RECIP_INTERFACE,
>  			      0, iface_no, &ctx->ncm_parm,
>  			      sizeof(ctx->ncm_parm));
> -	if (err < 0) {
> +	if (err != sizeof(ctx->ncm_parm)) {
>  		dev_err(&dev->intf->dev, "failed GET_NTB_PARAMETERS\n");
> -		return err; /* GET_NTB_PARAMETERS is required */
> +		return err < 0 ? err : -EIO; /* GET_NTB_PARAMETERS is required */
>  	}

[Severity: Medium]
Can this turn a short GET_NTB_PARAMETERS reply, which used to be tolerated,
into a bind failure for devices that currently work?

ctx->ncm_parm sits in the ctx that cdc_ncm_bind_common() allocates zeroed.
__usbnet_read_cmd() copies only the bytes the device actually returns, so
any fields that are missing read as zero.

The driver already replaces a zero value in each of these fields:

  cdc_ncm_check_rx_max() raises a too-small dwNtbInMaxSize to
  USB_CDC_NCM_NTB_MIN_IN_SIZE.

  cdc_ncm_check_tx_max():
	if (le32_to_cpu(ctx->ncm_parm.dwNtbOutMaxSize) == 0)
		max = CDC_NCM_NTB_MAX_SIZE_TX; /* dwNtbOutMaxSize not set */

  cdc_ncm_fix_modulus() resets tx_ndp_modulus, tx_modulus and tx_remainder
  to defaults.

  cdc_ncm_init() itself:
	/* devices prior to NCM Errata shall set this field to zero */
	ctx->tx_max_datagrams = le16_to_cpu(ctx->ncm_parm.wNtbOutMaxDatagrams);
	[ ... ]
	if ((ctx->tx_max_datagrams == 0) ||
			(ctx->tx_max_datagrams > CDC_NCM_DPT_DATAGRAMS_MAX))
		ctx->tx_max_datagrams = CDC_NCM_DPT_DATAGRAMS_MAX;

  If bmNtbFormatsSupported is 0, the driver simply keeps the mandatory
  NTB16 format.

With this change, any reply of 0 to 27 bytes makes cdc_ncm_init() return
-EIO, and cdc_ncm_bind_common() then takes the error path:

	if (cdc_ncm_init(dev))
		goto error2;

cdc_ncm_bind(), cdc_mbim_bind() and huawei_cdc_ncm_bind() all call
cdc_ncm_bind_common(). Would any NCM, MBIM or Huawei NCM device whose
firmware sends a short reply now lose its network interface?

For example, a pre-errata device that leaves out only the trailing
wNtbOutMaxDatagrams field would have worked before this patch. Would it
work to require only the bytes up to and including dwNtbOutMaxSize (or
wNdpOutAlignment)? Another option is to log a warning and continue.

The commit message says:

  "A short response therefore leaves the missing bytes zero-filled, and
   initialization continues with parameters that were not supplied by
   the device."

It does not say that those zero values are already replaced with
defaults, or that a tolerated firmware quirk now becomes a probe failure.
Could the commit message describe this trade-off?

About the tag:

  "Fixes: 36c35416a94f ("cdc_ncm: fix endianness problem.")"

Before 36c35416a94f, this code went through cdc_ncm_do_request() with
actlen == NULL. That returned 0 for any non-negative usb_control_msg()
result, so the reply length was not checked there either.

Is 36c35416a94f the right commit for the Fixes tag? Because the tag points
at a 2011 commit, this behaviour change is likely to be backported to
stable kernels.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261011035150.53315-1-xinsheng.zhu.ny%40gmail.com

Re: [PATCH net v2] net: usb: cdc_ncm: reject short GET_NTB_PARAMETERS responses

[Andrew Lunn]

On Sat, Oct 10, 2026 at 11:51:50PM -0400, Xinsheng Zhu wrote:
> usbnet_read_cmd() returns the actual transfer length, but cdc_ncm_init()
> only checks for negative errors when reading the NTB parameters. A short
> response therefore leaves the missing bytes zero-filled, and
> initialization continues with parameters that were not supplied by the
> device.
> 
> CDC NCM 1.1, section 7.2.1, Table 7-3 defines a 28-byte NTB
> parameter structure. Require GET_NTB_PARAMETERS to return the
> complete structure before continuing initialization.
> 
> Return -EIO for short responses, including zero-length responses, and
> preserve negative errors from usbnet_read_cmd(). This ensures that a
> zero-byte response cannot be reported as successful initialization when
> rejecting an incomplete response.
> 
> The issue is theoretical and was found by manual inspection of
> usbnet_read_cmd() callers with LLM assistance.

If this is theoretical, please drop the Fixes tag and post to
net-next.

	Andrew