Fix the race condition between the following two flows that run in
parallel:
1. l2cap_reassemble_sdu -> chan->ops->recv -> l2cap_sock_recv_cb ->
__sock_queue_rcv_skb.
2. bt_sock_recvmsg -> skb_recv_datagram, skb_free_datagram.
An SKB can be queued by the first flow and immediately dequeued and
freed by the second flow, therefore the callers of l2cap_reassemble_sdu
can't use the SKB after that function returns. However, some places
continue accessing struct l2cap_ctrl that resides in the SKB's CB for a
short time after l2cap_reassemble_sdu returns, leading to a
use-after-free condition (the stack trace is below, line numbers for
kernel 5.19.8).
Fix it by keeping a local copy of struct l2cap_ctrl.
BUG: KASAN: use-after-free in l2cap_rx_state_recv (net/bluetooth/l2cap_core.c:6906) bluetooth
Read of size 1 at addr ffff88812025f2f0 by task kworker/u17:3/43169
Workqueue: hci0 hci_rx_work [bluetooth]
Call Trace:
<TASK>
dump_stack_lvl (lib/dump_stack.c:107 (discriminator 4))
print_report.cold (mm/kasan/report.c:314 mm/kasan/report.c:429)
? l2cap_rx_state_recv (net/bluetooth/l2cap_core.c:6906) bluetooth
kasan_report (mm/kasan/report.c:162 mm/kasan/report.c:493)
? l2cap_rx_state_recv (net/bluetooth/l2cap_core.c:6906) bluetooth
l2cap_rx_state_recv (net/bluetooth/l2cap_core.c:6906) bluetooth
l2cap_rx (net/bluetooth/l2cap_core.c:7236 net/bluetooth/l2cap_core.c:7271) bluetooth
? sk_filter_trim_cap (net/core/filter.c:123)
? l2cap_chan_hold_unless_zero (./arch/x86/include/asm/atomic.h:202 ./include/linux/atomic/atomic-instrumented.h:560 ./include/linux/refcount.h:157 ./include/linux/refcount.h:227 ./include/linux/refcount.h:245 ./include/linux/kref.h:111 net/bluetooth/l2cap_core.c:517) bluetooth
? l2cap_rx_state_recv (net/bluetooth/l2cap_core.c:7252) bluetooth
l2cap_recv_frame (net/bluetooth/l2cap_core.c:7405 net/bluetooth/l2cap_core.c:7620 net/bluetooth/l2cap_core.c:7723) bluetooth
? lock_release (./include/trace/events/lock.h:69 kernel/locking/lockdep.c:5677)
? hci_rx_work (net/bluetooth/hci_core.c:3637 net/bluetooth/hci_core.c:3832) bluetooth
? reacquire_held_locks (kernel/locking/lockdep.c:5674)
? trace_contention_end (./include/trace/events/lock.h:122 ./include/trace/events/lock.h:122)
? l2cap_config_rsp.isra.0 (net/bluetooth/l2cap_core.c:7674) bluetooth
? hci_rx_work (./include/net/bluetooth/hci_core.h:1024 net/bluetooth/hci_core.c:3634 net/bluetooth/hci_core.c:3832) bluetooth
? lock_acquire (./include/trace/events/lock.h:24 kernel/locking/lockdep.c:5637)
? __mutex_unlock_slowpath (./arch/x86/include/asm/atomic64_64.h:190 ./include/linux/atomic/atomic-long.h:449 ./include/linux/atomic/atomic-instrumented.h:1790 kernel/locking/mutex.c:924)
? rcu_read_lock_sched_held (kernel/rcu/update.c:104 kernel/rcu/update.c:123)
? memcpy (mm/kasan/shadow.c:65 (discriminator 1))
? l2cap_recv_frag (net/bluetooth/l2cap_core.c:8340) bluetooth
l2cap_recv_acldata (net/bluetooth/l2cap_core.c:8486) bluetooth
hci_rx_work (net/bluetooth/hci_core.c:3642 net/bluetooth/hci_core.c:3832) bluetooth
process_one_work (kernel/workqueue.c:2289)
? lock_downgrade (kernel/locking/lockdep.c:5634)
? pwq_dec_nr_in_flight (kernel/workqueue.c:2184)
? rwlock_bug.part.0 (kernel/locking/spinlock_debug.c:113)
worker_thread (./include/linux/list.h:292 kernel/workqueue.c:2437)
? __kthread_parkme (./arch/x86/include/asm/bitops.h:207 (discriminator 4) ./include/asm-generic/bitops/instrumented-non-atomic.h:135 (discriminator 4) kernel/kthread.c:270 (discriminator 4))
? process_one_work (kernel/workqueue.c:2379)
kthread (kernel/kthread.c:376)
? kthread_complete_and_exit (kernel/kthread.c:331)
ret_from_fork (arch/x86/entry/entry_64.S:306)
</TASK>
Allocated by task 43169:
kasan_save_stack (mm/kasan/common.c:39)
__kasan_slab_alloc (mm/kasan/common.c:45 mm/kasan/common.c:436 mm/kasan/common.c:469)
kmem_cache_alloc_node (mm/slab.h:750 mm/slub.c:3243 mm/slub.c:3293)
__alloc_skb (net/core/skbuff.c:414)
l2cap_recv_frag (./include/net/bluetooth/bluetooth.h:425 net/bluetooth/l2cap_core.c:8329) bluetooth
l2cap_recv_acldata (net/bluetooth/l2cap_core.c:8442) bluetooth
hci_rx_work (net/bluetooth/hci_core.c:3642 net/bluetooth/hci_core.c:3832) bluetooth
process_one_work (kernel/workqueue.c:2289)
worker_thread (./include/linux/list.h:292 kernel/workqueue.c:2437)
kthread (kernel/kthread.c:376)
ret_from_fork (arch/x86/entry/entry_64.S:306)
Freed by task 27920:
kasan_save_stack (mm/kasan/common.c:39)
kasan_set_track (mm/kasan/common.c:45)
kasan_set_free_info (mm/kasan/generic.c:372)
____kasan_slab_free (mm/kasan/common.c:368 mm/kasan/common.c:328)
slab_free_freelist_hook (mm/slub.c:1780)
kmem_cache_free (mm/slub.c:3536 mm/slub.c:3553)
skb_free_datagram (./include/net/sock.h:1578 ./include/net/sock.h:1639 net/core/datagram.c:323)
bt_sock_recvmsg (net/bluetooth/af_bluetooth.c:295) bluetooth
l2cap_sock_recvmsg (net/bluetooth/l2cap_sock.c:1212) bluetooth
sock_read_iter (net/socket.c:1087)
new_sync_read (./include/linux/fs.h:2052 fs/read_write.c:401)
vfs_read (fs/read_write.c:482)
ksys_read (fs/read_write.c:620)
do_syscall_64 (arch/x86/entry/common.c:50 arch/x86/entry/common.c:80)
entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:120)
Link: https://lore.kernel.org/linux-bluetooth/CAKErNvoqga1WcmoR3-0875esY6TVWFQDandbVZncSiuGPBQXLA@mail.gmail.com/T/#u
Fixes: d2a7ac5d5d3a ("Bluetooth: Add the ERTM receive state machine")
Fixes: 4b51dae96731 ("Bluetooth: Add streaming mode receive and incoming packet classifier")
Signed-off-by: Maxim Mikityanskiy <[email protected]>
---
net/bluetooth/l2cap_core.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 2c9de67daadc..6bdce147d2fe 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -6876,6 +6876,7 @@ static int l2cap_rx_state_recv(struct l2cap_chan *chan,
struct l2cap_ctrl *control,
struct sk_buff *skb, u8 event)
{
+ struct l2cap_ctrl local_control;
int err = 0;
bool skb_in_use = false;
@@ -6900,15 +6901,16 @@ static int l2cap_rx_state_recv(struct l2cap_chan *chan,
chan->buffer_seq = chan->expected_tx_seq;
skb_in_use = true;
+ local_control = *control;
err = l2cap_reassemble_sdu(chan, skb, control);
if (err)
break;
- if (control->final) {
+ if (local_control.final) {
if (!test_and_clear_bit(CONN_REJ_ACT,
&chan->conn_state)) {
- control->final = 0;
- l2cap_retransmit_all(chan, control);
+ local_control.final = 0;
+ l2cap_retransmit_all(chan, &local_control);
l2cap_ertm_send(chan);
}
}
@@ -7288,11 +7290,12 @@ static int l2cap_rx(struct l2cap_chan *chan, struct l2cap_ctrl *control,
static int l2cap_stream_rx(struct l2cap_chan *chan, struct l2cap_ctrl *control,
struct sk_buff *skb)
{
+ u16 txseq = control->txseq;
+
BT_DBG("chan %p, control %p, skb %p, state %d", chan, control, skb,
chan->rx_state);
- if (l2cap_classify_txseq(chan, control->txseq) ==
- L2CAP_TXSEQ_EXPECTED) {
+ if (l2cap_classify_txseq(chan, txseq) == L2CAP_TXSEQ_EXPECTED) {
l2cap_pass_to_tx(chan, control);
BT_DBG("buffer_seq %u->%u", chan->buffer_seq,
@@ -7315,8 +7318,8 @@ static int l2cap_stream_rx(struct l2cap_chan *chan, struct l2cap_ctrl *control,
}
}
- chan->last_acked_seq = control->txseq;
- chan->expected_tx_seq = __next_seq(chan, control->txseq);
+ chan->last_acked_seq = txseq;
+ chan->expected_tx_seq = __next_seq(chan, txseq);
return 0;
}
--
2.37.3
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=682811
---Test result---
Test Summary:
CheckPatch FAIL 1.43 seconds
GitLint FAIL 0.48 seconds
SubjectPrefix PASS 0.31 seconds
BuildKernel PASS 44.58 seconds
BuildKernel32 PASS 40.50 seconds
Incremental Build with patchesPASS 59.79 seconds
TestRunner: Setup PASS 678.57 seconds
TestRunner: l2cap-tester PASS 21.22 seconds
TestRunner: iso-tester PASS 21.65 seconds
TestRunner: bnep-tester PASS 8.40 seconds
TestRunner: mgmt-tester PASS 132.37 seconds
TestRunner: rfcomm-tester PASS 12.71 seconds
TestRunner: sco-tester PASS 12.05 seconds
TestRunner: ioctl-tester PASS 13.91 seconds
TestRunner: smp-tester PASS 11.98 seconds
TestRunner: userchan-tester PASS 8.60 seconds
Details
##############################
Test: CheckPatch - FAIL - 1.43 seconds
Run checkpatch.pl script with rule in .checkpatch.conf
[v2] Bluetooth: L2CAP: Fix use-after-free caused by l2cap_reassemble_sdu\WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#115:
l2cap_rx (net/bluetooth/l2cap_core.c:7236 net/bluetooth/l2cap_core.c:7271) bluetooth
total: 0 errors, 1 warnings, 0 checks, 50 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
/github/workspace/src/12997694.patch has style problems, please review.
NOTE: Ignored message types: UNKNOWN_COMMIT_ID
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
##############################
Test: GitLint - FAIL - 0.48 seconds
Run gitlint with rule in .gitlint
[v2] Bluetooth: L2CAP: Fix use-after-free caused by l2cap_reassemble_sdu
21: B1 Line exceeds max length (93>80): "BUG: KASAN: use-after-free in l2cap_rx_state_recv (net/bluetooth/l2cap_core.c:6906) bluetooth"
33: B1 Line exceeds max length (85>80): " l2cap_rx (net/bluetooth/l2cap_core.c:7236 net/bluetooth/l2cap_core.c:7271) bluetooth"
35: B1 Line exceeds max length (278>80): " ? l2cap_chan_hold_unless_zero (./arch/x86/include/asm/atomic.h:202 ./include/linux/atomic/atomic-instrumented.h:560 ./include/linux/refcount.h:157 ./include/linux/refcount.h:227 ./include/linux/refcount.h:245 ./include/linux/kref.h:111 net/bluetooth/l2cap_core.c:517) bluetooth"
37: B1 Line exceeds max length (125>80): " l2cap_recv_frame (net/bluetooth/l2cap_core.c:7405 net/bluetooth/l2cap_core.c:7620 net/bluetooth/l2cap_core.c:7723) bluetooth"
39: B1 Line exceeds max length (86>80): " ? hci_rx_work (net/bluetooth/hci_core.c:3637 net/bluetooth/hci_core.c:3832) bluetooth"
41: B1 Line exceeds max length (93>80): " ? trace_contention_end (./include/trace/events/lock.h:122 ./include/trace/events/lock.h:122)"
43: B1 Line exceeds max length (126>80): " ? hci_rx_work (./include/net/bluetooth/hci_core.h:1024 net/bluetooth/hci_core.c:3634 net/bluetooth/hci_core.c:3832) bluetooth"
45: B1 Line exceeds max length (187>80): " ? __mutex_unlock_slowpath (./arch/x86/include/asm/atomic64_64.h:190 ./include/linux/atomic/atomic-long.h:449 ./include/linux/atomic/atomic-instrumented.h:1790 kernel/locking/mutex.c:924)"
50: B1 Line exceeds max length (84>80): " hci_rx_work (net/bluetooth/hci_core.c:3642 net/bluetooth/hci_core.c:3832) bluetooth"
56: B1 Line exceeds max length (191>80): " ? __kthread_parkme (./arch/x86/include/asm/bitops.h:207 (discriminator 4) ./include/asm-generic/bitops/instrumented-non-atomic.h:135 (discriminator 4) kernel/kthread.c:270 (discriminator 4))"
65: B1 Line exceeds max length (86>80): " __kasan_slab_alloc (mm/kasan/common.c:45 mm/kasan/common.c:436 mm/kasan/common.c:469)"
68: B1 Line exceeds max length (100>80): " l2cap_recv_frag (./include/net/bluetooth/bluetooth.h:425 net/bluetooth/l2cap_core.c:8329) bluetooth"
70: B1 Line exceeds max length (84>80): " hci_rx_work (net/bluetooth/hci_core.c:3642 net/bluetooth/hci_core.c:3832) bluetooth"
83: B1 Line exceeds max length (96>80): " skb_free_datagram (./include/net/sock.h:1578 ./include/net/sock.h:1639 net/core/datagram.c:323)"
93: B1 Line exceeds max length (117>80): "Link: https://lore.kernel.org/linux-bluetooth/CAKErNvoqga1WcmoR3-0875esY6TVWFQDandbVZncSiuGPBQXLA@mail.gmail.com/T/#u"
---
Regards,
Linux Bluetooth
Hi Maxim,
On Mon, Oct 3, 2022 at 10:25 AM Maxim Mikityanskiy <[email protected]> wrote:
>
> Fix the race condition between the following two flows that run in
> parallel:
>
> 1. l2cap_reassemble_sdu -> chan->ops->recv -> l2cap_sock_recv_cb ->
> __sock_queue_rcv_skb.
>
> 2. bt_sock_recvmsg -> skb_recv_datagram, skb_free_datagram.
>
> An SKB can be queued by the first flow and immediately dequeued and
> freed by the second flow, therefore the callers of l2cap_reassemble_sdu
> can't use the SKB after that function returns. However, some places
> continue accessing struct l2cap_ctrl that resides in the SKB's CB for a
> short time after l2cap_reassemble_sdu returns, leading to a
> use-after-free condition (the stack trace is below, line numbers for
> kernel 5.19.8).
>
> Fix it by keeping a local copy of struct l2cap_ctrl.
>
> BUG: KASAN: use-after-free in l2cap_rx_state_recv (net/bluetooth/l2cap_core.c:6906) bluetooth
> Read of size 1 at addr ffff88812025f2f0 by task kworker/u17:3/43169
>
> Workqueue: hci0 hci_rx_work [bluetooth]
> Call Trace:
> <TASK>
> dump_stack_lvl (lib/dump_stack.c:107 (discriminator 4))
> print_report.cold (mm/kasan/report.c:314 mm/kasan/report.c:429)
> ? l2cap_rx_state_recv (net/bluetooth/l2cap_core.c:6906) bluetooth
> kasan_report (mm/kasan/report.c:162 mm/kasan/report.c:493)
> ? l2cap_rx_state_recv (net/bluetooth/l2cap_core.c:6906) bluetooth
> l2cap_rx_state_recv (net/bluetooth/l2cap_core.c:6906) bluetooth
> l2cap_rx (net/bluetooth/l2cap_core.c:7236 net/bluetooth/l2cap_core.c:7271) bluetooth
> ? sk_filter_trim_cap (net/core/filter.c:123)
> ? l2cap_chan_hold_unless_zero (./arch/x86/include/asm/atomic.h:202 ./include/linux/atomic/atomic-instrumented.h:560 ./include/linux/refcount.h:157 ./include/linux/refcount.h:227 ./include/linux/refcount.h:245 ./include/linux/kref.h:111 net/bluetooth/l2cap_core.c:517) bluetooth
> ? l2cap_rx_state_recv (net/bluetooth/l2cap_core.c:7252) bluetooth
> l2cap_recv_frame (net/bluetooth/l2cap_core.c:7405 net/bluetooth/l2cap_core.c:7620 net/bluetooth/l2cap_core.c:7723) bluetooth
> ? lock_release (./include/trace/events/lock.h:69 kernel/locking/lockdep.c:5677)
> ? hci_rx_work (net/bluetooth/hci_core.c:3637 net/bluetooth/hci_core.c:3832) bluetooth
> ? reacquire_held_locks (kernel/locking/lockdep.c:5674)
> ? trace_contention_end (./include/trace/events/lock.h:122 ./include/trace/events/lock.h:122)
> ? l2cap_config_rsp.isra.0 (net/bluetooth/l2cap_core.c:7674) bluetooth
> ? hci_rx_work (./include/net/bluetooth/hci_core.h:1024 net/bluetooth/hci_core.c:3634 net/bluetooth/hci_core.c:3832) bluetooth
> ? lock_acquire (./include/trace/events/lock.h:24 kernel/locking/lockdep.c:5637)
> ? __mutex_unlock_slowpath (./arch/x86/include/asm/atomic64_64.h:190 ./include/linux/atomic/atomic-long.h:449 ./include/linux/atomic/atomic-instrumented.h:1790 kernel/locking/mutex.c:924)
> ? rcu_read_lock_sched_held (kernel/rcu/update.c:104 kernel/rcu/update.c:123)
> ? memcpy (mm/kasan/shadow.c:65 (discriminator 1))
> ? l2cap_recv_frag (net/bluetooth/l2cap_core.c:8340) bluetooth
> l2cap_recv_acldata (net/bluetooth/l2cap_core.c:8486) bluetooth
> hci_rx_work (net/bluetooth/hci_core.c:3642 net/bluetooth/hci_core.c:3832) bluetooth
> process_one_work (kernel/workqueue.c:2289)
> ? lock_downgrade (kernel/locking/lockdep.c:5634)
> ? pwq_dec_nr_in_flight (kernel/workqueue.c:2184)
> ? rwlock_bug.part.0 (kernel/locking/spinlock_debug.c:113)
> worker_thread (./include/linux/list.h:292 kernel/workqueue.c:2437)
> ? __kthread_parkme (./arch/x86/include/asm/bitops.h:207 (discriminator 4) ./include/asm-generic/bitops/instrumented-non-atomic.h:135 (discriminator 4) kernel/kthread.c:270 (discriminator 4))
> ? process_one_work (kernel/workqueue.c:2379)
> kthread (kernel/kthread.c:376)
> ? kthread_complete_and_exit (kernel/kthread.c:331)
> ret_from_fork (arch/x86/entry/entry_64.S:306)
> </TASK>
>
> Allocated by task 43169:
> kasan_save_stack (mm/kasan/common.c:39)
> __kasan_slab_alloc (mm/kasan/common.c:45 mm/kasan/common.c:436 mm/kasan/common.c:469)
> kmem_cache_alloc_node (mm/slab.h:750 mm/slub.c:3243 mm/slub.c:3293)
> __alloc_skb (net/core/skbuff.c:414)
> l2cap_recv_frag (./include/net/bluetooth/bluetooth.h:425 net/bluetooth/l2cap_core.c:8329) bluetooth
> l2cap_recv_acldata (net/bluetooth/l2cap_core.c:8442) bluetooth
> hci_rx_work (net/bluetooth/hci_core.c:3642 net/bluetooth/hci_core.c:3832) bluetooth
> process_one_work (kernel/workqueue.c:2289)
> worker_thread (./include/linux/list.h:292 kernel/workqueue.c:2437)
> kthread (kernel/kthread.c:376)
> ret_from_fork (arch/x86/entry/entry_64.S:306)
>
> Freed by task 27920:
> kasan_save_stack (mm/kasan/common.c:39)
> kasan_set_track (mm/kasan/common.c:45)
> kasan_set_free_info (mm/kasan/generic.c:372)
> ____kasan_slab_free (mm/kasan/common.c:368 mm/kasan/common.c:328)
> slab_free_freelist_hook (mm/slub.c:1780)
> kmem_cache_free (mm/slub.c:3536 mm/slub.c:3553)
> skb_free_datagram (./include/net/sock.h:1578 ./include/net/sock.h:1639 net/core/datagram.c:323)
> bt_sock_recvmsg (net/bluetooth/af_bluetooth.c:295) bluetooth
> l2cap_sock_recvmsg (net/bluetooth/l2cap_sock.c:1212) bluetooth
> sock_read_iter (net/socket.c:1087)
> new_sync_read (./include/linux/fs.h:2052 fs/read_write.c:401)
> vfs_read (fs/read_write.c:482)
> ksys_read (fs/read_write.c:620)
> do_syscall_64 (arch/x86/entry/common.c:50 arch/x86/entry/common.c:80)
> entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:120)
>
> Link: https://lore.kernel.org/linux-bluetooth/CAKErNvoqga1WcmoR3-0875esY6TVWFQDandbVZncSiuGPBQXLA@mail.gmail.com/T/#u
> Fixes: d2a7ac5d5d3a ("Bluetooth: Add the ERTM receive state machine")
> Fixes: 4b51dae96731 ("Bluetooth: Add streaming mode receive and incoming packet classifier")
> Signed-off-by: Maxim Mikityanskiy <[email protected]>
> ---
> net/bluetooth/l2cap_core.c | 17 ++++++++++-------
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 2c9de67daadc..6bdce147d2fe 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -6876,6 +6876,7 @@ static int l2cap_rx_state_recv(struct l2cap_chan *chan,
> struct l2cap_ctrl *control,
> struct sk_buff *skb, u8 event)
> {
> + struct l2cap_ctrl local_control;
> int err = 0;
> bool skb_in_use = false;
>
> @@ -6900,15 +6901,16 @@ static int l2cap_rx_state_recv(struct l2cap_chan *chan,
> chan->buffer_seq = chan->expected_tx_seq;
> skb_in_use = true;
>
> + local_control = *control;
Have a comment on why we are creating a local copy, btw should we pass
the copy to l2cap_reassemble_sdu?
> err = l2cap_reassemble_sdu(chan, skb, control);
> if (err)
> break;
>
> - if (control->final) {
> + if (local_control.final) {
> if (!test_and_clear_bit(CONN_REJ_ACT,
> &chan->conn_state)) {
> - control->final = 0;
> - l2cap_retransmit_all(chan, control);
> + local_control.final = 0;
> + l2cap_retransmit_all(chan, &local_control);
> l2cap_ertm_send(chan);
> }
> }
> @@ -7288,11 +7290,12 @@ static int l2cap_rx(struct l2cap_chan *chan, struct l2cap_ctrl *control,
> static int l2cap_stream_rx(struct l2cap_chan *chan, struct l2cap_ctrl *control,
> struct sk_buff *skb)
> {
> + u16 txseq = control->txseq;
> +
> BT_DBG("chan %p, control %p, skb %p, state %d", chan, control, skb,
> chan->rx_state);
>
> - if (l2cap_classify_txseq(chan, control->txseq) ==
> - L2CAP_TXSEQ_EXPECTED) {
> + if (l2cap_classify_txseq(chan, txseq) == L2CAP_TXSEQ_EXPECTED) {
> l2cap_pass_to_tx(chan, control);
>
> BT_DBG("buffer_seq %u->%u", chan->buffer_seq,
> @@ -7315,8 +7318,8 @@ static int l2cap_stream_rx(struct l2cap_chan *chan, struct l2cap_ctrl *control,
> }
> }
>
> - chan->last_acked_seq = control->txseq;
> - chan->expected_tx_seq = __next_seq(chan, control->txseq);
> + chan->last_acked_seq = txseq;
> + chan->expected_tx_seq = __next_seq(chan, txseq);
>
> return 0;
> }
> --
> 2.37.3
>
--
Luiz Augusto von Dentz
On Mon, Oct 3, 2022 at 10:13 PM Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi Maxim,
>
> On Mon, Oct 3, 2022 at 10:25 AM Maxim Mikityanskiy <[email protected]> wrote:
> >
> > Fix the race condition between the following two flows that run in
> > parallel:
> >
> > 1. l2cap_reassemble_sdu -> chan->ops->recv -> l2cap_sock_recv_cb ->
> > __sock_queue_rcv_skb.
> >
> > 2. bt_sock_recvmsg -> skb_recv_datagram, skb_free_datagram.
> >
> > An SKB can be queued by the first flow and immediately dequeued and
> > freed by the second flow, therefore the callers of l2cap_reassemble_sdu
> > can't use the SKB after that function returns. However, some places
> > continue accessing struct l2cap_ctrl that resides in the SKB's CB for a
> > short time after l2cap_reassemble_sdu returns, leading to a
> > use-after-free condition (the stack trace is below, line numbers for
> > kernel 5.19.8).
> >
> > Fix it by keeping a local copy of struct l2cap_ctrl.
> >
> > BUG: KASAN: use-after-free in l2cap_rx_state_recv (net/bluetooth/l2cap_core.c:6906) bluetooth
> > Read of size 1 at addr ffff88812025f2f0 by task kworker/u17:3/43169
> >
> > Workqueue: hci0 hci_rx_work [bluetooth]
> > Call Trace:
> > <TASK>
> > dump_stack_lvl (lib/dump_stack.c:107 (discriminator 4))
> > print_report.cold (mm/kasan/report.c:314 mm/kasan/report.c:429)
> > ? l2cap_rx_state_recv (net/bluetooth/l2cap_core.c:6906) bluetooth
> > kasan_report (mm/kasan/report.c:162 mm/kasan/report.c:493)
> > ? l2cap_rx_state_recv (net/bluetooth/l2cap_core.c:6906) bluetooth
> > l2cap_rx_state_recv (net/bluetooth/l2cap_core.c:6906) bluetooth
> > l2cap_rx (net/bluetooth/l2cap_core.c:7236 net/bluetooth/l2cap_core.c:7271) bluetooth
> > ? sk_filter_trim_cap (net/core/filter.c:123)
> > ? l2cap_chan_hold_unless_zero (./arch/x86/include/asm/atomic.h:202 ./include/linux/atomic/atomic-instrumented.h:560 ./include/linux/refcount.h:157 ./include/linux/refcount.h:227 ./include/linux/refcount.h:245 ./include/linux/kref.h:111 net/bluetooth/l2cap_core.c:517) bluetooth
> > ? l2cap_rx_state_recv (net/bluetooth/l2cap_core.c:7252) bluetooth
> > l2cap_recv_frame (net/bluetooth/l2cap_core.c:7405 net/bluetooth/l2cap_core.c:7620 net/bluetooth/l2cap_core.c:7723) bluetooth
> > ? lock_release (./include/trace/events/lock.h:69 kernel/locking/lockdep.c:5677)
> > ? hci_rx_work (net/bluetooth/hci_core.c:3637 net/bluetooth/hci_core.c:3832) bluetooth
> > ? reacquire_held_locks (kernel/locking/lockdep.c:5674)
> > ? trace_contention_end (./include/trace/events/lock.h:122 ./include/trace/events/lock.h:122)
> > ? l2cap_config_rsp.isra.0 (net/bluetooth/l2cap_core.c:7674) bluetooth
> > ? hci_rx_work (./include/net/bluetooth/hci_core.h:1024 net/bluetooth/hci_core.c:3634 net/bluetooth/hci_core.c:3832) bluetooth
> > ? lock_acquire (./include/trace/events/lock.h:24 kernel/locking/lockdep.c:5637)
> > ? __mutex_unlock_slowpath (./arch/x86/include/asm/atomic64_64.h:190 ./include/linux/atomic/atomic-long.h:449 ./include/linux/atomic/atomic-instrumented.h:1790 kernel/locking/mutex.c:924)
> > ? rcu_read_lock_sched_held (kernel/rcu/update.c:104 kernel/rcu/update.c:123)
> > ? memcpy (mm/kasan/shadow.c:65 (discriminator 1))
> > ? l2cap_recv_frag (net/bluetooth/l2cap_core.c:8340) bluetooth
> > l2cap_recv_acldata (net/bluetooth/l2cap_core.c:8486) bluetooth
> > hci_rx_work (net/bluetooth/hci_core.c:3642 net/bluetooth/hci_core.c:3832) bluetooth
> > process_one_work (kernel/workqueue.c:2289)
> > ? lock_downgrade (kernel/locking/lockdep.c:5634)
> > ? pwq_dec_nr_in_flight (kernel/workqueue.c:2184)
> > ? rwlock_bug.part.0 (kernel/locking/spinlock_debug.c:113)
> > worker_thread (./include/linux/list.h:292 kernel/workqueue.c:2437)
> > ? __kthread_parkme (./arch/x86/include/asm/bitops.h:207 (discriminator 4) ./include/asm-generic/bitops/instrumented-non-atomic.h:135 (discriminator 4) kernel/kthread.c:270 (discriminator 4))
> > ? process_one_work (kernel/workqueue.c:2379)
> > kthread (kernel/kthread.c:376)
> > ? kthread_complete_and_exit (kernel/kthread.c:331)
> > ret_from_fork (arch/x86/entry/entry_64.S:306)
> > </TASK>
> >
> > Allocated by task 43169:
> > kasan_save_stack (mm/kasan/common.c:39)
> > __kasan_slab_alloc (mm/kasan/common.c:45 mm/kasan/common.c:436 mm/kasan/common.c:469)
> > kmem_cache_alloc_node (mm/slab.h:750 mm/slub.c:3243 mm/slub.c:3293)
> > __alloc_skb (net/core/skbuff.c:414)
> > l2cap_recv_frag (./include/net/bluetooth/bluetooth.h:425 net/bluetooth/l2cap_core.c:8329) bluetooth
> > l2cap_recv_acldata (net/bluetooth/l2cap_core.c:8442) bluetooth
> > hci_rx_work (net/bluetooth/hci_core.c:3642 net/bluetooth/hci_core.c:3832) bluetooth
> > process_one_work (kernel/workqueue.c:2289)
> > worker_thread (./include/linux/list.h:292 kernel/workqueue.c:2437)
> > kthread (kernel/kthread.c:376)
> > ret_from_fork (arch/x86/entry/entry_64.S:306)
> >
> > Freed by task 27920:
> > kasan_save_stack (mm/kasan/common.c:39)
> > kasan_set_track (mm/kasan/common.c:45)
> > kasan_set_free_info (mm/kasan/generic.c:372)
> > ____kasan_slab_free (mm/kasan/common.c:368 mm/kasan/common.c:328)
> > slab_free_freelist_hook (mm/slub.c:1780)
> > kmem_cache_free (mm/slub.c:3536 mm/slub.c:3553)
> > skb_free_datagram (./include/net/sock.h:1578 ./include/net/sock.h:1639 net/core/datagram.c:323)
> > bt_sock_recvmsg (net/bluetooth/af_bluetooth.c:295) bluetooth
> > l2cap_sock_recvmsg (net/bluetooth/l2cap_sock.c:1212) bluetooth
> > sock_read_iter (net/socket.c:1087)
> > new_sync_read (./include/linux/fs.h:2052 fs/read_write.c:401)
> > vfs_read (fs/read_write.c:482)
> > ksys_read (fs/read_write.c:620)
> > do_syscall_64 (arch/x86/entry/common.c:50 arch/x86/entry/common.c:80)
> > entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:120)
> >
> > Link: https://lore.kernel.org/linux-bluetooth/CAKErNvoqga1WcmoR3-0875esY6TVWFQDandbVZncSiuGPBQXLA@mail.gmail.com/T/#u
> > Fixes: d2a7ac5d5d3a ("Bluetooth: Add the ERTM receive state machine")
> > Fixes: 4b51dae96731 ("Bluetooth: Add streaming mode receive and incoming packet classifier")
> > Signed-off-by: Maxim Mikityanskiy <[email protected]>
> > ---
> > net/bluetooth/l2cap_core.c | 17 ++++++++++-------
> > 1 file changed, 10 insertions(+), 7 deletions(-)
> >
> > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> > index 2c9de67daadc..6bdce147d2fe 100644
> > --- a/net/bluetooth/l2cap_core.c
> > +++ b/net/bluetooth/l2cap_core.c
> > @@ -6876,6 +6876,7 @@ static int l2cap_rx_state_recv(struct l2cap_chan *chan,
> > struct l2cap_ctrl *control,
> > struct sk_buff *skb, u8 event)
> > {
> > + struct l2cap_ctrl local_control;
> > int err = 0;
> > bool skb_in_use = false;
> >
> > @@ -6900,15 +6901,16 @@ static int l2cap_rx_state_recv(struct l2cap_chan *chan,
> > chan->buffer_seq = chan->expected_tx_seq;
> > skb_in_use = true;
> >
> > + local_control = *control;
>
> Have a comment on why we are creating a local copy,
OK, I'll add a comment.
> btw should we pass the copy to l2cap_reassemble_sdu?
No need, l2cap_reassemble_sdu only reads control->sar in the very
beginning and never accesses control afterwards.
>
> > err = l2cap_reassemble_sdu(chan, skb, control);
> > if (err)
> > break;
> >
> > - if (control->final) {
> > + if (local_control.final) {
> > if (!test_and_clear_bit(CONN_REJ_ACT,
> > &chan->conn_state)) {
> > - control->final = 0;
> > - l2cap_retransmit_all(chan, control);
> > + local_control.final = 0;
> > + l2cap_retransmit_all(chan, &local_control);
> > l2cap_ertm_send(chan);
> > }
> > }
> > @@ -7288,11 +7290,12 @@ static int l2cap_rx(struct l2cap_chan *chan, struct l2cap_ctrl *control,
> > static int l2cap_stream_rx(struct l2cap_chan *chan, struct l2cap_ctrl *control,
> > struct sk_buff *skb)
> > {
> > + u16 txseq = control->txseq;
> > +
> > BT_DBG("chan %p, control %p, skb %p, state %d", chan, control, skb,
> > chan->rx_state);
> >
> > - if (l2cap_classify_txseq(chan, control->txseq) ==
> > - L2CAP_TXSEQ_EXPECTED) {
> > + if (l2cap_classify_txseq(chan, txseq) == L2CAP_TXSEQ_EXPECTED) {
> > l2cap_pass_to_tx(chan, control);
> >
> > BT_DBG("buffer_seq %u->%u", chan->buffer_seq,
> > @@ -7315,8 +7318,8 @@ static int l2cap_stream_rx(struct l2cap_chan *chan, struct l2cap_ctrl *control,
> > }
> > }
> >
> > - chan->last_acked_seq = control->txseq;
> > - chan->expected_tx_seq = __next_seq(chan, control->txseq);
> > + chan->last_acked_seq = txseq;
> > + chan->expected_tx_seq = __next_seq(chan, txseq);
> >
> > return 0;
> > }
> > --
> > 2.37.3
> >
>
>
> --
> Luiz Augusto von Dentz