On Fri, Apr 26, 2024 at 5:16 AM Dan Carpenter <[email protected]> wrote:
>
> On Fri, Apr 26, 2024 at 03:20:05AM -0400, Sungwoo Kim wrote:
> > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> > index 84fc70862..a8f414ab8 100644
> > --- a/net/bluetooth/l2cap_core.c
> > +++ b/net/bluetooth/l2cap_core.c
> > @@ -3953,6 +3953,9 @@ static struct l2cap_chan *l2cap_connect(struct l2cap_conn *conn,
> > if (!chan)
> > goto response;
> >
> > + l2cap_chan_hold(chan);
> > + l2cap_chan_lock(chan);
> > +
> > /* For certain devices (ex: HID mouse), support for authentication,
> > * pairing and bonding is optional. For such devices, inorder to avoid
> > * the ACL alive for too long after L2CAP disconnection, reset the ACL
> > @@ -4041,6 +4044,11 @@ static struct l2cap_chan *l2cap_connect(struct l2cap_conn *conn,
> > chan->num_conf_req++;
> > }
> >
> > + if (chan) {
> > + l2cap_chan_unlock(chan);
> > + l2cap_chan_put(chan);
> > + }
> > +
> > return chan;
> ^^^^^^^^^^^^
> This doesn't fix the bug because we're returning chan.
>
> As soon as you call l2cap_chan_put() then chan will be freed by in the
> other thread which is doing l2cap_conn_del() resulting in a use after
> free in the caller.
Thank you for pointing this out.
No caller uses the return value of l2cap_connect() if the kernel
versions >= v6.9.
So, l2cap_connect() can return void.
One caller uses the return value of l2cap_connect() in v4.19 <= the
kernel versions <= v6.8.
In this case, the caller should unlock and put a channel.
Question: Can different patches be applied for different versions like
the above?
Thanks,
Sungwoo Kim.
On Fri, Apr 26, 2024 at 05:35:01AM -0400, Sungwoo Kim wrote:
> > > +
> > > return chan;
> > ^^^^^^^^^^^^
> > This doesn't fix the bug because we're returning chan.
> >
> > As soon as you call l2cap_chan_put() then chan will be freed by in the
> > other thread which is doing l2cap_conn_del() resulting in a use after
> > free in the caller.
>
> Thank you for pointing this out.
> No caller uses the return value of l2cap_connect() if the kernel
> versions >= v6.9.
> So, l2cap_connect() can return void.
>
> One caller uses the return value of l2cap_connect() in v4.19 <= the
> kernel versions <= v6.8.
> In this case, the caller should unlock and put a channel.
>
> Question: Can different patches be applied for different versions like
> the above?
Ah... Very good. I assumed it was used. The the commit which stopped
using the return value, commit e7b02296fb40 ("Bluetooth: Remove BT_HS"),
has been back ported to earlier kernels as well.
Generally, we just write code against the latest kernel and worry about
backports as a separate issue. We sometimes re-write patches slightly
if that's necessary for the backport.
I'm not an expert in bluetooth, but I think your patch seems correct.
Let's make l2cap_connect() void as well. Wait for a day or two for
other comments and then send a v2 patch.
https://staticthinking.wordpress.com/2022/07/27/how-to-send-a-v2-patch/
Here is how you write the commit message:
========================================================================
[PATCH v2] Bluetooth: L2CAP: Fix slab-use-after-free in l2cap_connect()
KASAN detected a use after free in l2cap_send_cmd().
BUG: KASAN: slab-use-after-free in l2cap_send_cmd+0x5dc/0x830 net/bluetooth/l2cap_core.c:968
[free]
l2cap_conn_del
┌ mutex_lock(&conn->chan_lock);
│ foreach chan in conn->chan_l: ... (2)
│ l2cap_chan_put(chan);
│ l2cap_chan_destroy
│ kfree(chan) ... (3) <-- chan freed
└ mutex_unlock(&conn->chan_lock);
[use]
l2cap_bredr_sig_cmd
l2cap_connect
┌ mutex_lock(&conn->chan_lock);
│ chan = pchan->ops->new_connection(pchan); <-- allocates chan
│ __l2cap_chan_add(conn, chan);
│ l2cap_chan_hold(chan);
│ list_add(&chan->list, &conn->chan_l); ... (1)
└ mutex_unlock(&conn->chan_lock);
chan->conf_state ... (4) <-- use after free
To fix this, this patch holds and locks the l2cap channel.
Also make the l2cap_connect() return type void. Nothing is using the
returned value but it is ugly to return a potentially freed pointer.
Making it void will help with backports because earlier kernels did use
the return value. Now the compile will break for kernels where this
patch is not a complete fix.
Fixes: 73ffa904b782 ("Bluetooth: Move conf_{req,rsp} stuff to struct l2cap_chan")
Signed-off-by: