linux-kernel - Re: [PATCH v3 resend] Bluetooth: virtio

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: <794b9160-9647-7157-c84f-e278a375716e@opensynergy.com>
Date:   Tue, 11 Oct 2022 14:18:17 +0200
From:   Igor Skalkin <igor.skalkin@...nsynergy.com>
To:     "Michael S. Tsirkin" <mst@...hat.com>, linux-kernel@...r.kernel.org
Cc:     Dan Carpenter <dan.carpenter@...cle.com>,
        Hulk Robot <hulkci@...wei.com>,
        Yang Yingliang <yangyingliang@...wei.com>,
        Marcel Holtmann <marcel@...tmann.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        Luiz Augusto von Dentz <luiz.dentz@...il.com>,
        linux-bluetooth@...r.kernel.org
Subject: Re: [PATCH v3 resend] Bluetooth: virtio_bt: fix device removal

Tested, works fine in our projects.

Tested-by: Igor Skalkin <Igor.Skalkin@...nsynergy.com>

On 10/10/22 19:14, Michael S. Tsirkin wrote:
> Device removal is clearly out of virtio spec: it attempts to remove
> unused buffers from a VQ before invoking device reset. To fix, make
> open/close NOPs and do all cleanup/setup in probe/remove.
>
> NB: This is a hacky way to handle this - virtbt_{open,close} as NOP is
> not really what a driver is supposed to be doing. These are transport
> enable/disable callbacks from the BT core towards the driver. It maps to
> a device being enabled/disabled by something like bluetoothd for
> example. So if disabled, users expect that no resources/queues are in
> use.  It does work with all other transports like USB, SDIO, UART etc.
> There should be no buffer used if the device is powered off. We also
> don’t have any USB URBs in-flight if the transport is not active.
>
> The way to implement a proper fix would be using vq reset if supported,
> or even using a full device reset.
>
> The cost of the hack is a single skb wasted on an unused bt device.
>
> NB2: with this fix in place driver still suffers from a race condition
> if an interrupt triggers while device is being reset.  To fix, in the
> virtbt_close() callback we should deactivate all interrupts.  To be
> fixed.
>
> squashed fixup: bluetooth: virtio_bt: fix an error code in probe()
>
> Signed-off-by: Dan Carpenter <dan.carpenter@...cle.com>
> Reported-by: Hulk Robot <hulkci@...wei.com>
> Signed-off-by: Yang Yingliang <yangyingliang@...wei.com>
> Signed-off-by: Michael S. Tsirkin <mst@...hat.com>
> Message-Id: <20220811080943.198245-1-mst@...hat.com>
> ---
>
> resending due to v3 having been dropped
> changes from v2:
>       tkeaked commit log to make lines shorter
> changes from v1:
>       fixed error handling
>
>   drivers/bluetooth/virtio_bt.c | 19 +++++++++++++++++--
>   1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/bluetooth/virtio_bt.c b/drivers/bluetooth/virtio_bt.c
> index 67c21263f9e0..f6d699fed139 100644
> --- a/drivers/bluetooth/virtio_bt.c
> +++ b/drivers/bluetooth/virtio_bt.c
> @@ -50,8 +50,11 @@ static int virtbt_add_inbuf(struct virtio_bluetooth *vbt)
>
>   static int virtbt_open(struct hci_dev *hdev)
>   {
> -     struct virtio_bluetooth *vbt = hci_get_drvdata(hdev);
> +     return 0;
> +}
>
> +static int virtbt_open_vdev(struct virtio_bluetooth *vbt)
> +{
>       if (virtbt_add_inbuf(vbt) < 0)
>               return -EIO;
>
> @@ -61,7 +64,11 @@ static int virtbt_open(struct hci_dev *hdev)
>
>   static int virtbt_close(struct hci_dev *hdev)
>   {
> -     struct virtio_bluetooth *vbt = hci_get_drvdata(hdev);
> +     return 0;
> +}
> +
> +static int virtbt_close_vdev(struct virtio_bluetooth *vbt)
> +{
>       int i;
>
>       cancel_work_sync(&vbt->rx);
> @@ -354,8 +361,15 @@ static int virtbt_probe(struct virtio_device *vdev)
>               goto failed;
>       }
>
> +     virtio_device_ready(vdev);
> +     err = virtbt_open_vdev(vbt);
> +     if (err)
> +             goto open_failed;
> +
>       return 0;
>
> +open_failed:
> +     hci_free_dev(hdev);
>   failed:
>       vdev->config->del_vqs(vdev);
>       return err;
> @@ -368,6 +382,7 @@ static void virtbt_remove(struct virtio_device *vdev)
>
>       hci_unregister_dev(hdev);
>       virtio_reset_device(vdev);
> +     virtbt_close_vdev(vbt);
>
>       hci_free_dev(hdev);
>       vbt->hdev = NULL;

Please mind our privacy notice<https://www.opensynergy.com/datenschutzerklaerung/privacy-notice-for-business-partners-pursuant-to-article-13-of-the-general-data-protection-regulation-gdpr/> pursuant to Art. 13 GDPR. // Unsere Hinweise zum Datenschutz gem. Art. 13 DSGVO finden Sie hier.<https://www.opensynergy.com/de/datenschutzerklaerung/datenschutzhinweise-fuer-geschaeftspartner-gem-art-13-dsgvo/>