netdev - Re: [PATCH v2] Bluetooth: Fix locking in bt_accept

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:   Mon, 7 Jan 2019 10:50:39 -0800
From:   Doug Anderson <dianders@...omium.org>
To:     Matthias Kaehlcke <mka@...omium.org>
Cc:     Marcel Holtmann <marcel@...tmann.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Dean Jenkins <Dean_Jenkins@...tor.com>,
        linux-bluetooth@...r.kernel.org,
        LKML <linux-kernel@...r.kernel.org>,
        netdev <netdev@...r.kernel.org>,
        Konstantin Khlebnikov <khlebnikov@...dex-team.ru>,
        Balakrishna Godavarthi <bgodavar@...eaurora.org>,
        Dmitry Grinberg <dmitrygr@...gle.com>, stable@...r.kernel.org
Subject: Re: [PATCH v2] Bluetooth: Fix locking in bt_accept_enqueue() for BH context

Hi,

On Wed, Jan 2, 2019 at 4:11 PM Matthias Kaehlcke <mka@...omium.org> wrote:
>
> With commit e16337622016 ("Bluetooth: Handle bt_accept_enqueue() socket
> atomically") lock_sock[_nested]() is used to acquire the socket lock
> before manipulating the socket. lock_sock[_nested]() may block, which
> is problematic since bt_accept_enqueue() can be called in bottom half
> context (e.g. from rfcomm_connect_ind()):
>
> [<ffffff80080d81ec>] __might_sleep+0x4c/0x80
> [<ffffff800876c7b0>] lock_sock_nested+0x24/0x58
> [<ffffff8000d7c27c>] bt_accept_enqueue+0x48/0xd4 [bluetooth]
> [<ffffff8000e67d8c>] rfcomm_connect_ind+0x190/0x218 [rfcomm]
>
> Add a parameter to bt_accept_enqueue() to indicate whether the
> function is called from BH context, and acquire the socket lock
> with bh_lock_sock_nested() if that's the case.
>
> Also adapt all callers of bt_accept_enqueue() to pass the new
> parameter:
>
> - l2cap_sock_new_connection_cb()
>   - uses lock_sock() to lock the parent socket => process context
>
> - rfcomm_connect_ind()
>   - acquires the parent socket lock with bh_lock_sock() => BH
>     context
>
> - __sco_chan_add()
>   - called from sco_chan_add(), which is called from sco_connect().
>     parent is NULL, hence bt_accept_enqueue() isn't called in this
>     code path and we can ignore it
>   - also called from sco_conn_ready(). uses bh_lock_sock() to acquire
>     the parent lock => BH context
>
> Fixes: e16337622016 ("Bluetooth: Handle bt_accept_enqueue() socket atomically")
> Signed-off-by: Matthias Kaehlcke <mka@...omium.org>
> ---
> Changes in v2:
> - use parameter in bt_accept_enqueue() to decide which lock to
>   acquire and adapt all callers
> - updated commit message
> ---
>  include/net/bluetooth/bluetooth.h |  2 +-
>  net/bluetooth/af_bluetooth.c      | 16 +++++++++++++---
>  net/bluetooth/l2cap_sock.c        |  2 +-
>  net/bluetooth/rfcomm/sock.c       |  2 +-
>  net/bluetooth/sco.c               |  2 +-
>  5 files changed, 17 insertions(+), 7 deletions(-)

This looks nice to me, thanks!

Cc: stable@...r.kernel.org
Reviewed-by: Douglas Anderson <dianders@...omium.org>