netdev - Re: [PATCH v3 1/2] Bluetooth: btusb: define HCI packet sizes of USB Alts

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: <9810329.nUPlyArG6x@zen.local>
Date:   Tue, 08 Dec 2020 15:04:29 -0800
From:   Trent Piepho <tpiepho@...il.com>
To:     Joseph Hwang <josephsih@...gle.com>,
        linux-bluetooth <linux-bluetooth@...r.kernel.org>
Cc:     Marcel Holtmann <marcel@...tmann.org>,
        Luiz Augusto von Dentz <luiz.dentz@...il.com>,
        chromeos-bluetooth-upstreaming 
        <chromeos-bluetooth-upstreaming@...omium.org>,
        Alain Michaud <alainm@...omium.org>,
        Abhishek Pandit-Subedi <abhishekpandit@...omium.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        Pali Rohár <pali@...nel.org>
Subject: Re: [PATCH v3 1/2] Bluetooth: btusb: define HCI packet sizes of USB Alts

On Wednesday, September 23, 2020 3:22:15 AM PST Pali Rohár wrote:
> On Monday 14 September 2020 20:18:27 Joseph Hwang wrote:
> > On Thu, Sep 10, 2020 at 4:18 PM Pali Rohár <pali@...nel.org> wrote:
> > > And this part of code which you write is Realtek specific.
> > 
> > We currently only have Intel and Realtek platforms to test with. If
> > making it generic without proper testing platforms is fine, I will
> > make it generic. Or do you think it might be better to make it
> > customized with particular vendors for now; and make it generic later
> > when it works well with sufficient vendors?
> 
> I understood that those packet size changes are generic to bluetooth
> specification and therefore it is not vendor specific code. Those packet
> sizes for me really seems to be USB specific.
> 
> Therefore it should apply for all vendors, not only for Realtek and
> Intel.

I have tried to test WBS with some different USB adapters.  So far, all use 
these packet sizes.  Tested were:

Broadcom BRCM20702A
Realtek RTL8167B
Realtek RTL8821C
CSR CSR8510 (probably fake)

In all cases, WBS works best with packet size of (USB packet size for alt mode 
selected) * 3 packets - 3 bytes HCI header.  None of these devices support alt 
6 mode, where supposedly one packet is better, but I can find no BT adapter on 
which to test this.

> +static const int hci_packet_size_usb_alt[] = { 0, 24, 48, 72, 96, 144, 60};

Note that the packet sizes here are based on the max isoc packet length for 
the USB alt mode used, e.g. alt 1 is 9 bytes.  That value is only a 
"recommended" value from the bluetooth spec.  It seems like it would be more 
correct use (btusb_data*)->isoc_tx_ep->wMaxPacketSize to find the MTU.

> > [Issue 2] The btusb_work() is performed by a worker. There would be a
> > timing issue here if we let btusb_work() to do “hdev->sco_mtu =
> > hci_packet_size_usb_alt[i]” because there is no guarantee how soon the
> > btusb_work() can be finished and get “hdev->sco_mtu” value set
> > correctly. In order to avoid the potential race condition, I suggest
> > to determine air_mode in btusb_notify() before
> > schedule_work(&data->work) is executed so that “hdev->sco_mtu =
> > hci_packet_size_usb_alt[i]” is guaranteed to be performed when
> > btusb_notify() finished. In this way, hci_sync_conn_complete_evt() can
> > set conn->mtu correctly as described in [Issue 1] above.

Does this also give userspace a clear point at which to determine MTU setting, 
_before_ data is sent over SCO connection?  It will not work if sco_mtu is not 
valid until after userspace sends data to SCO connection with incorrect mtu.