netdev - RE: [PATCH v2 net-next 3/8] net: cdc_ncm: inform usbnet when rx buffers are reduced

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 30 May 2014 12:39:55 +0000
From:	David Laight <David.Laight@...LAB.COM>
To:	'Bjørn Mork' <bjorn@...k.no>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
	Alexey Orishko <alexey.orishko@...il.com>,
	Oliver Neukum <oliver@...kum.org>,
	Enrico Mioso <mrkiko.rs@...il.com>,
	Lars Melin <larsm17@...il.com>, Peter Stuge <peter@...ge.se>,
	Greg Suarez <gsuarez@...thmicro.com>
Subject: RE: [PATCH v2 net-next 3/8] net: cdc_ncm: inform usbnet when rx
 buffers are reduced

From: Bjørn Mork
> David Laight <David.Laight@...LAB.COM> writes:
> > From: Bjørn Mork
> >> It doesn't matter whether the buffer size goes up or down.  We have to
> >> keep usbnet and device syncronized to be able to split transfers at the
> >> correct boundaries. The spec allow skipping short packets when using
> >> max sized transfers.  If we don't tell usbnet about our new expected rx
> >> buffer size, then it will merge and/or split NTBs.  The driver does not
> >> support this, and the result will be lots of framing errors.
> >>
> >> Fix by always reallocating usbnet rx buffers when the rx_max value
> >> changes.
> >
> > I'm guessing that the rx_max value is the maximum size of the USB bulk
> > data 'message' that the device generates?
> >
> > As such the URB only need to be longer that it.
> 
> So did I think too at first.  That's how I added the bug fixed by this
> commit :-)
> 
> The problem with NCM is that it explicitly allows (and encourage) using
> transfers which are multiples of the USB packet size, *without* any
> terminating short packet (0 or 1 byte). This means that the USB core
> won't know or care about the end of one transfer and the beginning of
> the next.  Which is fine.  But the cdc_ncm driver has to know, because
> it must split the transfers into frames it can decode.
> 
> Now, the current cdc_ncm implementation has a built-in assumption that
> the size of the URB == rx_max.  This lets it simplify the splitting into
> frames to nearly nothing: Any received URB contains exactly one frame.
> Therefore we need to keep the rx URB size strictly syncronized the
> rx_max.

Hmmm....
So there is an ethernet packet size somewhere near 500 that exactly fills
a 512 byte USB2 frame.
If you receive one of those does the hardware send a 0 length terminating
fragment?
If not the then URB won't complete until after the next ethernet frame
arrives.
Receive three of them followed by a longer frame and you'll overrun the
end of the URB.

At least one path through usbnet ensures that the rx buffers aren't
a multiple of the USB frame size. I'm not sure whether that really helps.

Sounds like the hardware expects you to receive each USB bulk buffer
separately.

	David