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

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

[Xinsheng Zhu]

usbnet_read_cmd() returns the number of bytes read on success, which may
be smaller than the requested size. cdc_ncm_init() calls it and only
checks for negative return values, allowing initialization to continue
with an incomplete NTB parameter response.

The context is allocated and zero-initialized by kzalloc_obj() in
cdc_ncm_bind_common(). Bytes missing from a short response therefore
remain zero and may be interpreted as device-provided parameters.

Require the return value to equal sizeof(ctx->ncm_parm), consistent with
the existing exact-length check for GET_MAX_DATAGRAM_SIZE in
cdc_ncm_set_dgram_size().

Signed-off-by: Xinsheng Zhu <xinsheng.zhu.ny@gmail.com>
---
 drivers/net/usb/cdc_ncm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 35db38cb3e4a..66f40f0d9c27 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -524,7 +524,7 @@ 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 */
 	}

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

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[Xinsheng Zhu]

I noticed this while manually reviewing usbnet_read_cmd() callers for
short-read handling.
This is a theoretical issue based on code inspection.

On Mon, Oct 5, 2026 at 12:19 AM <netdev-bot+sinfo@kernel.org> wrote:
>
> Hi!
>
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
>
>  - How the issue was discovered, e.g. hit in production, hit during
>    development, syzbot report, manual code inspection, LLM or static
>    analysis tool scan.
>
>  - Whether the issue was actually triggered, or is only theoretical
>    (e.g. found by code inspection). If it was triggered please include
>    the symptoms, like the stack trace or error messages.
>
> Please do not repost the series just to address the above. Instead,
> reply to this email with the missing information, so that reviewers
> can take it into account. If the series needs another revision for
> other reasons, please include the information in the commit messages
> then.
>
> The evaluation is done by an LLM so it may be wrong, if you think
> that is the case please reply and explain.

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

[netdev-bot+sashiko]

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

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

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

- [High] In cdc_ncm_init() (drivers/net/usb/cdc_ncm.c:527-529), the new
  check `if (err != sizeof(ctx->ncm_parm)) { dev_err(...); return err;…
- [Low] The patch makes any GET_NTB_PARAMETERS response shorter than 28
  bytes fatal to bind (cdc_ncm_bind_common() -> error2 -> -ENODEV).
- [Low] The commit message describes a defect: cdc_ncm_init() only
  checked `err < 0`, so initialization continued with an incomplete…

--

net: usb: cdc_ncm: reject short GET_NTB_PARAMETERS responses

This patch changes the GET_NTB_PARAMETERS check in cdc_ncm_init() from
err < 0 to an exact-length comparison against sizeof(ctx->ncm_parm). The
goal is to reject short responses instead of continuing initialization
with zero-filled NTB parameters.

> Require the return value to equal sizeof(ctx->ncm_parm), consistent with
> the existing exact-length check for GET_MAX_DATAGRAM_SIZE in
> cdc_ncm_set_dgram_size().
>
> Signed-off-by: Xinsheng Zhu <xinsheng.zhu.ny@gmail.com>

[Severity: Low]
This isn't a bug, but this fixes the unchecked response length in
cdc_ncm_init(). Should it carry a Fixes: tag pointing at the commit that
added the err < 0 check, so stable backporters can follow it?

> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> index 35db38cb3e4a2..66f40f0d9c273 100644
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -524,7 +524,7 @@ 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: Low]
With this check, any response shorter than sizeof(ctx->ncm_parm) now makes
bind fail. There is no fallback, and the wLength the device reports is not
used.

Before this patch, short responses were accepted. The zero-filled fields
were then sanitized by cdc_ncm_fix_modulus(), by
cdc_ncm_check_rx_max()/cdc_ncm_check_tx_max(), and by the
tx_max_datagrams == 0 default.

The commit message calls this "consistent with the existing exact-length
check for GET_MAX_DATAGRAM_SIZE in cdc_ncm_set_dgram_size()". That check
is not fatal, though:

	if (err != sizeof(max_datagram_size)) {
		dev_dbg(&dev->intf->dev, "GET_MAX_DATAGRAM_SIZE failed\n");
		goto out;
	}

No device that sends a short response has been identified. That makes
this a compatibility question, not confirmed breakage. Is it intended that
these responses now cause a probe failure for cdc_ncm, cdc_mbim and
huawei_cdc_ncm, which all share this bind path?

>  		dev_err(&dev->intf->dev, "failed GET_NTB_PARAMETERS\n");
>  		return err; /* GET_NTB_PARAMETERS is required */
                        ^^^

[Severity: High]
What happens when the device ends the GET_NTB_PARAMETERS data stage with
a zero-length packet? usbnet_read_cmd() then returns 0. Does this branch
log "failed GET_NTB_PARAMETERS" and still return 0, which the caller reads
as success?

__usbnet_read_cmd() passes the usb_control_msg() length through unchanged:

drivers/net/usb/usbnet.c:__usbnet_read_cmd() {
    ...
	err = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
			      cmd, reqtype, value, index, buf, size,
			      USB_CTRL_GET_TIMEOUT);
	if (err > 0 && err <= size) {
    ...
	return err;
}

cdc_ncm_bind_common() only treats a non-zero return as a failure:

	if (cdc_ncm_init(dev))
		goto error2;

So bind would continue, but the rest of cdc_ncm_init() never runs.
SET_CRC_MODE and SET_NTB_FORMAT are never sent. ctx->is_ndp16,
ctx->tx_max_datagrams, ctx->max_ndp_size and ctx->timer_interval all keep
their zero values from the kzalloc.

With ctx->tx_max_datagrams == 0, this loop in cdc_ncm_fill_tx_frame()
never runs:

	for (n = ctx->tx_curr_frame_num; n < ctx->tx_max_datagrams; n++) {

Each incoming skb then goes to the "free up any dangling skb" block, gets
freed and is counted in tx_dropped.

On the RX side, is_ndp16 is 0, so cdc_ncm_rx_fixup() calls
cdc_ncm_rx_verify_nth32(). The device was never sent SET_NTB_FORMAT, so it
is still using its default NTB16 format.

With CDC_NCM_FLAG_NDP_TO_END set, kzalloc(ctx->max_ndp_size) with a size
of 0 returns ZERO_SIZE_PTR. That passes the NULL check in
cdc_ncm_bind_common(), so the problem is not caught there either.

Before this patch, a zero-length response went through the full
initialization with sanitized defaults. Isn't this a regression for the
zero-length case? Short reads of 1 to 27 bytes do fail bind, but only
because the positive byte count is non-zero.

Should this branch return a negative errno for short reads? For example:

	return err < 0 ? err : -EIO;

>  	}

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

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

[Xinsheng Zhu]

Thanks for the review.

> Should this branch return a negative errno for short reads? For example:
>
>         return err < 0 ? err : -EIO;

Yes. I changed the condition without adjusting the return value and
overlooked the zero-length case.

When usbnet_read_cmd() returns 0, the new branch returns 0 before the
remaining initialization has run. cdc_ncm_bind_common() then treats that
as success. This is a regression introduced by v1.

I will fix this in v2 by preserving negative errors and returning -EIO
for short responses, including zero-length responses:

        if (err != sizeof(ctx->ncm_parm)) {
                dev_err(&dev->intf->dev, "failed GET_NTB_PARAMETERS\n");
                return err < 0 ? err : -EIO; /* GET_NTB_PARAMETERS is
required */
        }

> Is it intended that these responses now cause a probe failure for
> cdc_ncm, cdc_mbim and huawei_cdc_ncm, which all share this bind path?

Rejecting incomplete GET_NTB_PARAMETERS responses here was the intent,
to avoid continuing initialization with parameters that the device did
not provide.

> Should it carry a Fixes: tag?

Yes. I will add the missing tag in v2:

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

Thanks,
Xinsheng

pw-bot: cr

On Wed, Oct 7, 2026 at 12:16 AM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 · Medium: 0 · Low: 2
>
> 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).
>
> - [High] In cdc_ncm_init() (drivers/net/usb/cdc_ncm.c:527-529), the new
>   check `if (err != sizeof(ctx->ncm_parm)) { dev_err(...); return err;…
> - [Low] The patch makes any GET_NTB_PARAMETERS response shorter than 28
>   bytes fatal to bind (cdc_ncm_bind_common() -> error2 -> -ENODEV).
> - [Low] The commit message describes a defect: cdc_ncm_init() only
>   checked `err < 0`, so initialization continued with an incomplete…
>
> --
>
> net: usb: cdc_ncm: reject short GET_NTB_PARAMETERS responses
>
> This patch changes the GET_NTB_PARAMETERS check in cdc_ncm_init() from
> err < 0 to an exact-length comparison against sizeof(ctx->ncm_parm). The
> goal is to reject short responses instead of continuing initialization
> with zero-filled NTB parameters.
>
> > Require the return value to equal sizeof(ctx->ncm_parm), consistent with
> > the existing exact-length check for GET_MAX_DATAGRAM_SIZE in
> > cdc_ncm_set_dgram_size().
> >
> > Signed-off-by: Xinsheng Zhu <xinsheng.zhu.ny@gmail.com>
>
> [Severity: Low]
> This isn't a bug, but this fixes the unchecked response length in
> cdc_ncm_init(). Should it carry a Fixes: tag pointing at the commit that
> added the err < 0 check, so stable backporters can follow it?
>
> > diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> > index 35db38cb3e4a2..66f40f0d9c273 100644
> > --- a/drivers/net/usb/cdc_ncm.c
> > +++ b/drivers/net/usb/cdc_ncm.c
> > @@ -524,7 +524,7 @@ 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: Low]
> With this check, any response shorter than sizeof(ctx->ncm_parm) now makes
> bind fail. There is no fallback, and the wLength the device reports is not
> used.
>
> Before this patch, short responses were accepted. The zero-filled fields
> were then sanitized by cdc_ncm_fix_modulus(), by
> cdc_ncm_check_rx_max()/cdc_ncm_check_tx_max(), and by the
> tx_max_datagrams == 0 default.
>
> The commit message calls this "consistent with the existing exact-length
> check for GET_MAX_DATAGRAM_SIZE in cdc_ncm_set_dgram_size()". That check
> is not fatal, though:
>
>         if (err != sizeof(max_datagram_size)) {
>                 dev_dbg(&dev->intf->dev, "GET_MAX_DATAGRAM_SIZE failed\n");
>                 goto out;
>         }
>
> No device that sends a short response has been identified. That makes
> this a compatibility question, not confirmed breakage. Is it intended that
> these responses now cause a probe failure for cdc_ncm, cdc_mbim and
> huawei_cdc_ncm, which all share this bind path?
>
> >               dev_err(&dev->intf->dev, "failed GET_NTB_PARAMETERS\n");
> >               return err; /* GET_NTB_PARAMETERS is required */
>                         ^^^
>
> [Severity: High]
> What happens when the device ends the GET_NTB_PARAMETERS data stage with
> a zero-length packet? usbnet_read_cmd() then returns 0. Does this branch
> log "failed GET_NTB_PARAMETERS" and still return 0, which the caller reads
> as success?
>
> __usbnet_read_cmd() passes the usb_control_msg() length through unchanged:
>
> drivers/net/usb/usbnet.c:__usbnet_read_cmd() {
>     ...
>         err = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
>                               cmd, reqtype, value, index, buf, size,
>                               USB_CTRL_GET_TIMEOUT);
>         if (err > 0 && err <= size) {
>     ...
>         return err;
> }
>
> cdc_ncm_bind_common() only treats a non-zero return as a failure:
>
>         if (cdc_ncm_init(dev))
>                 goto error2;
>
> So bind would continue, but the rest of cdc_ncm_init() never runs.
> SET_CRC_MODE and SET_NTB_FORMAT are never sent. ctx->is_ndp16,
> ctx->tx_max_datagrams, ctx->max_ndp_size and ctx->timer_interval all keep
> their zero values from the kzalloc.
>
> With ctx->tx_max_datagrams == 0, this loop in cdc_ncm_fill_tx_frame()
> never runs:
>
>         for (n = ctx->tx_curr_frame_num; n < ctx->tx_max_datagrams; n++) {
>
> Each incoming skb then goes to the "free up any dangling skb" block, gets
> freed and is counted in tx_dropped.
>
> On the RX side, is_ndp16 is 0, so cdc_ncm_rx_fixup() calls
> cdc_ncm_rx_verify_nth32(). The device was never sent SET_NTB_FORMAT, so it
> is still using its default NTB16 format.
>
> With CDC_NCM_FLAG_NDP_TO_END set, kzalloc(ctx->max_ndp_size) with a size
> of 0 returns ZERO_SIZE_PTR. That passes the NULL check in
> cdc_ncm_bind_common(), so the problem is not caught there either.
>
> Before this patch, a zero-length response went through the full
> initialization with sanitized defaults. Isn't this a regression for the
> zero-length case? Short reads of 1 to 27 bytes do fail bind, but only
> because the positive byte count is non-zero.
>
> Should this branch return a negative errno for short reads? For example:
>
>         return err < 0 ? err : -EIO;
>
> >       }
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261005041525.49111-1-xinsheng.zhu.ny%40gmail.com

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

[Andrew Lunn]

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

What has endinesses got to do with message length? A truncated wrong
endian message is just as bad as a truncated correct endian message.

If you want a fixes tag, please make sure it is actually correct, go
back in the history and find the real place the issue was introduced,
not the last time the code was touched.

    Andrew