On Tue, 2025-06-10 at 14:20 +0200, Alexander Bluhm wrote: > On Tue, Jun 10, 2025 at 09:34:19AM +0000, Gerhard Roth wrote: > > > > Can (int)dlen be negative??? I am not an USB expert, but it seems > > > > that this length ultimatly comes from sc_rx_bufsz or sc_tx_bufsz. > > > > There I see another UGETDW(np.dwNtbInMaxSize) and > > > > UGETDW(np.dwNtbOutMaxSize).?? Do we need a size check in > > > > umb_ncm_setup()? > > > > You probably mean 'len' (instead of 'dlen'). > > I agree that an additional check in umb_ncm_setup() would be > > a good idea. > > > > Please look at the proposed patch at the end of this mail. > > Now you have DPRINTFN() only in the successful case. Maybe warn > or DPRINTF if maxpktlen lies within unexpected range.
Ok, will add a DRPINTFN() there before committing the change.
>
> There exists a MBIM_MAXSEGSZ_MINVAL. Should the sanity check also
> test maxpktlen >= MBIM_MAXSEGSZ_MINVAL ?
I wouldn't do so. This value is defined by the MBIM spec and it states
for 'wMaxSegmentSize', that "This number must not be smaller than 2048."
But the driver has no problem in handling even smaller segments.
Gerhard
>
> OK bluhm@
>
> > Index: sys/dev/usb/if_umb.c
> > ===================================================================
> > RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
> > diff -u -p -u -p -r1.62 if_umb.c
> > --- sys/dev/usb/if_umb.c 10 Jun 2025 08:29:57 -0000 1.62
> > +++ sys/dev/usb/if_umb.c 10 Jun 2025 09:31:02 -0000
> > @@ -359,6 +359,7 @@ umb_attach(struct device *parent, struct
> > int altnum;
> > int s;
> > struct ifnet *ifp;
> > + int maxpktlen;
> >
> > sc->sc_udev = uaa->device;
> > sc->sc_ctrl_ifaceno = uaa->ifaceno;
> > @@ -461,10 +462,14 @@ umb_attach(struct device *parent, struct
> > MBIM_CTRLMSG_MAXLEN);
> > /* cont. anyway */
> > }
> > - sc->sc_maxpktlen = UGETW(md->wMaxSegmentSize);
> > - DPRINTFN(2, "%s: ctrl_len=%d, maxpktlen=%d,
> > cap=0x%x\n",
> > - DEVNAM(sc), sc->sc_ctrl_len, sc->sc_maxpktlen,
> > - md->bmNetworkCapabilities);
> > + maxpktlen = UGETW(md->wMaxSegmentSize);
> > + if (maxpktlen > 0 && maxpktlen <= INT32_MAX) {
> > + sc->sc_maxpktlen = maxpktlen;
> > + DPRINTFN(2, "%s: ctrl_len=%d, maxpktlen=%d,
> > "
> > + "cap=0x%x\n", DEVNAM(sc),
> > sc->sc_ctrl_len,
> > + sc->sc_maxpktlen,
> > + md->bmNetworkCapabilities);
> > + }
> > break;
> > default:
> > break;
> > @@ -709,8 +714,10 @@ umb_ncm_setup(struct umb_softc *sc)
> > USETW(req.wLength, sizeof (np));
> > if (usbd_do_request(sc->sc_udev, &req, &np) ==
> > USBD_NORMAL_COMPLETION &&
> > UGETW(np.wLength) == sizeof (np)) {
> > - sc->sc_rx_bufsz = UGETDW(np.dwNtbInMaxSize);
> > - sc->sc_tx_bufsz = UGETDW(np.dwNtbOutMaxSize);
> > + sc->sc_rx_bufsz = MIN(UGETDW(np.dwNtbInMaxSize),
> > + (uint32_t)sc->sc_maxpktlen);
> > + sc->sc_tx_bufsz = MIN(UGETDW(np.dwNtbOutMaxSize),
> > + (uint32_t)sc->sc_maxpktlen);
> > sc->sc_maxdgram = UGETW(np.wNtbOutMaxDatagrams);
> > sc->sc_align = UGETW(np.wNdpOutAlignment);
> > sc->sc_ndp_div = UGETW(np.wNdpOutDivisor);
> >
> >
>
>
smime.p7s
Description: S/MIME cryptographic signature
