* [PATCH] Bluetooth: L2CAP: Fix use-after-free caused by l2cap_chan_put
@ 2022-07-20 17:06 Luiz Augusto von Dentz
2022-07-20 18:13 ` bluez.test.bot
2022-07-21 7:43 ` [PATCH] " Lee Jones
0 siblings, 2 replies; 4+ messages in thread
From: Luiz Augusto von Dentz @ 2022-07-20 17:06 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
This fixes the following trace which is caused by hci_rx_work starting up
*after* the final channel reference has been put() during sock_close() but
*before* the references to the channel have been destroyed, so instead
the code now rely on kref_get_unless_zero/l2cap_chan_hold_unless_zero to
prevent referencing a channel that is about to be destroyed.
refcount_t: increment on 0; use-after-free.
BUG: KASAN: use-after-free in refcount_dec_and_test+0x20/0xd0
Read of size 4 at addr ffffffc114f5bf18 by task kworker/u17:14/705
CPU: 4 PID: 705 Comm: kworker/u17:14 Tainted: G S W
4.14.234-00003-g1fb6d0bd49a4-dirty #28
Hardware name: Qualcomm Technologies, Inc. SM8150 V2 PM8150
Google Inc. MSM sm8150 Flame DVT (DT)
Workqueue: hci0 hci_rx_work
Call trace:
dump_backtrace+0x0/0x378
show_stack+0x20/0x2c
dump_stack+0x124/0x148
print_address_description+0x80/0x2e8
__kasan_report+0x168/0x188
kasan_report+0x10/0x18
__asan_load4+0x84/0x8c
refcount_dec_and_test+0x20/0xd0
l2cap_chan_put+0x48/0x12c
l2cap_recv_frame+0x4770/0x6550
l2cap_recv_acldata+0x44c/0x7a4
hci_acldata_packet+0x100/0x188
hci_rx_work+0x178/0x23c
process_one_work+0x35c/0x95c
worker_thread+0x4cc/0x960
kthread+0x1a8/0x1c4
ret_from_fork+0x10/0x18
Cc: stable@kernel.org
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
include/net/bluetooth/l2cap.h | 1 +
net/bluetooth/l2cap_core.c | 61 +++++++++++++++++++++++++++--------
2 files changed, 49 insertions(+), 13 deletions(-)
diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 3c4f550e5a8b..2f766e3437ce 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -847,6 +847,7 @@ enum {
};
void l2cap_chan_hold(struct l2cap_chan *c);
+struct l2cap_chan *l2cap_chan_hold_unless_zero(struct l2cap_chan *c);
void l2cap_chan_put(struct l2cap_chan *c);
static inline void l2cap_chan_lock(struct l2cap_chan *chan)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 09ecaf556de5..77c0aac14539 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -111,7 +111,8 @@ static struct l2cap_chan *__l2cap_get_chan_by_scid(struct l2cap_conn *conn,
}
/* Find channel with given SCID.
- * Returns locked channel. */
+ * Returns a reference locked channel.
+ */
static struct l2cap_chan *l2cap_get_chan_by_scid(struct l2cap_conn *conn,
u16 cid)
{
@@ -119,15 +120,19 @@ static struct l2cap_chan *l2cap_get_chan_by_scid(struct l2cap_conn *conn,
mutex_lock(&conn->chan_lock);
c = __l2cap_get_chan_by_scid(conn, cid);
- if (c)
- l2cap_chan_lock(c);
+ if (c) {
+ /* Only lock if chan reference is not 0 */
+ c = l2cap_chan_hold_unless_zero(c);
+ if (c)
+ l2cap_chan_lock(c);
+ }
mutex_unlock(&conn->chan_lock);
return c;
}
/* Find channel with given DCID.
- * Returns locked channel.
+ * Returns a reference locked channel.
*/
static struct l2cap_chan *l2cap_get_chan_by_dcid(struct l2cap_conn *conn,
u16 cid)
@@ -136,8 +141,12 @@ static struct l2cap_chan *l2cap_get_chan_by_dcid(struct l2cap_conn *conn,
mutex_lock(&conn->chan_lock);
c = __l2cap_get_chan_by_dcid(conn, cid);
- if (c)
- l2cap_chan_lock(c);
+ if (c) {
+ /* Only lock if chan reference is not 0 */
+ c = l2cap_chan_hold_unless_zero(c);
+ if (c)
+ l2cap_chan_lock(c);
+ }
mutex_unlock(&conn->chan_lock);
return c;
@@ -162,8 +171,12 @@ static struct l2cap_chan *l2cap_get_chan_by_ident(struct l2cap_conn *conn,
mutex_lock(&conn->chan_lock);
c = __l2cap_get_chan_by_ident(conn, ident);
- if (c)
- l2cap_chan_lock(c);
+ if (c) {
+ /* Only lock if chan reference is not 0 */
+ c = l2cap_chan_hold_unless_zero(c);
+ if (c)
+ l2cap_chan_lock(c);
+ }
mutex_unlock(&conn->chan_lock);
return c;
@@ -497,6 +510,16 @@ void l2cap_chan_hold(struct l2cap_chan *c)
kref_get(&c->kref);
}
+struct l2cap_chan *l2cap_chan_hold_unless_zero(struct l2cap_chan *c)
+{
+ BT_DBG("chan %p orig refcnt %u", c, kref_read(&c->kref));
+
+ if (!kref_get_unless_zero(&c->kref))
+ return NULL;
+
+ return c;
+}
+
void l2cap_chan_put(struct l2cap_chan *c)
{
BT_DBG("chan %p orig refcnt %u", c, kref_read(&c->kref));
@@ -1969,7 +1992,10 @@ static struct l2cap_chan *l2cap_global_chan_by_psm(int state, __le16 psm,
src_match = !bacmp(&c->src, src);
dst_match = !bacmp(&c->dst, dst);
if (src_match && dst_match) {
- l2cap_chan_hold(c);
+ c = l2cap_chan_hold_unless_zero(c);
+ if (!c)
+ continue;
+
read_unlock(&chan_list_lock);
return c;
}
@@ -1984,7 +2010,7 @@ static struct l2cap_chan *l2cap_global_chan_by_psm(int state, __le16 psm,
}
if (c1)
- l2cap_chan_hold(c1);
+ c1 = l2cap_chan_hold_unless_zero(c1);
read_unlock(&chan_list_lock);
@@ -4464,6 +4490,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn,
unlock:
l2cap_chan_unlock(chan);
+ l2cap_chan_put(chan);
return err;
}
@@ -4578,6 +4605,7 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn,
done:
l2cap_chan_unlock(chan);
+ l2cap_chan_put(chan);
return err;
}
@@ -5305,6 +5333,7 @@ static inline int l2cap_move_channel_req(struct l2cap_conn *conn,
l2cap_send_move_chan_rsp(chan, result);
l2cap_chan_unlock(chan);
+ l2cap_chan_put(chan);
return 0;
}
@@ -5397,6 +5426,7 @@ static void l2cap_move_continue(struct l2cap_conn *conn, u16 icid, u16 result)
}
l2cap_chan_unlock(chan);
+ l2cap_chan_put(chan);
}
static void l2cap_move_fail(struct l2cap_conn *conn, u8 ident, u16 icid,
@@ -5426,6 +5456,7 @@ static void l2cap_move_fail(struct l2cap_conn *conn, u8 ident, u16 icid,
l2cap_send_move_chan_cfm(chan, L2CAP_MC_UNCONFIRMED);
l2cap_chan_unlock(chan);
+ l2cap_chan_put(chan);
}
static int l2cap_move_channel_rsp(struct l2cap_conn *conn,
@@ -5489,6 +5520,7 @@ static int l2cap_move_channel_confirm(struct l2cap_conn *conn,
l2cap_send_move_chan_cfm_rsp(conn, cmd->ident, icid);
l2cap_chan_unlock(chan);
+ l2cap_chan_put(chan);
return 0;
}
@@ -5524,6 +5556,7 @@ static inline int l2cap_move_channel_confirm_rsp(struct l2cap_conn *conn,
}
l2cap_chan_unlock(chan);
+ l2cap_chan_put(chan);
return 0;
}
@@ -5896,12 +5929,11 @@ static inline int l2cap_le_credits(struct l2cap_conn *conn,
if (credits > max_credits) {
BT_ERR("LE credits overflow");
l2cap_send_disconn_req(chan, ECONNRESET);
- l2cap_chan_unlock(chan);
/* Return 0 so that we don't trigger an unnecessary
* command reject packet.
*/
- return 0;
+ goto unlock;
}
chan->tx_credits += credits;
@@ -5912,7 +5944,9 @@ static inline int l2cap_le_credits(struct l2cap_conn *conn,
if (chan->tx_credits)
chan->ops->resume(chan);
+unlock:
l2cap_chan_unlock(chan);
+ l2cap_chan_put(chan);
return 0;
}
@@ -7598,6 +7632,7 @@ static void l2cap_data_channel(struct l2cap_conn *conn, u16 cid,
done:
l2cap_chan_unlock(chan);
+ l2cap_chan_put(chan);
}
static void l2cap_conless_channel(struct l2cap_conn *conn, __le16 psm,
@@ -8086,7 +8121,7 @@ static struct l2cap_chan *l2cap_global_fixed_chan(struct l2cap_chan *c,
if (src_type != c->src_type)
continue;
- l2cap_chan_hold(c);
+ c = l2cap_chan_hold_unless_zero(c);
read_unlock(&chan_list_lock);
return c;
}
--
2.36.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* RE: Bluetooth: L2CAP: Fix use-after-free caused by l2cap_chan_put
2022-07-20 17:06 [PATCH] Bluetooth: L2CAP: Fix use-after-free caused by l2cap_chan_put Luiz Augusto von Dentz
@ 2022-07-20 18:13 ` bluez.test.bot
2022-07-21 7:43 ` [PATCH] " Lee Jones
1 sibling, 0 replies; 4+ messages in thread
From: bluez.test.bot @ 2022-07-20 18:13 UTC (permalink / raw)
To: linux-bluetooth, luiz.dentz
[-- Attachment #1: Type: text/plain, Size: 1100 bytes --]
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=661544
---Test result---
Test Summary:
CheckPatch PASS 1.49 seconds
GitLint PASS 0.49 seconds
SubjectPrefix PASS 0.31 seconds
BuildKernel PASS 45.10 seconds
BuildKernel32 PASS 38.60 seconds
Incremental Build with patchesPASS 54.60 seconds
TestRunner: Setup PASS 661.14 seconds
TestRunner: l2cap-tester PASS 19.94 seconds
TestRunner: bnep-tester PASS 7.38 seconds
TestRunner: mgmt-tester PASS 122.17 seconds
TestRunner: rfcomm-tester PASS 11.24 seconds
TestRunner: sco-tester PASS 11.31 seconds
TestRunner: smp-tester PASS 11.12 seconds
TestRunner: userchan-tester PASS 7.22 seconds
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Bluetooth: L2CAP: Fix use-after-free caused by l2cap_chan_put
2022-07-20 17:06 [PATCH] Bluetooth: L2CAP: Fix use-after-free caused by l2cap_chan_put Luiz Augusto von Dentz
2022-07-20 18:13 ` bluez.test.bot
@ 2022-07-21 7:43 ` Lee Jones
2022-07-21 16:11 ` Luiz Augusto von Dentz
1 sibling, 1 reply; 4+ messages in thread
From: Lee Jones @ 2022-07-21 7:43 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
On Wed, 20 Jul 2022, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> This fixes the following trace which is caused by hci_rx_work starting up
> *after* the final channel reference has been put() during sock_close() but
> *before* the references to the channel have been destroyed, so instead
> the code now rely on kref_get_unless_zero/l2cap_chan_hold_unless_zero to
> prevent referencing a channel that is about to be destroyed.
>
> refcount_t: increment on 0; use-after-free.
> BUG: KASAN: use-after-free in refcount_dec_and_test+0x20/0xd0
> Read of size 4 at addr ffffffc114f5bf18 by task kworker/u17:14/705
>
> CPU: 4 PID: 705 Comm: kworker/u17:14 Tainted: G S W
> 4.14.234-00003-g1fb6d0bd49a4-dirty #28
> Hardware name: Qualcomm Technologies, Inc. SM8150 V2 PM8150
> Google Inc. MSM sm8150 Flame DVT (DT)
> Workqueue: hci0 hci_rx_work
> Call trace:
> dump_backtrace+0x0/0x378
> show_stack+0x20/0x2c
> dump_stack+0x124/0x148
> print_address_description+0x80/0x2e8
> __kasan_report+0x168/0x188
> kasan_report+0x10/0x18
> __asan_load4+0x84/0x8c
> refcount_dec_and_test+0x20/0xd0
> l2cap_chan_put+0x48/0x12c
> l2cap_recv_frame+0x4770/0x6550
> l2cap_recv_acldata+0x44c/0x7a4
> hci_acldata_packet+0x100/0x188
> hci_rx_work+0x178/0x23c
> process_one_work+0x35c/0x95c
> worker_thread+0x4cc/0x960
> kthread+0x1a8/0x1c4
> ret_from_fork+0x10/0x18
>
> Cc: stable@kernel.org
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by isn't the correct tag for this since I wasn't
responsible for any of the coding, nor am I in the merge path.
Either as a v2 or when the patch is applied, please swap out for:
Reported-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
Tested-by: Lee Jones <lee.jones@linaro.org>
> ---
> include/net/bluetooth/l2cap.h | 1 +
> net/bluetooth/l2cap_core.c | 61 +++++++++++++++++++++++++++--------
> 2 files changed, 49 insertions(+), 13 deletions(-)
>
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index 3c4f550e5a8b..2f766e3437ce 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -847,6 +847,7 @@ enum {
> };
>
> void l2cap_chan_hold(struct l2cap_chan *c);
> +struct l2cap_chan *l2cap_chan_hold_unless_zero(struct l2cap_chan *c);
> void l2cap_chan_put(struct l2cap_chan *c);
>
> static inline void l2cap_chan_lock(struct l2cap_chan *chan)
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 09ecaf556de5..77c0aac14539 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -111,7 +111,8 @@ static struct l2cap_chan *__l2cap_get_chan_by_scid(struct l2cap_conn *conn,
> }
>
> /* Find channel with given SCID.
> - * Returns locked channel. */
> + * Returns a reference locked channel.
> + */
> static struct l2cap_chan *l2cap_get_chan_by_scid(struct l2cap_conn *conn,
> u16 cid)
> {
> @@ -119,15 +120,19 @@ static struct l2cap_chan *l2cap_get_chan_by_scid(struct l2cap_conn *conn,
>
> mutex_lock(&conn->chan_lock);
> c = __l2cap_get_chan_by_scid(conn, cid);
> - if (c)
> - l2cap_chan_lock(c);
> + if (c) {
> + /* Only lock if chan reference is not 0 */
> + c = l2cap_chan_hold_unless_zero(c);
> + if (c)
> + l2cap_chan_lock(c);
> + }
> mutex_unlock(&conn->chan_lock);
>
> return c;
> }
>
> /* Find channel with given DCID.
> - * Returns locked channel.
> + * Returns a reference locked channel.
> */
> static struct l2cap_chan *l2cap_get_chan_by_dcid(struct l2cap_conn *conn,
> u16 cid)
> @@ -136,8 +141,12 @@ static struct l2cap_chan *l2cap_get_chan_by_dcid(struct l2cap_conn *conn,
>
> mutex_lock(&conn->chan_lock);
> c = __l2cap_get_chan_by_dcid(conn, cid);
> - if (c)
> - l2cap_chan_lock(c);
> + if (c) {
> + /* Only lock if chan reference is not 0 */
> + c = l2cap_chan_hold_unless_zero(c);
> + if (c)
> + l2cap_chan_lock(c);
> + }
> mutex_unlock(&conn->chan_lock);
>
> return c;
> @@ -162,8 +171,12 @@ static struct l2cap_chan *l2cap_get_chan_by_ident(struct l2cap_conn *conn,
>
> mutex_lock(&conn->chan_lock);
> c = __l2cap_get_chan_by_ident(conn, ident);
> - if (c)
> - l2cap_chan_lock(c);
> + if (c) {
> + /* Only lock if chan reference is not 0 */
> + c = l2cap_chan_hold_unless_zero(c);
> + if (c)
> + l2cap_chan_lock(c);
> + }
> mutex_unlock(&conn->chan_lock);
>
> return c;
> @@ -497,6 +510,16 @@ void l2cap_chan_hold(struct l2cap_chan *c)
> kref_get(&c->kref);
> }
>
> +struct l2cap_chan *l2cap_chan_hold_unless_zero(struct l2cap_chan *c)
> +{
> + BT_DBG("chan %p orig refcnt %u", c, kref_read(&c->kref));
> +
> + if (!kref_get_unless_zero(&c->kref))
> + return NULL;
> +
> + return c;
> +}
> +
> void l2cap_chan_put(struct l2cap_chan *c)
> {
> BT_DBG("chan %p orig refcnt %u", c, kref_read(&c->kref));
> @@ -1969,7 +1992,10 @@ static struct l2cap_chan *l2cap_global_chan_by_psm(int state, __le16 psm,
> src_match = !bacmp(&c->src, src);
> dst_match = !bacmp(&c->dst, dst);
> if (src_match && dst_match) {
> - l2cap_chan_hold(c);
> + c = l2cap_chan_hold_unless_zero(c);
> + if (!c)
> + continue;
> +
> read_unlock(&chan_list_lock);
> return c;
> }
> @@ -1984,7 +2010,7 @@ static struct l2cap_chan *l2cap_global_chan_by_psm(int state, __le16 psm,
> }
>
> if (c1)
> - l2cap_chan_hold(c1);
> + c1 = l2cap_chan_hold_unless_zero(c1);
>
> read_unlock(&chan_list_lock);
>
> @@ -4464,6 +4490,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn,
>
> unlock:
> l2cap_chan_unlock(chan);
> + l2cap_chan_put(chan);
> return err;
> }
>
> @@ -4578,6 +4605,7 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn,
>
> done:
> l2cap_chan_unlock(chan);
> + l2cap_chan_put(chan);
> return err;
> }
>
> @@ -5305,6 +5333,7 @@ static inline int l2cap_move_channel_req(struct l2cap_conn *conn,
> l2cap_send_move_chan_rsp(chan, result);
>
> l2cap_chan_unlock(chan);
> + l2cap_chan_put(chan);
>
> return 0;
> }
> @@ -5397,6 +5426,7 @@ static void l2cap_move_continue(struct l2cap_conn *conn, u16 icid, u16 result)
> }
>
> l2cap_chan_unlock(chan);
> + l2cap_chan_put(chan);
> }
>
> static void l2cap_move_fail(struct l2cap_conn *conn, u8 ident, u16 icid,
> @@ -5426,6 +5456,7 @@ static void l2cap_move_fail(struct l2cap_conn *conn, u8 ident, u16 icid,
> l2cap_send_move_chan_cfm(chan, L2CAP_MC_UNCONFIRMED);
>
> l2cap_chan_unlock(chan);
> + l2cap_chan_put(chan);
> }
>
> static int l2cap_move_channel_rsp(struct l2cap_conn *conn,
> @@ -5489,6 +5520,7 @@ static int l2cap_move_channel_confirm(struct l2cap_conn *conn,
> l2cap_send_move_chan_cfm_rsp(conn, cmd->ident, icid);
>
> l2cap_chan_unlock(chan);
> + l2cap_chan_put(chan);
>
> return 0;
> }
> @@ -5524,6 +5556,7 @@ static inline int l2cap_move_channel_confirm_rsp(struct l2cap_conn *conn,
> }
>
> l2cap_chan_unlock(chan);
> + l2cap_chan_put(chan);
>
> return 0;
> }
> @@ -5896,12 +5929,11 @@ static inline int l2cap_le_credits(struct l2cap_conn *conn,
> if (credits > max_credits) {
> BT_ERR("LE credits overflow");
> l2cap_send_disconn_req(chan, ECONNRESET);
> - l2cap_chan_unlock(chan);
>
> /* Return 0 so that we don't trigger an unnecessary
> * command reject packet.
> */
> - return 0;
> + goto unlock;
> }
>
> chan->tx_credits += credits;
> @@ -5912,7 +5944,9 @@ static inline int l2cap_le_credits(struct l2cap_conn *conn,
> if (chan->tx_credits)
> chan->ops->resume(chan);
>
> +unlock:
> l2cap_chan_unlock(chan);
> + l2cap_chan_put(chan);
>
> return 0;
> }
> @@ -7598,6 +7632,7 @@ static void l2cap_data_channel(struct l2cap_conn *conn, u16 cid,
>
> done:
> l2cap_chan_unlock(chan);
> + l2cap_chan_put(chan);
> }
>
> static void l2cap_conless_channel(struct l2cap_conn *conn, __le16 psm,
> @@ -8086,7 +8121,7 @@ static struct l2cap_chan *l2cap_global_fixed_chan(struct l2cap_chan *c,
> if (src_type != c->src_type)
> continue;
>
> - l2cap_chan_hold(c);
> + c = l2cap_chan_hold_unless_zero(c);
> read_unlock(&chan_list_lock);
> return c;
> }
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Bluetooth: L2CAP: Fix use-after-free caused by l2cap_chan_put
2022-07-21 7:43 ` [PATCH] " Lee Jones
@ 2022-07-21 16:11 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 4+ messages in thread
From: Luiz Augusto von Dentz @ 2022-07-21 16:11 UTC (permalink / raw)
To: Lee Jones; +Cc: linux-bluetooth@vger.kernel.org
Hi Lee,
On Thu, Jul 21, 2022 at 12:43 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Wed, 20 Jul 2022, Luiz Augusto von Dentz wrote:
>
> > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> >
> > This fixes the following trace which is caused by hci_rx_work starting up
> > *after* the final channel reference has been put() during sock_close() but
> > *before* the references to the channel have been destroyed, so instead
> > the code now rely on kref_get_unless_zero/l2cap_chan_hold_unless_zero to
> > prevent referencing a channel that is about to be destroyed.
> >
> > refcount_t: increment on 0; use-after-free.
> > BUG: KASAN: use-after-free in refcount_dec_and_test+0x20/0xd0
> > Read of size 4 at addr ffffffc114f5bf18 by task kworker/u17:14/705
> >
> > CPU: 4 PID: 705 Comm: kworker/u17:14 Tainted: G S W
> > 4.14.234-00003-g1fb6d0bd49a4-dirty #28
> > Hardware name: Qualcomm Technologies, Inc. SM8150 V2 PM8150
> > Google Inc. MSM sm8150 Flame DVT (DT)
> > Workqueue: hci0 hci_rx_work
> > Call trace:
> > dump_backtrace+0x0/0x378
> > show_stack+0x20/0x2c
> > dump_stack+0x124/0x148
> > print_address_description+0x80/0x2e8
> > __kasan_report+0x168/0x188
> > kasan_report+0x10/0x18
> > __asan_load4+0x84/0x8c
> > refcount_dec_and_test+0x20/0xd0
> > l2cap_chan_put+0x48/0x12c
> > l2cap_recv_frame+0x4770/0x6550
> > l2cap_recv_acldata+0x44c/0x7a4
> > hci_acldata_packet+0x100/0x188
> > hci_rx_work+0x178/0x23c
> > process_one_work+0x35c/0x95c
> > worker_thread+0x4cc/0x960
> > kthread+0x1a8/0x1c4
> > ret_from_fork+0x10/0x18
> >
> > Cc: stable@kernel.org
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
>
> Signed-off-by isn't the correct tag for this since I wasn't
> responsible for any of the coding, nor am I in the merge path.
>
> Either as a v2 or when the patch is applied, please swap out for:
>
> Reported-by: Lee Jones <lee.jones@linaro.org>
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> Tested-by: Lee Jones <lee.jones@linaro.org>
v2 sent.
> > ---
> > include/net/bluetooth/l2cap.h | 1 +
> > net/bluetooth/l2cap_core.c | 61 +++++++++++++++++++++++++++--------
> > 2 files changed, 49 insertions(+), 13 deletions(-)
> >
> > diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> > index 3c4f550e5a8b..2f766e3437ce 100644
> > --- a/include/net/bluetooth/l2cap.h
> > +++ b/include/net/bluetooth/l2cap.h
> > @@ -847,6 +847,7 @@ enum {
> > };
> >
> > void l2cap_chan_hold(struct l2cap_chan *c);
> > +struct l2cap_chan *l2cap_chan_hold_unless_zero(struct l2cap_chan *c);
> > void l2cap_chan_put(struct l2cap_chan *c);
> >
> > static inline void l2cap_chan_lock(struct l2cap_chan *chan)
> > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> > index 09ecaf556de5..77c0aac14539 100644
> > --- a/net/bluetooth/l2cap_core.c
> > +++ b/net/bluetooth/l2cap_core.c
> > @@ -111,7 +111,8 @@ static struct l2cap_chan *__l2cap_get_chan_by_scid(struct l2cap_conn *conn,
> > }
> >
> > /* Find channel with given SCID.
> > - * Returns locked channel. */
> > + * Returns a reference locked channel.
> > + */
> > static struct l2cap_chan *l2cap_get_chan_by_scid(struct l2cap_conn *conn,
> > u16 cid)
> > {
> > @@ -119,15 +120,19 @@ static struct l2cap_chan *l2cap_get_chan_by_scid(struct l2cap_conn *conn,
> >
> > mutex_lock(&conn->chan_lock);
> > c = __l2cap_get_chan_by_scid(conn, cid);
> > - if (c)
> > - l2cap_chan_lock(c);
> > + if (c) {
> > + /* Only lock if chan reference is not 0 */
> > + c = l2cap_chan_hold_unless_zero(c);
> > + if (c)
> > + l2cap_chan_lock(c);
> > + }
> > mutex_unlock(&conn->chan_lock);
> >
> > return c;
> > }
> >
> > /* Find channel with given DCID.
> > - * Returns locked channel.
> > + * Returns a reference locked channel.
> > */
> > static struct l2cap_chan *l2cap_get_chan_by_dcid(struct l2cap_conn *conn,
> > u16 cid)
> > @@ -136,8 +141,12 @@ static struct l2cap_chan *l2cap_get_chan_by_dcid(struct l2cap_conn *conn,
> >
> > mutex_lock(&conn->chan_lock);
> > c = __l2cap_get_chan_by_dcid(conn, cid);
> > - if (c)
> > - l2cap_chan_lock(c);
> > + if (c) {
> > + /* Only lock if chan reference is not 0 */
> > + c = l2cap_chan_hold_unless_zero(c);
> > + if (c)
> > + l2cap_chan_lock(c);
> > + }
> > mutex_unlock(&conn->chan_lock);
> >
> > return c;
> > @@ -162,8 +171,12 @@ static struct l2cap_chan *l2cap_get_chan_by_ident(struct l2cap_conn *conn,
> >
> > mutex_lock(&conn->chan_lock);
> > c = __l2cap_get_chan_by_ident(conn, ident);
> > - if (c)
> > - l2cap_chan_lock(c);
> > + if (c) {
> > + /* Only lock if chan reference is not 0 */
> > + c = l2cap_chan_hold_unless_zero(c);
> > + if (c)
> > + l2cap_chan_lock(c);
> > + }
> > mutex_unlock(&conn->chan_lock);
> >
> > return c;
> > @@ -497,6 +510,16 @@ void l2cap_chan_hold(struct l2cap_chan *c)
> > kref_get(&c->kref);
> > }
> >
> > +struct l2cap_chan *l2cap_chan_hold_unless_zero(struct l2cap_chan *c)
> > +{
> > + BT_DBG("chan %p orig refcnt %u", c, kref_read(&c->kref));
> > +
> > + if (!kref_get_unless_zero(&c->kref))
> > + return NULL;
> > +
> > + return c;
> > +}
> > +
> > void l2cap_chan_put(struct l2cap_chan *c)
> > {
> > BT_DBG("chan %p orig refcnt %u", c, kref_read(&c->kref));
> > @@ -1969,7 +1992,10 @@ static struct l2cap_chan *l2cap_global_chan_by_psm(int state, __le16 psm,
> > src_match = !bacmp(&c->src, src);
> > dst_match = !bacmp(&c->dst, dst);
> > if (src_match && dst_match) {
> > - l2cap_chan_hold(c);
> > + c = l2cap_chan_hold_unless_zero(c);
> > + if (!c)
> > + continue;
> > +
> > read_unlock(&chan_list_lock);
> > return c;
> > }
> > @@ -1984,7 +2010,7 @@ static struct l2cap_chan *l2cap_global_chan_by_psm(int state, __le16 psm,
> > }
> >
> > if (c1)
> > - l2cap_chan_hold(c1);
> > + c1 = l2cap_chan_hold_unless_zero(c1);
> >
> > read_unlock(&chan_list_lock);
> >
> > @@ -4464,6 +4490,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn,
> >
> > unlock:
> > l2cap_chan_unlock(chan);
> > + l2cap_chan_put(chan);
> > return err;
> > }
> >
> > @@ -4578,6 +4605,7 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn,
> >
> > done:
> > l2cap_chan_unlock(chan);
> > + l2cap_chan_put(chan);
> > return err;
> > }
> >
> > @@ -5305,6 +5333,7 @@ static inline int l2cap_move_channel_req(struct l2cap_conn *conn,
> > l2cap_send_move_chan_rsp(chan, result);
> >
> > l2cap_chan_unlock(chan);
> > + l2cap_chan_put(chan);
> >
> > return 0;
> > }
> > @@ -5397,6 +5426,7 @@ static void l2cap_move_continue(struct l2cap_conn *conn, u16 icid, u16 result)
> > }
> >
> > l2cap_chan_unlock(chan);
> > + l2cap_chan_put(chan);
> > }
> >
> > static void l2cap_move_fail(struct l2cap_conn *conn, u8 ident, u16 icid,
> > @@ -5426,6 +5456,7 @@ static void l2cap_move_fail(struct l2cap_conn *conn, u8 ident, u16 icid,
> > l2cap_send_move_chan_cfm(chan, L2CAP_MC_UNCONFIRMED);
> >
> > l2cap_chan_unlock(chan);
> > + l2cap_chan_put(chan);
> > }
> >
> > static int l2cap_move_channel_rsp(struct l2cap_conn *conn,
> > @@ -5489,6 +5520,7 @@ static int l2cap_move_channel_confirm(struct l2cap_conn *conn,
> > l2cap_send_move_chan_cfm_rsp(conn, cmd->ident, icid);
> >
> > l2cap_chan_unlock(chan);
> > + l2cap_chan_put(chan);
> >
> > return 0;
> > }
> > @@ -5524,6 +5556,7 @@ static inline int l2cap_move_channel_confirm_rsp(struct l2cap_conn *conn,
> > }
> >
> > l2cap_chan_unlock(chan);
> > + l2cap_chan_put(chan);
> >
> > return 0;
> > }
> > @@ -5896,12 +5929,11 @@ static inline int l2cap_le_credits(struct l2cap_conn *conn,
> > if (credits > max_credits) {
> > BT_ERR("LE credits overflow");
> > l2cap_send_disconn_req(chan, ECONNRESET);
> > - l2cap_chan_unlock(chan);
> >
> > /* Return 0 so that we don't trigger an unnecessary
> > * command reject packet.
> > */
> > - return 0;
> > + goto unlock;
> > }
> >
> > chan->tx_credits += credits;
> > @@ -5912,7 +5944,9 @@ static inline int l2cap_le_credits(struct l2cap_conn *conn,
> > if (chan->tx_credits)
> > chan->ops->resume(chan);
> >
> > +unlock:
> > l2cap_chan_unlock(chan);
> > + l2cap_chan_put(chan);
> >
> > return 0;
> > }
> > @@ -7598,6 +7632,7 @@ static void l2cap_data_channel(struct l2cap_conn *conn, u16 cid,
> >
> > done:
> > l2cap_chan_unlock(chan);
> > + l2cap_chan_put(chan);
> > }
> >
> > static void l2cap_conless_channel(struct l2cap_conn *conn, __le16 psm,
> > @@ -8086,7 +8121,7 @@ static struct l2cap_chan *l2cap_global_fixed_chan(struct l2cap_chan *c,
> > if (src_type != c->src_type)
> > continue;
> >
> > - l2cap_chan_hold(c);
> > + c = l2cap_chan_hold_unless_zero(c);
> > read_unlock(&chan_list_lock);
> > return c;
> > }
>
> --
> Lee Jones [李琼斯]
> Principal Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-07-21 16:11 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-07-20 17:06 [PATCH] Bluetooth: L2CAP: Fix use-after-free caused by l2cap_chan_put Luiz Augusto von Dentz
2022-07-20 18:13 ` bluez.test.bot
2022-07-21 7:43 ` [PATCH] " Lee Jones
2022-07-21 16:11 ` Luiz Augusto von Dentz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.