netdev - Re: [PATCH net-next v3 2/2] usbnet: ipheth: add CDC NCM support

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: Wed, 31 May 2023 17:10:01 +0200
From: Foster Snowhill <forst@....gy>
To: Paolo Abeni <pabeni@...hat.com>, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>
Cc: Georgi Valkov <gvalkov@...il.com>,
 Simon Horman <simon.horman@...igine.com>, Jan Kiszka
 <jan.kiszka@...mens.com>, linux-usb@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v3 2/2] usbnet: ipheth: add CDC NCM support

Hello Paolo,

Thank you for the review!

>> -	rx_buf = usb_alloc_coherent(iphone->udev, IPHETH_BUF_SIZE + IPHETH_IP_ALIGN,
>> +	rx_buf = usb_alloc_coherent(iphone->udev, IPHETH_RX_BUF_SIZE,
> 
> Here the driver already knows if the device is in NCM or legacy mode,
> so perhaps we could select the buffer size accordingly? You would
> probably need to store the actual buffer size somewhere to keep the
> buffer handling consistent and simple in later code.

Agreed, I will make the buffer size dynamic in the next revision.

The RX buffer size is 1516 bytes for legacy mode (2 bytes padding +
1514 bytes Ethernet frame), and 65536 bytes for NCM mode.

>>  	memcpy(dev->tx_buf, skb->data, skb->len);
>> -	if (skb->len < IPHETH_BUF_SIZE)
>> -		memset(dev->tx_buf + skb->len, 0, IPHETH_BUF_SIZE - skb->len);
>>
>>  	usb_fill_bulk_urb(dev->tx_urb, udev,
>>  			  usb_sndbulkpipe(udev, dev->bulk_out),
>> -			  dev->tx_buf, IPHETH_BUF_SIZE,
>> +			  dev->tx_buf, skb->len,
>>  			  ipheth_sndbulk_callback,
>>  			  dev);
>>  	dev->tx_urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
> 
> This chunk looks unrelated from NCM support, and unconditionally
> changes the established behaviour even with legacy mode, why?
> 
> Does that works even with old(er) devices?

I see Georgi Valkov said he tested v3 of the patch on older iOS devices
and confirmed it working. I'll chat with him to get some USB traffic
captures, to check what is macOS' behaviour with such devices (to make
sure we behave the same way as the official driver). I also wanted to
investigate a bit, when was NCM support even added to iOS.

Personally I remember testing this in legacy mode a while ago, before
I implemented NCM. I will re-test it again in legacy mode in addition
to Georgi's efforts.

>From my side, I think it's reasonable to split this out into a separate
patch, since it technically applies to the legacy mode as well, and
doesn't (directly) relate to NCM support, as you pointed out.

There's no reason to send the full buffer every time including padding,
but I'll check out traffic captures on macOS + older devices, maybe
that's what they're doing.