netdev - Re: [REGRESSION] asix: Lots of asix_rx_fixup() errors and slow transmissions

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 for Android: free password hash cracker in your pocket

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

Message-ID: <e4efd48d3c16c27de68d464ad3170b02@i4031.net>
Date:	Tue, 03 May 2016 20:28:10 -0400
From:	"David B. Robins" <linux@...idrobins.net>
To:	Dean Jenkins <Dean_Jenkins@...tor.com>
Cc:	John Stultz <john.stultz@...aro.org>,
	lkml <linux-kernel@...r.kernel.org>,
	Mark Craske <Mark_Craske@...tor.com>,
	"David S. Miller" <davem@...emloft.net>,
	YongQin Liu <yongqin.liu@...aro.org>,
	Guodong Xu <guodong.xu@...aro.org>, linux-usb@...r.kernel.org,
	netdev@...r.kernel.org, Ivan Vecera <ivecera@...hat.com>
Subject: Re: [REGRESSION] asix: Lots of asix_rx_fixup() errors and slow transmissions

On 2016-05-03 17:16, Dean Jenkins wrote:
> On 03/05/16 15:42, David B. Robins wrote:
>> 
>> I don't think the first one is giving you problems (except as 
>> triggered by the second) but I had concerns about the second myself 
>> (and emailed the author off-list, but received no reply), and we did 
>> not take that commit for our own product.
>> 
> Sorry, I might have missed your original E-mail.
> 
>> Specifically, the second change, 3f30... (original patch: 
>> https://www.mail-archive.com/netdev@vger.kernel.org/msg80720.html) (1) 
>> appears to do the exact opposite of what it claims, i.e., instead of 
>> "resync if this looks like a header", it does "resync if this does NOT 
>> look like a (packet) header", where "looks like a header" means "bits 
>> 0-10 (size) are equal to the bitwise-NOT of bits 16-26", and (2) can 
>> happen by coincidence for 1/2048 32-bit values starting a continuation 
>> URB (easy to hit dealing with large volumes of video data as we were). 
>> It appears to expect the header for every URB whereas the rest of the 
>> code at least expects it only once per network packet (look at 
>> following code that only reads it for remaining == 0).
> 
> David, I think that your interpretation is incorrect. Please see below.
> 
> Here is the code snippet from the patch with my annotations between #
> #, I will try to explain my intentions. Feel free to point out any
> flaws:
> 
>     if (rx->remaining && (rx->remaining + sizeof(u32) <= skb->len)) {
>         # Only runs when rx->remaining !=0 and the end of the Ethernet
> frame + next 32-bit header word is within the URB buffer. #
>         # Therefore, this code does not run when the end of an
> Ethernet frame has been reached in the previous URB #
>         # or when the end of the Ethernet frame + next 32-bit header
> word will be in a later URB buffer #

It may well be. I don't have the setup with me now, but I can try 
tomorrow to reproduce an environment where I can add some more detailed 
logging.

Since the URB length has to be >= than the remaining data plus a u32, 
the devices that John Stultz and I are using (AX88772B in my case) may 
be adding some additional data/padding after an Ethernet frame, 
expecting it to be discarded, and running into this check and its 
consequences. This may mean the device is badly behaved, if it is 
specified not to send anything extra; in any case, a well-intentioned 
error correction has gone badly, but I better understand the intent now. 
I am curious to know how often the device you are using benefits from 
this block of code.

> Regards,
> Dean

David