diff mbox series

Bluetooth: L2CAP: Fix slab-use-after-free in l2cap_send_cmd

Message ID 20240426072006.358802-1-iam@sung-woo.kim (mailing list archive)
State New, archived
Headers show
Series Bluetooth: L2CAP: Fix slab-use-after-free in l2cap_send_cmd | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/CheckPatch warning WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit description?) #113: Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014 total: 0 errors, 1 warnings, 0 checks, 20 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/src/13644166.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.
tedd_an/GitLint fail WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search 33: B1 Line exceeds max length (92>80): "BUG: KASAN: slab-use-after-free in l2cap_send_cmd+0x5dc/0x830 net/bluetooth/l2cap_core.c:968" 160: B1 Line exceeds max length (91>80): "page:000000007f8eb4fe refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x10b628"
tedd_an/SubjectPrefix success Gitlint PASS
tedd_an/BuildKernel success BuildKernel PASS
tedd_an/CheckAllWarning success CheckAllWarning PASS
tedd_an/CheckSparse success CheckSparse PASS
tedd_an/CheckSmatch fail CheckSparse: FAIL: Segmentation fault (core dumped) make[4]: *** [scripts/Makefile.build:244: net/bluetooth/hci_core.o] Error 139 make[4]: *** Deleting file 'net/bluetooth/hci_core.o' make[3]: *** [scripts/Makefile.build:485: net/bluetooth] Error 2 make[2]: *** [scripts/Makefile.build:485: net] Error 2 make[2]: *** Waiting for unfinished jobs.... Segmentation fault (core dumped) make[4]: *** [scripts/Makefile.build:244: drivers/bluetooth/bcm203x.o] Error 139 make[4]: *** Deleting file 'drivers/bluetooth/bcm203x.o' make[4]: *** Waiting for unfinished jobs.... make[3]: *** [scripts/Makefile.build:485: drivers/bluetooth] Error 2 make[2]: *** [scripts/Makefile.build:485: drivers] Error 2 make[1]: *** [/github/workspace/src/src/Makefile:1919: .] Error 2 make: *** [Makefile:240: __sub-make] Error 2
tedd_an/BuildKernel32 success BuildKernel32 PASS
tedd_an/TestRunnerSetup success TestRunnerSetup PASS
tedd_an/TestRunner_l2cap-tester success TestRunner PASS
tedd_an/TestRunner_iso-tester fail TestRunner_iso-tester: Total: 122, Passed: 121 (99.2%), Failed: 1, Not Run: 0
tedd_an/TestRunner_bnep-tester success TestRunner PASS

Commit Message

Sungwoo Kim April 26, 2024, 7:20 a.m. UTC
Hello, could you review a bug and its fix?

This is a racy bug. Call stack is as below:

[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)
└ mutex_unlock(&conn->chan_lock);

[use]
l2cap_bredr_sig_cmd
  l2cap_connect
  ┌ mutex_lock(&conn->chan_lock);
  │ chan = pchan->ops->new_connection(pchan); // alloc 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 // uaf at chan           ... (4)

To fix this, this patch holds and locks the l2cap channel.

Thanks,
Sungwoo.

==================================================================
BUG: KASAN: slab-use-after-free in l2cap_send_cmd+0x5dc/0x830 net/bluetooth/l2cap_core.c:968
Read of size 1 at addr ffff88810b62c274 by task kworker/0:1/10

CPU: 0 PID: 10 Comm: kworker/0:1 Not tainted 6.8.0+ #61
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
Workqueue: events l2cap_info_timeout
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
 __asan_report_load1_noabort+0x18/0x20 mm/kasan/report_generic.c:378
 l2cap_send_cmd+0x5dc/0x830 net/bluetooth/l2cap_core.c:968
 l2cap_send_conn_req net/bluetooth/l2cap_core.c:1286 [inline]
 l2cap_start_connection+0x25e/0x530 net/bluetooth/l2cap_core.c:1514
 l2cap_conn_start+0x952/0xe60 net/bluetooth/l2cap_core.c:1661
 l2cap_info_timeout+0x5f/0x90 net/bluetooth/l2cap_core.c:1807
 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 290:
 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]
 __do_kmalloc_node mm/slub.c:3981 [inline]
 __kmalloc+0x228/0x4d0 mm/slub.c:3994
 kmalloc include/linux/slab.h:594 [inline]
 kzalloc include/linux/slab.h:711 [inline]
 hci_alloc_dev_priv+0x30/0x1ea0 net/bluetooth/hci_core.c:2468
 hci_alloc_dev include/net/bluetooth/hci_core.h:1651 [inline]
 __vhci_create_device drivers/bluetooth/hci_vhci.c:406 [inline]
 vhci_create_device+0x10e/0x710 drivers/bluetooth/hci_vhci.c:480
 vhci_get_user drivers/bluetooth/hci_vhci.c:537 [inline]
 vhci_write+0x398/0x460 drivers/bluetooth/hci_vhci.c:617
 call_write_iter include/linux/fs.h:2087 [inline]
 new_sync_write fs/read_write.c:497 [inline]
 vfs_write+0x9ee/0xd40 fs/read_write.c:590
 ksys_write+0x103/0x1f0 fs/read_write.c:643
 __do_sys_write fs/read_write.c:655 [inline]
 __se_sys_write fs/read_write.c:652 [inline]
 __x64_sys_write+0x84/0xa0 fs/read_write.c:652
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0x84/0x120 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x6e/0x76

Freed by task 1158:
 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
 hci_release_dev+0x1114/0x1250 net/bluetooth/hci_core.c:2799
 bt_host_release+0x77/0x90 net/bluetooth/hci_sysfs.c:94
 device_release+0xa4/0x1d0
 kobject_cleanup lib/kobject.c:689 [inline]
 kobject_release lib/kobject.c:720 [inline]
 kref_put include/linux/kref.h:65 [inline]
 kobject_put+0x1f1/0x410 lib/kobject.c:737
 put_device+0x28/0x40 drivers/base/core.c:3747
 hci_free_dev+0x25/0x30 net/bluetooth/hci_core.c:2594
 vhci_release+0x91/0xe0 drivers/bluetooth/hci_vhci.c:675
 __fput+0x35b/0x740 fs/file_table.c:376
 ____fput+0x1e/0x30 fs/file_table.c:404
 task_work_run+0x1d9/0x270 kernel/task_work.c:180
 exit_task_work include/linux/task_work.h:38 [inline]
 do_exit+0x823/0x23e0 kernel/exit.c:871
 do_group_exit+0x1f1/0x2b0 kernel/exit.c:1020
 get_signal+0x1387/0x14d0 kernel/signal.c:2893
 arch_do_signal_or_restart+0x3b/0x650 arch/x86/kernel/signal.c:310
 exit_to_user_mode_loop kernel/entry/common.c:105 [inline]
 exit_to_user_mode_prepare include/linux/entry-common.h:328 [inline]
 __syscall_exit_to_user_mode_work kernel/entry/common.c:201 [inline]
 syscall_exit_to_user_mode+0x71/0x180 kernel/entry/common.c:212
 do_syscall_64+0x90/0x120 arch/x86/entry/common.c:89
 entry_SYSCALL_64_after_hwframe+0x6e/0x76

Last potentially related work creation:
 kasan_save_stack+0x2f/0x50 mm/kasan/common.c:47
 __kasan_record_aux_stack+0xc6/0xe0 mm/kasan/generic.c:551
 kasan_record_aux_stack_noalloc+0xf/0x20 mm/kasan/generic.c:561
 insert_work+0x3a/0x1d0 kernel/workqueue.c:1653
 __queue_work+0xa5b/0xdc0 kernel/workqueue.c:1806
 queue_work_on+0x74/0xa0 kernel/workqueue.c:1837
 queue_work include/linux/workqueue.h:548 [inline]
 hci_recv_frame+0x375/0x490 net/bluetooth/hci_core.c:2947
 hci_reset_dev+0x126/0x170 net/bluetooth/hci_core.c:2904
 hci_ncmd_timeout+0xb1/0xd0 net/bluetooth/hci_core.c:1525
 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

Second to last potentially related work creation:
 kasan_save_stack+0x2f/0x50 mm/kasan/common.c:47
 __kasan_record_aux_stack+0xc6/0xe0 mm/kasan/generic.c:551
 kasan_record_aux_stack_noalloc+0xf/0x20 mm/kasan/generic.c:561
 insert_work kernel/workqueue.c:1653 [inline]
 __queue_work+0x8cf/0xdc0 kernel/workqueue.c:1802
 delayed_work_timer_fn+0x6a/0x90 kernel/workqueue.c:1931
 call_timer_fn+0x44/0x240 kernel/time/timer.c:1700
 expire_timers kernel/time/timer.c:1746 [inline]
 __run_timers+0x63a/0x870 kernel/time/timer.c:2038
 run_timer_softirq+0x26/0x40 kernel/time/timer.c:2051
 __do_softirq+0x180/0x523 kernel/softirq.c:553

The buggy address belongs to the object at ffff88810b62c000
 which belongs to the cache kmalloc-8k of size 8192
The buggy address is located 628 bytes inside of
 freed 8192-byte region [ffff88810b62c000, ffff88810b62e000)

The buggy address belongs to the physical page:
page:000000007f8eb4fe refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x10b628
head:000000007f8eb4fe order:3 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 ffff888100042280 0000000000000000 0000000000000001
raw: 0000000000000000 0000000000020002 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
 ffff88810b62c100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 ffff88810b62c180: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>ffff88810b62c200: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
                                                             ^
 ffff88810b62c280: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 ffff88810b62c300: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================

Signed-off-by: Sungwoo Kim <iam@sung-woo.kim>
---
 net/bluetooth/l2cap_core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Sungwoo Kim April 26, 2024, 7:31 a.m. UTC | #1
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
==================================================================
Paul Menzel April 26, 2024, 7:55 a.m. UTC | #2
Dear Sungwoo,


Am 26.04.24 um 09:31 schrieb Sungwoo Kim:
> Wrong call trace was attached. The correct one is below.
> Sorry!

No problem, thank you for noticing. Please send a v2, so people using 
automatic tools can just use those.


Kind regards,

Paul
Markus Elfring April 26, 2024, 8:25 a.m. UTC | #3
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
Sungwoo Kim April 26, 2024, 8:38 a.m. UTC | #4
On Fri, Apr 26, 2024 at 4:26 AM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> I prefer that you would put recipient specifications also into the message field “To”
> (besides “Cc”).

Okay.

>
>
> > 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

Thank you. I'll thoroughly read this.

>
>
> …
> > To fix this, this patch holds and locks the l2cap channel.
>
> Please choose a corresponding imperative wording.

Okay.

>
>
> You would probably like to improve your patch approach further
> so that provided data will be kept consistent.

I will.

> https://lore.kernel.org/lkml/20240426073142.363876-1-iam@sung-woo.kim/
>
> Regards,
> Markus
>

On Fri, Apr 26, 2024 at 4:26 AM Markus Elfring <Markus.Elfring@web.de> wrote:
>
> 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
>
Dan Carpenter April 26, 2024, 9:16 a.m. UTC | #5
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.

regards,
dan carpenter
Sungwoo Kim April 26, 2024, 9:35 a.m. UTC | #6
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.
Dan Carpenter April 26, 2024, 9:38 a.m. UTC | #7
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
Sungwoo Kim April 26, 2024, 9:48 a.m. UTC | #8
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 :)
Markus Elfring April 26, 2024, 10:01 a.m. UTC | #9
> 1) It doesn't matter at all if there is anyone in the To: header.

I suggest to reconsider this view according to proper selection of message recipients.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9-rc5#n231


> 2) You are allowed to ask questions.

Yes, of course.

But we are looking more for “answers” in consistent and improved change descriptions,
aren't we?

The placement of related wordings can be adjusted another bit.


> 3) Yes, the commit message will need to be changed but first fix the bug
>    and then we can worry about the commit message.

Will such a suggestion trigger further clarifications for the desired patching process?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9-rc5#n77


I hope that recurring communication difficulties will result in helpful ideas.

Regards,
Markus
Dan Carpenter April 26, 2024, 10:24 a.m. UTC | #10
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 mbox series

Patch

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;
 }