Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-4104-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Fri, 26 Apr 2024 13:24:54 +0300
From: Dan Carpenter <dan.carpenter@linaro.org>
To: Sungwoo Kim <iam@sung-woo.kim>
Cc: daveti@purdue.edu, Marcel Holtmann <marcel@holtmann.org>,
	Johan Hedberg <johan.hedberg@gmail.com>,
	Luiz Augusto von Dentz <luiz.dentz@gmail.com>,
	linux-bluetooth@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] Bluetooth: L2CAP: Fix slab-use-after-free in
 l2cap_send_cmd
Message-ID: <5639ab86-2d12-45ce-864c-80fbc0c79094@moroto.mountain>
References: <20240426072006.358802-1-iam@sung-woo.kim>
 <cff764c2-a3d1-4a12-9260-54122e7a1fef@moroto.mountain>
 <CAJNyHp+eCq1p_gTbxAJBKouAw-oQ=j5xwyjk5o-91pf_hGdrAg@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAJNyHp+eCq1p_gTbxAJBKouAw-oQ=j5xwyjk5o-91pf_hGdrAg@mail.gmail.com>

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: