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@xxxxxxxxxx> 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@xxxxxxxxx>
>
> [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