netdev - Re: [PATCH net-next 02/14] net: cdc_ncm: use device rx

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]

Message-ID: <87vce6u52w.fsf@nemi.mork.no>
Date:	Fri, 19 Oct 2012 12:30:31 +0200
From:	Bjørn Mork <bjorn@...k.no>
To:	Alexey Orishko <alexey.orishko@...il.com>
Cc:	Oliver Neukum <oliver@...kum.org>, netdev@...r.kernel.org,
	linux-usb@...r.kernel.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Greg Suarez <gpsuarez2512@...il.com>,
	"Fangxiaozhi \(Franko\)" <fangxiaozhi@...wei.com>,
	Dan Williams <dcbw@...hat.com>,
	Aleksander Morgado <aleksander@...edo.com>
Subject: Re: [PATCH net-next 02/14] net: cdc_ncm: use device rx_max value if update failed

Alexey Orishko <alexey.orishko@...il.com> writes:
> On Fri, Oct 19, 2012 at 11:30 AM, Bjørn Mork <bjorn@...k.no> wrote:
>> Bjørn Mork <bjorn@...k.no> writes:
>>> Alexey Orishko <alexey.orishko@...il.com> writes:
>>>
>> OK, I did some more experiments, and I am wondering if the real firmware
>> problem is in the MBIM descriptor.  It is
>>
>>       CDC MBIM:
>>         bcdMBIMVersion       1.00
>>         wMaxControlMessage   1536
>>         bNumberFilters       16
>>         bMaxFilterSize       40
>>         wMaxSegmentSize      4096
>>         bmNetworkCapabilities 0x20
>>           8-byte ntb input size
>>
>>
>> so we use the 8-byte version of USB_CDC_SET_NTB_INPUT_SIZE, which fails
>> with -EPIPE.  But forcing the 4-byte version seems to work. Hmm, I also
>> see that the device returns 4 bytes in response to at
>> USB_CDC_GET_NTB_INPUT_SIZE with an 8-byte buffer.  Maybe we can
>> auto-quirk based on that?  I.e., if USB_CDC_GET_NTB_INPUT_SIZE returns
>> only 4 bytes then we assume that the bmNetworkCapabilities flag is
>> wrong.
>>
>> Is that acceptable?  Then it seems we are able to inform this device of
>> the reduced buffer, and the other problems can be ignored.
>>
>
> Based on NCM errata, NCM functional descriptor: if Bit 5 is set, then
> device can (must) handle 8-byte form of Set/GetNtbInputSize.

Yes, and this flag is copied with the same requirements in MBIM.  So it
is definitely a firmware bug.

> If they set a flag, but don't support the feature, hm.. is it a
> prototype device or
> commercially available one?

The firmware is not commercially availabe AFAIK, but based on experience
I believe we should assume that any firmware bug ever seen will live
forever.  There are likely other devices with the same bug even if this
firmware and this device, and all other devices from the same vendor,
are fixed.

I believe the problem here is that the USB descriptors are somewhat
decoupled from the firmware functions.  The firmware functions are
usually implemented in firmware originating from the chipset vendor,
while the descriptors are up to the device vendor. This has led to
interesting situations before.  But for us, I believe it means that we
should put more trust in the responses to control messages than in
functional descriptors.  So if we can detect a mismatch like this one,
then we do that and use the control message.

> At least device must support Set request, but for GetNtbInputSize we
> could happily
> live without wNtbInMaxDatagrams (i.e. use 4 byte variant) on Linux.
> Since we must anyway receive a complete NTB and making any number of skb
> buffers from received NTB is not a problem at all.

Yes, it doesn't matter to us whether we get the 8 byte variant or
not. We are prepared to handle the 4 byte variant in any case, if the
functional descriptor flag is not set.  So I believe a fallback to 4
byte should not pose any problem.  The only difference will be that we
need to do the USB_CDC_GET_NTB_INPUT_SIZE to detect the bug in the first
place.

I'll cook up an alternative patch implementing this so you can evaluate
the impact.

Bjørn
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html