linux-kernel - Re: [syzbot] general protection fault in l2cap_chan

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>] [day] [month] [year] [list]

Message-ID: <CABBYNZJGs0OQd3s0r7BWcY8pXm90E+HB6FG5CDf4DdL7i9T4qg@mail.gmail.com>
Date:   Tue, 1 Jun 2021 13:48:30 -0700
From:   Luiz Augusto von Dentz <luiz.dentz@...il.com>
To:     Hillf Danton <hdanton@...a.com>
Cc:     syzbot <syzbot+008cdbf7a9044c2c2f99@...kaller.appspotmail.com>,
        David Miller <davem@...emloft.net>,
        Johan Hedberg <johan.hedberg@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        "linux-bluetooth@...r.kernel.org" <linux-bluetooth@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Marcel Holtmann <marcel@...tmann.org>,
        "open list:NETWORKING [GENERAL]" <netdev@...r.kernel.org>,
        syzkaller-bugs@...glegroups.com
Subject: Re: [syzbot] general protection fault in l2cap_chan_timeout (2)

Hi,

On Tue, Jun 1, 2021 at 12:54 AM Hillf Danton <hdanton@...a.com> wrote:
>
> On Mon, 31 May 2021 19:11:08 -0700 Luiz Augusto von Dentz wrote:
> >
> >Shouldn't we actually cancel the timeout work if the connection is
> >freed here? At least I don't see a valid reason to have a l2cap_chan
> >without l2cap_conn.
>
> A far neater approach at the cost of making l2cap_conn_put() blocking and
> nobody currently seems to care about it.

I wonder what is going on here, there doesn't seem to be any code path
where the chan_timer is not cleared since the code path should be:

l2cap_conn_del -> l2cap_chan_del -> __clear_chan_timer -> cancel_delayed_work
                                                             chan->conn = NULL

Perhaps the problem is that cancel_delayed_work does not actually
prevent l2cap_chan_timeout to run if that is already pending, so maybe
something like this would work:

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 9ebb85df4db4..f6e423111dfc 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -414,17 +414,23 @@ static void l2cap_chan_timeout(struct work_struct *work)
 {
        struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
                                               chan_timer.work);
-       struct l2cap_conn *conn = chan->conn;
+       struct l2cap_conn *conn;
        int reason;

        BT_DBG("chan %p state %s", chan, state_to_string(chan->state));

-       mutex_lock(&conn->chan_lock);
        /* __set_chan_timer() calls l2cap_chan_hold(chan) while scheduling
         * this work. No need to call l2cap_chan_hold(chan) here again.
         */
        l2cap_chan_lock(chan);

+       conn = chan->conn;
+       if (!conn)
+               /* l2cap_conn_del might have run */
+               goto unlock;
+
+       mutex_lock(&conn->chan_lock);
+
        if (chan->state == BT_CONNECTED || chan->state == BT_CONFIG)
                reason = ECONNREFUSED;
        else if (chan->state == BT_CONNECT &&
@@ -437,10 +443,11 @@ static void l2cap_chan_timeout(struct work_struct *work)

        chan->ops->close(chan);

+       mutex_unlock(&conn->chan_lock);
+
+unlock:
        l2cap_chan_unlock(chan);
        l2cap_chan_put(chan);
-
-       mutex_unlock(&conn->chan_lock);
 }


-- 
Luiz Augusto von Dentz