Message ID | 20240426072006.358802-1-iam@sung-woo.kim |
---|---|
State | New |
Headers | show |
Series | Bluetooth: L2CAP: Fix slab-use-after-free in l2cap_send_cmd | expand |
Wrong call trace was attached. The correct one is below.
Sorry!
==================================================================
BUG: KASAN: slab-use-after-free in instrument_atomic_read include/linux/instrumented.h:68 [inline]
BUG: KASAN: slab-use-after-free in _test_bit include/asm-generic/bitops/instrumented-non-atomic.h:141 [inline]
BUG: KASAN: slab-use-after-free in l2cap_connect+0xa67/0x11a0 net/bluetooth/l2cap_core.c:4260
Read of size 8 at addr ffff88810bf040a0 by task kworker/u3:1/311
CPU: 0 PID: 311 Comm: kworker/u3:1 Not tainted 6.8.0+ #61
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
Workqueue: hci0 hci_rx_work
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0x85/0xb0 lib/dump_stack.c:106
print_address_description mm/kasan/report.c:377 [inline]
print_report+0x18f/0x560 mm/kasan/report.c:488
kasan_report+0xd7/0x110 mm/kasan/report.c:601
kasan_check_range+0x262/0x2f0 mm/kasan/generic.c:189
__kasan_check_read+0x15/0x20 mm/kasan/shadow.c:31
instrument_atomic_read include/linux/instrumented.h:68 [inline]
_test_bit include/asm-generic/bitops/instrumented-non-atomic.h:141 [inline]
l2cap_connect+0xa67/0x11a0 net/bluetooth/l2cap_core.c:4260
l2cap_bredr_sig_cmd+0x17fe/0x9a70
l2cap_sig_channel net/bluetooth/l2cap_core.c:6539 [inline]
l2cap_recv_frame+0x82e/0x86a0 net/bluetooth/l2cap_core.c:7818
l2cap_recv_acldata+0x379/0xbe0 net/bluetooth/l2cap_core.c:8536
hci_acldata_packet net/bluetooth/hci_core.c:3876 [inline]
hci_rx_work+0x64b/0xcb0 net/bluetooth/hci_core.c:4111
process_one_work kernel/workqueue.c:2633 [inline]
process_scheduled_works+0x6b9/0xdc0 kernel/workqueue.c:2706
worker_thread+0xb2b/0x13d0 kernel/workqueue.c:2787
kthread+0x2a9/0x340 kernel/kthread.c:388
ret_from_fork+0x5c/0x90 arch/x86/kernel/process.c:147
ret_from_fork_asm+0x1b/0x30 arch/x86/entry/entry_64.S:243
</TASK>
Allocated by task 311:
kasan_save_stack mm/kasan/common.c:47 [inline]
kasan_save_track+0x30/0x70 mm/kasan/common.c:68
kasan_save_alloc_info+0x3c/0x50 mm/kasan/generic.c:575
poison_kmalloc_redzone mm/kasan/common.c:370 [inline]
__kasan_kmalloc+0xa2/0xc0 mm/kasan/common.c:387
kasan_kmalloc include/linux/kasan.h:211 [inline]
kmalloc_trace+0x1c9/0x390 mm/slub.c:4012
kmalloc include/linux/slab.h:590 [inline]
kzalloc include/linux/slab.h:711 [inline]
l2cap_chan_create+0x59/0xc80 net/bluetooth/l2cap_core.c:466
l2cap_sock_alloc net/bluetooth/l2cap_sock.c:1849 [inline]
l2cap_sock_new_connection_cb+0x14d/0x2a0 net/bluetooth/l2cap_sock.c:1457
l2cap_connect+0x329/0x11a0 net/bluetooth/l2cap_core.c:4176
l2cap_bredr_sig_cmd+0x17fe/0x9a70
l2cap_sig_channel net/bluetooth/l2cap_core.c:6539 [inline]
l2cap_recv_frame+0x82e/0x86a0 net/bluetooth/l2cap_core.c:7818
l2cap_recv_acldata+0x379/0xbe0 net/bluetooth/l2cap_core.c:8536
hci_acldata_packet net/bluetooth/hci_core.c:3876 [inline]
hci_rx_work+0x64b/0xcb0 net/bluetooth/hci_core.c:4111
process_one_work kernel/workqueue.c:2633 [inline]
process_scheduled_works+0x6b9/0xdc0 kernel/workqueue.c:2706
worker_thread+0xb2b/0x13d0 kernel/workqueue.c:2787
kthread+0x2a9/0x340 kernel/kthread.c:388
ret_from_fork+0x5c/0x90 arch/x86/kernel/process.c:147
ret_from_fork_asm+0x1b/0x30 arch/x86/entry/entry_64.S:243
Freed by task 66:
kasan_save_stack mm/kasan/common.c:47 [inline]
kasan_save_track+0x30/0x70 mm/kasan/common.c:68
kasan_save_free_info+0x44/0x50 mm/kasan/generic.c:589
poison_slab_object+0x11a/0x190 mm/kasan/common.c:240
__kasan_slab_free+0x3b/0x60 mm/kasan/common.c:256
kasan_slab_free include/linux/kasan.h:184 [inline]
slab_free_hook mm/slub.c:2121 [inline]
slab_free mm/slub.c:4299 [inline]
kfree+0x106/0x2e0 mm/slub.c:4409
l2cap_chan_destroy net/bluetooth/l2cap_core.c:509 [inline]
kref_put include/linux/kref.h:65 [inline]
l2cap_chan_put+0x1e7/0x2b0 net/bluetooth/l2cap_core.c:533
l2cap_conn_del+0x38e/0x5f0 net/bluetooth/l2cap_core.c:1929
l2cap_connect_cfm+0xc2/0x11e0 net/bluetooth/l2cap_core.c:8254
hci_connect_cfm include/net/bluetooth/hci_core.h:1986 [inline]
hci_conn_failed+0x202/0x370 net/bluetooth/hci_conn.c:1289
hci_abort_conn_sync+0x913/0xae0 net/bluetooth/hci_sync.c:5359
abort_conn_sync+0xda/0x110 net/bluetooth/hci_conn.c:2988
hci_cmd_sync_work+0x20d/0x3e0 net/bluetooth/hci_sync.c:306
process_one_work kernel/workqueue.c:2633 [inline]
process_scheduled_works+0x6b9/0xdc0 kernel/workqueue.c:2706
worker_thread+0xb2b/0x13d0 kernel/workqueue.c:2787
kthread+0x2a9/0x340 kernel/kthread.c:388
ret_from_fork+0x5c/0x90 arch/x86/kernel/process.c:147
ret_from_fork_asm+0x1b/0x30 arch/x86/entry/entry_64.S:243
The buggy address belongs to the object at ffff88810bf04000
which belongs to the cache kmalloc-1k of size 1024
The buggy address is located 160 bytes inside of
freed 1024-byte region [ffff88810bf04000, ffff88810bf04400)
The buggy address belongs to the physical page:
page:00000000567b7faa refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x10bf04
head:00000000567b7faa order:2 entire_mapcount:0 nr_pages_mapped:0 pincount:0
anon flags: 0x17ffffc0000840(slab|head|node=0|zone=2|lastcpupid=0x1fffff)
page_type: 0xffffffff()
raw: 0017ffffc0000840 ffff888100041dc0 0000000000000000 0000000000000001
raw: 0000000000000000 0000000080080008 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected
Memory state around the buggy address:
ffff88810bf03f80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
ffff88810bf04000: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>ffff88810bf04080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
^
ffff88810bf04100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff88810bf04180: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================
I prefer that you would put recipient specifications also into the message field “To” (besides “Cc”). > Hello, could you review a bug and its fix? I suggest to omit such a question from better change descriptions. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9-rc5#n45 … > To fix this, this patch holds and locks the l2cap channel. Please choose a corresponding imperative wording. You would probably like to improve your patch approach further so that provided data will be kept consistent. https://lore.kernel.org/lkml/20240426073142.363876-1-iam@sung-woo.kim/ Regards, Markus
On Fri, Apr 26, 2024 at 5:16 AM Dan Carpenter <dan.carpenter@linaro.org> 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 5:38 AM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > Please just ignore Markus. A lot of people have asked him to stop > commenting on commit messages but he doesn't listen. Here is a message > from yesterday: > > https://lore.kernel.org/all/2024042547-shimmy-guileless-c7f2@gregkh/ > > 1) It doesn't matter at all if there is anyone in the To: header. > 2) You are allowed to ask questions. > 3) Yes, the commit message will need to be changed but first fix the bug > and then we can worry about the commit message. > > regards, > dan carpenter > > Thank you for letting me know :)
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:
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; }