Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220325033028.1.I67f8ad854ac2f48701902bfb34d6e2070011b779@changeid>
 <CABBYNZKF1Ye6D130XgaFmqN6JAssf78-FaQh_AEkwigy8qaVjw@mail.gmail.com>
In-Reply-To: <CABBYNZKF1Ye6D130XgaFmqN6JAssf78-FaQh_AEkwigy8qaVjw@mail.gmail.com>
From:   Ying Hsu <yinghsu@chromium.org>
Date:   Sat, 26 Mar 2022 14:31:22 +0800
Message-ID: <CADwQ6b6cCrKGfS-zhh5KnSNdxm_n_krwpZ=s68WYdoJ-XCHH-Q@mail.gmail.com>
Subject: Re: [PATCH] Bluetooth: fix dangling sco_conn and use-after-free in sco_sock_timeout
To:     Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc:     Marcel Holtmann <marcel@holtmann.org>,
        ChromeOS Bluetooth Upstreaming 
        <chromeos-bluetooth-upstreaming@chromium.org>,
        Joseph Hwang <josephsih@chromium.org>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Johan Hedberg <johan.hedberg@gmail.com>,
        Paolo Abeni <pabeni@redhat.com>,
        "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "open list:NETWORKING [GENERAL]" <netdev@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Luiz,

I compiled and ran the c-reproducer:
https://syzkaller.appspot.com/x/repro.c?x=152b93e8700000
I will add relevant links in the commit message. Thanks for the reminder.

While fixing the use-after-free problem , I also found a possible
deadlock in sco_sock_connect() and sco_sock_getsockopt() :
sco_sock_connect() {
  hci_dev_lock(hdev);
  lock_sock(sk);
}

sco_sock_getsockopt() {
  lock_sock(sk);
  case BT_CODEC:
    hci_dev_lock(hdev);
}

So, adjusting the locking order in sco_sock_connect() can also avoid
the possible deadlock.

Ying

On Sat, Mar 26, 2022 at 2:50 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Ying,
>
> On Thu, Mar 24, 2022 at 8:31 PM Ying Hsu <yinghsu@chromium.org> wrote:
> >
> > Connecting the same socket twice consecutively in sco_sock_connect()
> > could lead to a race condition where two sco_conn objects are created
> > but only one is associated with the socket. If the socket is closed
> > before the SCO connection is established, the timer associated with the
> > dangling sco_conn object won't be canceled. As the sock object is being
> > freed, the use-after-free problem happens when the timer callback
> > function sco_sock_timeout() accesses the socket. Here's the call trace:
> >
> > dump_stack+0x107/0x163
> > ? refcount_inc+0x1c/
> > print_address_description.constprop.0+0x1c/0x47e
> > ? refcount_inc+0x1c/0x7b
> > kasan_report+0x13a/0x173
> > ? refcount_inc+0x1c/0x7b
> > check_memory_region+0x132/0x139
> > refcount_inc+0x1c/0x7b
> > sco_sock_timeout+0xb2/0x1ba
> > process_one_work+0x739/0xbd1
> > ? cancel_delayed_work+0x13f/0x13f
> > ? __raw_spin_lock_init+0xf0/0xf0
> > ? to_kthread+0x59/0x85
> > worker_thread+0x593/0x70e
> > kthread+0x346/0x35a
> > ? drain_workqueue+0x31a/0x31a
> > ? kthread_bind+0x4b/0x4b
> > ret_from_fork+0x1f/0x30
> >
> > Signed-off-by: Ying Hsu <yinghsu@chromium.org>
> > Reviewed-by: Joseph Hwang <josephsih@chromium.org>
> > ---
> > Tested this commit using a C reproducer on qemu-x86_64 for 8 hours.
>
> We should probably add a link or something to the reproducer then, was
> it syzbot? It does have some instructions on how to link its issues.
>
> >  net/bluetooth/sco.c | 21 +++++++++++++--------
> >  1 file changed, 13 insertions(+), 8 deletions(-)
> >
> > diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> > index 8eabf41b2993..380c63194736 100644
> > --- a/net/bluetooth/sco.c
> > +++ b/net/bluetooth/sco.c
> > @@ -574,19 +574,24 @@ static int sco_sock_connect(struct socket *sock, struct sockaddr *addr, int alen
> >             addr->sa_family != AF_BLUETOOTH)
> >                 return -EINVAL;
> >
> > -       if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND)
> > -               return -EBADFD;
> > +       lock_sock(sk);
> > +       if (sk->sk_state != BT_OPEN && sk->sk_state != BT_BOUND) {
> > +               err = -EBADFD;
> > +               goto done;
> > +       }
> >
> > -       if (sk->sk_type != SOCK_SEQPACKET)
> > -               return -EINVAL;
> > +       if (sk->sk_type != SOCK_SEQPACKET) {
> > +               err = -EINVAL;
> > +               goto done;
> > +       }
> >
> >         hdev = hci_get_route(&sa->sco_bdaddr, &sco_pi(sk)->src, BDADDR_BREDR);
> > -       if (!hdev)
> > -               return -EHOSTUNREACH;
> > +       if (!hdev) {
> > +               err = -EHOSTUNREACH;
> > +               goto done;
> > +       }
> >         hci_dev_lock(hdev);
> >
> > -       lock_sock(sk);
> > -
>
> Also are we sure we are not introducing a locking hierarchy problem
> here? Previously we had hci_dev_lock then sock_lock now it is the
> opposite, or perhaps we never want to have them at the same time?
>
> >         /* Set destination address and psm */
> >         bacpy(&sco_pi(sk)->dst, &sa->sco_bdaddr);
> >
> > --
> > 2.35.1.1021.g381101b075-goog
> >
>
>
> --
> Luiz Augusto von Dentz