| Message ID | 20240701194531.97576-2-yskelg@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | hci: fix double free in hci_req_sync | expand |
| 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?) #129: 6.10.0-rc4-00217-g35bb670d65fc-dirty #22 Hardware name: linux,dummy-virt (DT) CHECK: Macro argument reuse 'hdev' - possible side-effects? #304: FILE: net/bluetooth/hci_request.h:31: +#define hci_req_skb_release_and_set(hdev, val) \ +({ \ + if (hdev->req_skb) { \ + spin_lock(&hdev->req_skb_lock); \ + kfree_skb(hdev->req_skb); \ + hdev->req_skb = val; \ + spin_unlock(&hdev->req_skb_lock); \ + } \ +}) CHECK: Macro argument 'hdev' may be better as '(hdev)' to avoid precedence issues #304: FILE: net/bluetooth/hci_request.h:31: +#define hci_req_skb_release_and_set(hdev, val) \ +({ \ + if (hdev->req_skb) { \ + spin_lock(&hdev->req_skb_lock); \ + kfree_skb(hdev->req_skb); \ + hdev->req_skb = val; \ + spin_unlock(&hdev->req_skb_lock); \ + } \ +}) total: 0 errors, 1 warnings, 2 checks, 78 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/13718597.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 31: B1 Line exceeds max length (103>80): "BUG: KASAN: slab-use-after-free in skb_release_data+0x7d8/0x8a0 home/paran/linux/net/core/skbuff.c:1119" 44: B1 Line exceeds max length (86>80): " __asan_report_store1_noabort+0x44/0x60 home/paran/linux/mm/kasan/report_generic.c:383" 61: B1 Line exceeds max length (88>80): " el0_svc_common.constprop.0+0x2d4/0x3e8 home/paran/linux/arch/arm64/kernel/syscall.c:133" 64: B1 Line exceeds max length (87>80): " el0t_64_sync_handler+0x120/0x130 home/paran/linux/arch/arm64/kernel/entry-common.c:730" 139: B2 Line has trailing whitespace: "Since applying our patch, we have repeatedly run the same tests in " 142: B1 Line exceeds max length (122>80): "Link: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=89a32741f4217856066c198a4a7267bcdd1edd67" |
| tedd_an/SubjectPrefix | fail | "Bluetooth: " prefix is not specified in the subject |
| tedd_an/BuildKernel | success | BuildKernel PASS |
| tedd_an/CheckAllWarning | success | CheckAllWarning PASS |
| tedd_an/CheckSparse | success | CheckSparse PASS |
| tedd_an/CheckSmatch | success | CheckSparse PASS |
| 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 | success | TestRunner PASS |
| tedd_an/TestRunner_bnep-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mgmt-tester | fail | TestRunner_mgmt-tester: Total: 492, Passed: 489 (99.4%), Failed: 1, Not Run: 2 |
| tedd_an/TestRunner_rfcomm-tester | success | TestRunner PASS |
| tedd_an/TestRunner_sco-tester | success | TestRunner PASS |
| tedd_an/TestRunner_ioctl-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mesh-tester | success | TestRunner PASS |
| tedd_an/TestRunner_smp-tester | success | TestRunner PASS |
| tedd_an/TestRunner_userchan-tester | success | TestRunner PASS |
| tedd_an/IncrementalBuild | success | Incremental Build PASS |
Hi, On Mon, Jul 1, 2024 at 3:45 PM Yunseong Kim <yskelg@gmail.com> wrote: > > The approach taken to address the 'CVE-2024-35978' introduced another > double-free vulnerability. commit 45d355a926ab > ("Bluetooth: Fix memory leak in hci_req_sync_complete()") > > 'hdev->req_skb' double free scenario: > > cpu1 cpu2 > ==== ==== > sock_ioctl > sock_do_ioctl > hci_sock_ioctl > hci_dev_cmd > hci_req_sync hci_req_sync is no longer called from hci_dev_cmd: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/?id=6851d11d389ceb00c1220b267b9a04f54dbc4573 And I'm in process of removing hci_request.c completely. > __hci_req_sync hci_rx_work > kfree_skb(hdev->req_skb) hci_event_packet > (sleep) hci_req_sync_complete > \__ Longer times, kfree_skb(hdev->req_skb) > reproduce well hdev->req_skb = NULL > > The longer cpu1 sleep in '__hci_req_sync', the more reproducible it is. > We've tested it by inserting the 'msleep()' function, and it's frequently > at 1000ms, and It has been consistently reproducible at 2000ms. > > We confirmed the detection with various workloads that cause CPU1 to sleep. > The call trace below is one of the KASAN has seen. > > Bluetooth: hci0: unexpected cc 0x0c38 length: 249 > 2 > ================================================================== > BUG: KASAN: slab-use-after-free in skb_release_data+0x7d8/0x8a0 home/paran/linux/net/core/skbuff.c:1119 > Write of size 1 at addr ffff00000947d3fe by task syz-executor.2/2010 > > CPU: 1 PID: 2010 Comm: syz-executor.2 Not tainted > 6.10.0-rc4-00217-g35bb670d65fc-dirty #22 Hardware name: linux,dummy-virt (DT) > Call trace: > dump_backtrace+0x318/0x348 home/paran/linux/arch/arm64/kernel/stacktrace.c:317 > show_stack+0x4c/0x80 home/paran/linux/arch/arm64/kernel/stacktrace.c:324 > __dump_stack home/paran/linux/lib/dump_stack.c:88 [inline] > dump_stack_lvl+0x214/0x328 home/paran/linux/lib/dump_stack.c:114 > print_address_description home/paran/linux/mm/kasan/report.c:377 [inline] > print_report+0x2ac/0x948 home/paran/linux/mm/kasan/report.c:488 > kasan_report+0xc8/0x148 home/paran/linux/mm/kasan/report.c:601 > __asan_report_store1_noabort+0x44/0x60 home/paran/linux/mm/kasan/report_generic.c:383 > skb_release_data+0x7d8/0x8a0 home/paran/linux/net/core/skbuff.c:1119 > skb_release_all+0x80/0xe0 home/paran/linux/net/core/skbuff.c:1173 > __kfree_skb home/paran/linux/net/core/skbuff.c:1187 [inline] > kfree_skb_reason+0x138/0x3a8 home/paran/linux/net/core/skbuff.c:1223 > __hci_req_sync+0x404/0x948 [bluetooth] > hci_req_sync+0xc0/0x138 [bluetooth] > hci_dev_cmd+0x33c/0xc18 [bluetooth] > hci_sock_ioctl+0x800/0xb68 [bluetooth] > sock_do_ioctl+0xfc/0x2e0 home/paran/linux/net/socket.c:1222 > sock_ioctl+0x62c/0xab0 home/paran/linux/net/socket.c:1341 > vfs_ioctl+0x90/0x140 home/paran/linux/fs/ioctl.c:51 > __do_sys_ioctl home/paran/linux/fs/ioctl.c:907 [inline] > __se_sys_ioctl home/paran/linux/fs/ioctl.c:893 [inline] > __arm64_sys_ioctl+0x218/0x268 home/paran/linux/fs/ioctl.c:893 > __invoke_syscall home/paran/linux/arch/arm64/kernel/syscall.c:34 [inline] > invoke_syscall+0xdc/0x460 home/paran/linux/arch/arm64/kernel/syscall.c:48 > el0_svc_common.constprop.0+0x2d4/0x3e8 home/paran/linux/arch/arm64/kernel/syscall.c:133 > do_el0_svc+0x60/0x98 home/paran/linux/arch/arm64/kernel/syscall.c:152 > el0_svc+0xc4/0x240 home/paran/linux/arch/arm64/kernel/entry-common.c:712 > el0t_64_sync_handler+0x120/0x130 home/paran/linux/arch/arm64/kernel/entry-common.c:730 > el0t_64_sync+0x190/0x198 home/paran/linux/arch/arm64/kernel/entry.S:598 > > Allocated by task 577: > kasan_save_stack+0x48/0x90 home/paran/linux/mm/kasan/common.c:47 > kasan_save_track+0x38/0x60 home/paran/linux/mm/kasan/common.c:68 > kasan_save_alloc_info+0x64/0xc0 home/paran/linux/mm/kasan/generic.c:565 > unpoison_slab_object home/paran/linux/mm/kasan/common.c:312 [inline] > __kasan_slab_alloc+0x100/0x110 home/paran/linux/mm/kasan/common.c:338 > kasan_slab_alloc home/paran/linux/./include/linux/kasan.h:201 [inline] > slab_post_alloc_hook home/paran/linux/mm/slub.c:3941 [inline] > slab_alloc_node home/paran/linux/mm/slub.c:4001 [inline] > kmem_cache_alloc_noprof+0x2e8/0x630 home/paran/linux/mm/slub.c:4008 > skb_clone+0x1a4/0x4d0 home/paran/linux/net/core/skbuff.c:2052 > hci_cmd_work+0x78c/0x868 [bluetooth] > process_one_work home/paran/linux/kernel/workqueue.c:3231 [inline] > process_scheduled_works+0x9fc/0x1d98 home/paran/linux/kernel/workqueue.c:3312 > worker_thread+0x57c/0xf98 home/paran/linux/kernel/workqueue.c:3393 > kthread+0x3c8/0x478 home/paran/linux/kernel/kthread.c:389 > ret_from_fork+0x10/0x20 home/paran/linux/arch/arm64/kernel/entry.S:860 > > Freed by task 577: > kasan_save_stack+0x48/0x90 home/paran/linux/mm/kasan/common.c:47 > kasan_save_track+0x38/0x60 home/paran/linux/mm/kasan/common.c:68 > kasan_save_free_info+0x64/0xf0 home/paran/linux/mm/kasan/generic.c:579 > poison_slab_object+0x168/0x270 home/paran/linux/mm/kasan/common.c:240 > __kasan_slab_free+0x34/0xa0 home/paran/linux/mm/kasan/common.c:256 > kasan_slab_free home/paran/linux/./include/linux/kasan.h:184 [inline] > slab_free_hook home/paran/linux/mm/slub.c:2196 [inline] > slab_free home/paran/linux/mm/slub.c:4437 [inline] > kmem_cache_free+0x20c/0x670 home/paran/linux/mm/slub.c:4512 > kfree_skbmem+0x2b0/0x390 home/paran/linux/net/core/skbuff.c:1131 > __kfree_skb home/paran/linux/net/core/skbuff.c:1188 [inline] > kfree_skb_reason+0x14c/0x3a8 home/paran/linux/net/core/skbuff.c:1223 > hci_req_sync_complete+0x114/0x308 [bluetooth] > hci_event_packet+0xa10/0x12a0 [bluetooth] > hci_rx_work+0x4d8/0xa80 [bluetooth] > process_one_work home/paran/linux/kernel/workqueue.c:3231 [inline] > process_scheduled_works+0x9fc/0x1d98 home/paran/linux/kernel/workqueue.c:3312 > worker_thread+0x57c/0xf98 home/paran/linux/kernel/workqueue.c:3393 > kthread+0x3c8/0x478 home/paran/linux/kernel/kthread.c:389 > ret_from_fork+0x10/0x20 home/paran/linux/arch/arm64/kernel/entry.S:860 > > The buggy address belongs to the object at ffff00000947d380 > which belongs to the cache skbuff_head_cache of size 232 > The buggy address is located 126 bytes inside of > freed 232-byte region [ffff00000947d380, ffff00000947d468) > > The buggy address belongs to the physical page: > page: refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x4947c > head: order:1 mapcount:0 entire_mapcount:0 nr_pages_mapped:0 pincount:0 > memcg:ffff0000222a73b1 > flags: 0x3fffe0000000040(head|node=0|zone=0|lastcpupid=0x1ffff) > page_type: 0xffffefff(slab) > raw: 03fffe0000000040 ffff0000133a41c0 fffffdffc0e3a090 fffffdffc1069290 > raw: 0000000000000000 0000000000120012 00000001ffffefff ffff0000222a73b1 > head: 03fffe0000000040 ffff0000133a41c0 fffffdffc0e3a090 fffffdffc1069290 > head: 0000000000000000 0000000000120012 00000001ffffefff ffff0000222a73b1 > head: 03fffe0000000001 fffffdffc0251f01 ffffffffffffffff 0000000000000000 > head: 0000000000000002 0000000000000000 00000000ffffffff 0000000000000000 > page dumped because: kasan: bad access detected > > Memory state around the buggy address: > ffff00000947d280: 00 00 00 00 00 fc fc fc fc fc fc fc fc fc fc fc > ffff00000947d300: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc > >ffff00000947d380: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > ^ > ffff00000947d400: fb fb fb fb fb fb fb fb fb fb fb fb fb fc fc fc > ffff00000947d480: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc > ================================================================== > > We considered using the "hci_req_sync_lock" mutex, but concluded it > wasn't a good solution due to the increased sleep intervals causing > this issue. Instead, we introduced a spinlock member on 'struct hci_dev'. > > Since applying our patch, we have repeatedly run the same tests in > the syzkaller without encountering any issues. > > Link: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=89a32741f4217856066c198a4a7267bcdd1edd67 > Fixes: 45d355a926ab ("Bluetooth: Fix memory leak in hci_req_sync_complete()") > Signed-off-by: Levi Yun <yeoreum.yun@arm.com> > Signed-off-by: Yunseong Kim <yskelg@gmail.com> > --- > include/net/bluetooth/hci_core.h | 1 + > net/bluetooth/hci_core.c | 1 + > net/bluetooth/hci_request.c | 6 ++---- > net/bluetooth/hci_request.h | 9 +++++++++ > net/bluetooth/hci_sync.c | 13 +++---------- > 5 files changed, 16 insertions(+), 14 deletions(-) > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > index c43716edf205..8b95061f063b 100644 > --- a/include/net/bluetooth/hci_core.h > +++ b/include/net/bluetooth/hci_core.h > @@ -519,6 +519,7 @@ struct hci_dev { > struct sk_buff *recv_event; > > struct mutex req_lock; > + spinlock_t req_skb_lock; > wait_queue_head_t req_wait_q; > __u32 req_status; > __u32 req_result; > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c > index dd3b0f501018..138a6b19894d 100644 > --- a/net/bluetooth/hci_core.c > +++ b/net/bluetooth/hci_core.c > @@ -2572,6 +2572,7 @@ struct hci_dev *hci_alloc_dev_priv(int sizeof_priv) > > mutex_init(&hdev->lock); > mutex_init(&hdev->req_lock); > + spin_lock_init(&hdev->req_skb_lock); > > ida_init(&hdev->unset_handle_ida); > > diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c > index efea25eb56ce..6a109c1ad359 100644 > --- a/net/bluetooth/hci_request.c > +++ b/net/bluetooth/hci_request.c > @@ -106,8 +106,7 @@ void hci_req_sync_complete(struct hci_dev *hdev, u8 result, u16 opcode, > hdev->req_result = result; > hdev->req_status = HCI_REQ_DONE; > if (skb) { > - kfree_skb(hdev->req_skb); > - hdev->req_skb = skb_get(skb); > + hci_req_skb_release_and_set(hdev, skb_get(skb)); > } > wake_up_interruptible(&hdev->req_wait_q); > } > @@ -181,8 +180,7 @@ int __hci_req_sync(struct hci_dev *hdev, int (*func)(struct hci_request *req, > break; > } > > - kfree_skb(hdev->req_skb); > - hdev->req_skb = NULL; > + hci_req_skb_release_and_set(hdev, NULL); > hdev->req_status = hdev->req_result = 0; > > bt_dev_dbg(hdev, "end: err %d", err); > diff --git a/net/bluetooth/hci_request.h b/net/bluetooth/hci_request.h > index c91f2838f542..6526c78443bc 100644 > --- a/net/bluetooth/hci_request.h > +++ b/net/bluetooth/hci_request.h > @@ -28,6 +28,15 @@ > > #define hci_req_sync_lock(hdev) mutex_lock(&hdev->req_lock) > #define hci_req_sync_unlock(hdev) mutex_unlock(&hdev->req_lock) > +#define hci_req_skb_release_and_set(hdev, val) \ > +({ \ > + if (hdev->req_skb) { \ > + spin_lock(&hdev->req_skb_lock); \ > + kfree_skb(hdev->req_skb); \ > + hdev->req_skb = val; \ > + spin_unlock(&hdev->req_skb_lock); \ > + } \ > +}) > > struct hci_request { > struct hci_dev *hdev; > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c > index a8a7d2b36870..25c8d858c82e 100644 > --- a/net/bluetooth/hci_sync.c > +++ b/net/bluetooth/hci_sync.c > @@ -33,8 +33,7 @@ static void hci_cmd_sync_complete(struct hci_dev *hdev, u8 result, u16 opcode, > hdev->req_status = HCI_REQ_DONE; > > /* Free the request command so it is not used as response */ > - kfree_skb(hdev->req_skb); > - hdev->req_skb = NULL; This doesn't even apply upstream > + hci_req_skb_release_and_set(hdev, NULL); > > if (skb) { > struct sock *sk = hci_skb_sk(skb); > @@ -4935,10 +4934,7 @@ int hci_dev_open_sync(struct hci_dev *hdev) > hdev->sent_cmd = NULL; > } > > - if (hdev->req_skb) { > - kfree_skb(hdev->req_skb); > - hdev->req_skb = NULL; > - } > + hci_req_skb_release_and_set(hdev, NULL); > > clear_bit(HCI_RUNNING, &hdev->flags); > hci_sock_dev_event(hdev, HCI_DEV_CLOSE); > @@ -5100,10 +5096,7 @@ int hci_dev_close_sync(struct hci_dev *hdev) > } > > /* Drop last request */ > - if (hdev->req_skb) { > - kfree_skb(hdev->req_skb); > - hdev->req_skb = NULL; > - } > + hci_req_skb_release_and_set(hdev, NULL); > > clear_bit(HCI_RUNNING, &hdev->flags); > hci_sock_dev_event(hdev, HCI_DEV_CLOSE); > -- > 2.45.2 >
Hi Luiz, On 7/2/24 5:06 오전, Luiz Augusto von Dentz wrote: > Hi, > > On Mon, Jul 1, 2024 at 3:45 PM Yunseong Kim <yskelg@gmail.com> wrote: >> >> The approach taken to address the 'CVE-2024-35978' introduced another >> double-free vulnerability. commit 45d355a926ab >> ("Bluetooth: Fix memory leak in hci_req_sync_complete()") >> >> 'hdev->req_skb' double free scenario: >> >> cpu1 cpu2 >> ==== ==== >> sock_ioctl >> sock_do_ioctl >> hci_sock_ioctl >> hci_dev_cmd >> hci_req_sync > > hci_req_sync is no longer called from hci_dev_cmd: > > https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/?id=6851d11d389ceb00c1220b267b9a04f54dbc4573 > > And I'm in process of removing hci_request.c completely. > >> __hci_req_sync hci_rx_work >> kfree_skb(hdev->req_skb) hci_event_packet >> (sleep) hci_req_sync_complete >> \__ Longer times, kfree_skb(hdev->req_skb) >> reproduce well hdev->req_skb = NULL >> Thank you so much your hard work. I saw the patch email at the link below. Link: https://lore.kernel.org/all/a9609dd3d0cb3b8c3fd387efe8a81eddc821be0f.camel@iki.fi/T/ Does the patch you mentioned apply to backports as well? Warm regards, Yunseong Kim
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=867274
---Test result---
Test Summary:
CheckPatch FAIL 1.82 seconds
GitLint FAIL 0.49 seconds
SubjectPrefix FAIL 0.31 seconds
BuildKernel PASS 30.54 seconds
CheckAllWarning PASS 45.30 seconds
CheckSparse PASS 38.87 seconds
CheckSmatch PASS 107.07 seconds
BuildKernel32 PASS 29.74 seconds
TestRunnerSetup PASS 530.55 seconds
TestRunner_l2cap-tester PASS 24.05 seconds
TestRunner_iso-tester PASS 37.17 seconds
TestRunner_bnep-tester PASS 4.93 seconds
TestRunner_mgmt-tester FAIL 110.24 seconds
TestRunner_rfcomm-tester PASS 7.56 seconds
TestRunner_sco-tester PASS 15.26 seconds
TestRunner_ioctl-tester PASS 8.16 seconds
TestRunner_mesh-tester PASS 6.21 seconds
TestRunner_smp-tester PASS 7.00 seconds
TestRunner_userchan-tester PASS 10.04 seconds
IncrementalBuild PASS 28.81 seconds
Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
hci: fix double free in hci_req_sync
WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit description?)
#129:
6.10.0-rc4-00217-g35bb670d65fc-dirty #22 Hardware name: linux,dummy-virt (DT)
CHECK: Macro argument reuse 'hdev' - possible side-effects?
#304: FILE: net/bluetooth/hci_request.h:31:
+#define hci_req_skb_release_and_set(hdev, val) \
+({ \
+ if (hdev->req_skb) { \
+ spin_lock(&hdev->req_skb_lock); \
+ kfree_skb(hdev->req_skb); \
+ hdev->req_skb = val; \
+ spin_unlock(&hdev->req_skb_lock); \
+ } \
+})
CHECK: Macro argument 'hdev' may be better as '(hdev)' to avoid precedence issues
#304: FILE: net/bluetooth/hci_request.h:31:
+#define hci_req_skb_release_and_set(hdev, val) \
+({ \
+ if (hdev->req_skb) { \
+ spin_lock(&hdev->req_skb_lock); \
+ kfree_skb(hdev->req_skb); \
+ hdev->req_skb = val; \
+ spin_unlock(&hdev->req_skb_lock); \
+ } \
+})
total: 0 errors, 1 warnings, 2 checks, 78 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/13718597.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
Desc: Run gitlint
Output:
hci: fix double free in hci_req_sync
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
31: B1 Line exceeds max length (103>80): "BUG: KASAN: slab-use-after-free in skb_release_data+0x7d8/0x8a0 home/paran/linux/net/core/skbuff.c:1119"
44: B1 Line exceeds max length (86>80): " __asan_report_store1_noabort+0x44/0x60 home/paran/linux/mm/kasan/report_generic.c:383"
61: B1 Line exceeds max length (88>80): " el0_svc_common.constprop.0+0x2d4/0x3e8 home/paran/linux/arch/arm64/kernel/syscall.c:133"
64: B1 Line exceeds max length (87>80): " el0t_64_sync_handler+0x120/0x130 home/paran/linux/arch/arm64/kernel/entry-common.c:730"
139: B2 Line has trailing whitespace: "Since applying our patch, we have repeatedly run the same tests in "
142: B1 Line exceeds max length (122>80): "Link: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=89a32741f4217856066c198a4a7267bcdd1edd67"
##############################
Test: SubjectPrefix - FAIL
Desc: Check subject contains "Bluetooth" prefix
Output:
"Bluetooth: " prefix is not specified in the subject
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 492, Passed: 489 (99.4%), Failed: 1, Not Run: 2
Failed Test Cases
LL Privacy - Add Device 4 (2 Devices to AL) Failed 0.163 seconds
---
Regards,
Linux Bluetooth
… > +++ b/net/bluetooth/hci_request.c > @@ -106,8 +106,7 @@ void hci_req_sync_complete(struct hci_dev *hdev, u8 result, u16 opcode, > hdev->req_result = result; > hdev->req_status = HCI_REQ_DONE; > if (skb) { > - kfree_skb(hdev->req_skb); > - hdev->req_skb = skb_get(skb); > + hci_req_skb_release_and_set(hdev, skb_get(skb)); > } > wake_up_interruptible(&hdev->req_wait_q); … How do you think about to omit any curly brackets here? … > +++ b/net/bluetooth/hci_request.h > @@ -28,6 +28,15 @@ > > #define hci_req_sync_lock(hdev) mutex_lock(&hdev->req_lock) > #define hci_req_sync_unlock(hdev) mutex_unlock(&hdev->req_lock) > +#define hci_req_skb_release_and_set(hdev, val) \ > +({ \ > + if (hdev->req_skb) { \ > + spin_lock(&hdev->req_skb_lock); \ > + kfree_skb(hdev->req_skb); \ > + hdev->req_skb = val; \ > + spin_unlock(&hdev->req_skb_lock); \ > + } \ > +}) … * Do you expect that any data synchronisation should be performed for the shown pointer check? * Can it eventually matter to implement such a macro with a statement like “guard(spinlock)(&hdev->req_skb_lock);”? https://elixir.bootlin.com/linux/v6.10-rc6/source/include/linux/spinlock.h#L561 Regards, Markus
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h index c43716edf205..8b95061f063b 100644 --- a/include/net/bluetooth/hci_core.h +++ b/include/net/bluetooth/hci_core.h @@ -519,6 +519,7 @@ struct hci_dev { struct sk_buff *recv_event; struct mutex req_lock; + spinlock_t req_skb_lock; wait_queue_head_t req_wait_q; __u32 req_status; __u32 req_result; diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index dd3b0f501018..138a6b19894d 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -2572,6 +2572,7 @@ struct hci_dev *hci_alloc_dev_priv(int sizeof_priv) mutex_init(&hdev->lock); mutex_init(&hdev->req_lock); + spin_lock_init(&hdev->req_skb_lock); ida_init(&hdev->unset_handle_ida); diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c index efea25eb56ce..6a109c1ad359 100644 --- a/net/bluetooth/hci_request.c +++ b/net/bluetooth/hci_request.c @@ -106,8 +106,7 @@ void hci_req_sync_complete(struct hci_dev *hdev, u8 result, u16 opcode, hdev->req_result = result; hdev->req_status = HCI_REQ_DONE; if (skb) { - kfree_skb(hdev->req_skb); - hdev->req_skb = skb_get(skb); + hci_req_skb_release_and_set(hdev, skb_get(skb)); } wake_up_interruptible(&hdev->req_wait_q); } @@ -181,8 +180,7 @@ int __hci_req_sync(struct hci_dev *hdev, int (*func)(struct hci_request *req, break; } - kfree_skb(hdev->req_skb); - hdev->req_skb = NULL; + hci_req_skb_release_and_set(hdev, NULL); hdev->req_status = hdev->req_result = 0; bt_dev_dbg(hdev, "end: err %d", err); diff --git a/net/bluetooth/hci_request.h b/net/bluetooth/hci_request.h index c91f2838f542..6526c78443bc 100644 --- a/net/bluetooth/hci_request.h +++ b/net/bluetooth/hci_request.h @@ -28,6 +28,15 @@ #define hci_req_sync_lock(hdev) mutex_lock(&hdev->req_lock) #define hci_req_sync_unlock(hdev) mutex_unlock(&hdev->req_lock) +#define hci_req_skb_release_and_set(hdev, val) \ +({ \ + if (hdev->req_skb) { \ + spin_lock(&hdev->req_skb_lock); \ + kfree_skb(hdev->req_skb); \ + hdev->req_skb = val; \ + spin_unlock(&hdev->req_skb_lock); \ + } \ +}) struct hci_request { struct hci_dev *hdev; diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c index a8a7d2b36870..25c8d858c82e 100644 --- a/net/bluetooth/hci_sync.c +++ b/net/bluetooth/hci_sync.c @@ -33,8 +33,7 @@ static void hci_cmd_sync_complete(struct hci_dev *hdev, u8 result, u16 opcode, hdev->req_status = HCI_REQ_DONE; /* Free the request command so it is not used as response */ - kfree_skb(hdev->req_skb); - hdev->req_skb = NULL; + hci_req_skb_release_and_set(hdev, NULL); if (skb) { struct sock *sk = hci_skb_sk(skb); @@ -4935,10 +4934,7 @@ int hci_dev_open_sync(struct hci_dev *hdev) hdev->sent_cmd = NULL; } - if (hdev->req_skb) { - kfree_skb(hdev->req_skb); - hdev->req_skb = NULL; - } + hci_req_skb_release_and_set(hdev, NULL); clear_bit(HCI_RUNNING, &hdev->flags); hci_sock_dev_event(hdev, HCI_DEV_CLOSE); @@ -5100,10 +5096,7 @@ int hci_dev_close_sync(struct hci_dev *hdev) } /* Drop last request */ - if (hdev->req_skb) { - kfree_skb(hdev->req_skb); - hdev->req_skb = NULL; - } + hci_req_skb_release_and_set(hdev, NULL); clear_bit(HCI_RUNNING, &hdev->flags); hci_sock_dev_event(hdev, HCI_DEV_CLOSE);
The approach taken to address the 'CVE-2024-35978' introduced another double-free vulnerability. commit 45d355a926ab ("Bluetooth: Fix memory leak in hci_req_sync_complete()") 'hdev->req_skb' double free scenario: cpu1 cpu2 ==== ==== sock_ioctl sock_do_ioctl hci_sock_ioctl hci_dev_cmd hci_req_sync __hci_req_sync hci_rx_work kfree_skb(hdev->req_skb) hci_event_packet (sleep) hci_req_sync_complete \__ Longer times, kfree_skb(hdev->req_skb) reproduce well hdev->req_skb = NULL The longer cpu1 sleep in '__hci_req_sync', the more reproducible it is. We've tested it by inserting the 'msleep()' function, and it's frequently at 1000ms, and It has been consistently reproducible at 2000ms. We confirmed the detection with various workloads that cause CPU1 to sleep. The call trace below is one of the KASAN has seen. Bluetooth: hci0: unexpected cc 0x0c38 length: 249 > 2 ================================================================== BUG: KASAN: slab-use-after-free in skb_release_data+0x7d8/0x8a0 home/paran/linux/net/core/skbuff.c:1119 Write of size 1 at addr ffff00000947d3fe by task syz-executor.2/2010 CPU: 1 PID: 2010 Comm: syz-executor.2 Not tainted 6.10.0-rc4-00217-g35bb670d65fc-dirty #22 Hardware name: linux,dummy-virt (DT) Call trace: dump_backtrace+0x318/0x348 home/paran/linux/arch/arm64/kernel/stacktrace.c:317 show_stack+0x4c/0x80 home/paran/linux/arch/arm64/kernel/stacktrace.c:324 __dump_stack home/paran/linux/lib/dump_stack.c:88 [inline] dump_stack_lvl+0x214/0x328 home/paran/linux/lib/dump_stack.c:114 print_address_description home/paran/linux/mm/kasan/report.c:377 [inline] print_report+0x2ac/0x948 home/paran/linux/mm/kasan/report.c:488 kasan_report+0xc8/0x148 home/paran/linux/mm/kasan/report.c:601 __asan_report_store1_noabort+0x44/0x60 home/paran/linux/mm/kasan/report_generic.c:383 skb_release_data+0x7d8/0x8a0 home/paran/linux/net/core/skbuff.c:1119 skb_release_all+0x80/0xe0 home/paran/linux/net/core/skbuff.c:1173 __kfree_skb home/paran/linux/net/core/skbuff.c:1187 [inline] kfree_skb_reason+0x138/0x3a8 home/paran/linux/net/core/skbuff.c:1223 __hci_req_sync+0x404/0x948 [bluetooth] hci_req_sync+0xc0/0x138 [bluetooth] hci_dev_cmd+0x33c/0xc18 [bluetooth] hci_sock_ioctl+0x800/0xb68 [bluetooth] sock_do_ioctl+0xfc/0x2e0 home/paran/linux/net/socket.c:1222 sock_ioctl+0x62c/0xab0 home/paran/linux/net/socket.c:1341 vfs_ioctl+0x90/0x140 home/paran/linux/fs/ioctl.c:51 __do_sys_ioctl home/paran/linux/fs/ioctl.c:907 [inline] __se_sys_ioctl home/paran/linux/fs/ioctl.c:893 [inline] __arm64_sys_ioctl+0x218/0x268 home/paran/linux/fs/ioctl.c:893 __invoke_syscall home/paran/linux/arch/arm64/kernel/syscall.c:34 [inline] invoke_syscall+0xdc/0x460 home/paran/linux/arch/arm64/kernel/syscall.c:48 el0_svc_common.constprop.0+0x2d4/0x3e8 home/paran/linux/arch/arm64/kernel/syscall.c:133 do_el0_svc+0x60/0x98 home/paran/linux/arch/arm64/kernel/syscall.c:152 el0_svc+0xc4/0x240 home/paran/linux/arch/arm64/kernel/entry-common.c:712 el0t_64_sync_handler+0x120/0x130 home/paran/linux/arch/arm64/kernel/entry-common.c:730 el0t_64_sync+0x190/0x198 home/paran/linux/arch/arm64/kernel/entry.S:598 Allocated by task 577: kasan_save_stack+0x48/0x90 home/paran/linux/mm/kasan/common.c:47 kasan_save_track+0x38/0x60 home/paran/linux/mm/kasan/common.c:68 kasan_save_alloc_info+0x64/0xc0 home/paran/linux/mm/kasan/generic.c:565 unpoison_slab_object home/paran/linux/mm/kasan/common.c:312 [inline] __kasan_slab_alloc+0x100/0x110 home/paran/linux/mm/kasan/common.c:338 kasan_slab_alloc home/paran/linux/./include/linux/kasan.h:201 [inline] slab_post_alloc_hook home/paran/linux/mm/slub.c:3941 [inline] slab_alloc_node home/paran/linux/mm/slub.c:4001 [inline] kmem_cache_alloc_noprof+0x2e8/0x630 home/paran/linux/mm/slub.c:4008 skb_clone+0x1a4/0x4d0 home/paran/linux/net/core/skbuff.c:2052 hci_cmd_work+0x78c/0x868 [bluetooth] process_one_work home/paran/linux/kernel/workqueue.c:3231 [inline] process_scheduled_works+0x9fc/0x1d98 home/paran/linux/kernel/workqueue.c:3312 worker_thread+0x57c/0xf98 home/paran/linux/kernel/workqueue.c:3393 kthread+0x3c8/0x478 home/paran/linux/kernel/kthread.c:389 ret_from_fork+0x10/0x20 home/paran/linux/arch/arm64/kernel/entry.S:860 Freed by task 577: kasan_save_stack+0x48/0x90 home/paran/linux/mm/kasan/common.c:47 kasan_save_track+0x38/0x60 home/paran/linux/mm/kasan/common.c:68 kasan_save_free_info+0x64/0xf0 home/paran/linux/mm/kasan/generic.c:579 poison_slab_object+0x168/0x270 home/paran/linux/mm/kasan/common.c:240 __kasan_slab_free+0x34/0xa0 home/paran/linux/mm/kasan/common.c:256 kasan_slab_free home/paran/linux/./include/linux/kasan.h:184 [inline] slab_free_hook home/paran/linux/mm/slub.c:2196 [inline] slab_free home/paran/linux/mm/slub.c:4437 [inline] kmem_cache_free+0x20c/0x670 home/paran/linux/mm/slub.c:4512 kfree_skbmem+0x2b0/0x390 home/paran/linux/net/core/skbuff.c:1131 __kfree_skb home/paran/linux/net/core/skbuff.c:1188 [inline] kfree_skb_reason+0x14c/0x3a8 home/paran/linux/net/core/skbuff.c:1223 hci_req_sync_complete+0x114/0x308 [bluetooth] hci_event_packet+0xa10/0x12a0 [bluetooth] hci_rx_work+0x4d8/0xa80 [bluetooth] process_one_work home/paran/linux/kernel/workqueue.c:3231 [inline] process_scheduled_works+0x9fc/0x1d98 home/paran/linux/kernel/workqueue.c:3312 worker_thread+0x57c/0xf98 home/paran/linux/kernel/workqueue.c:3393 kthread+0x3c8/0x478 home/paran/linux/kernel/kthread.c:389 ret_from_fork+0x10/0x20 home/paran/linux/arch/arm64/kernel/entry.S:860 The buggy address belongs to the object at ffff00000947d380 which belongs to the cache skbuff_head_cache of size 232 The buggy address is located 126 bytes inside of freed 232-byte region [ffff00000947d380, ffff00000947d468) The buggy address belongs to the physical page: page: refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x4947c head: order:1 mapcount:0 entire_mapcount:0 nr_pages_mapped:0 pincount:0 memcg:ffff0000222a73b1 flags: 0x3fffe0000000040(head|node=0|zone=0|lastcpupid=0x1ffff) page_type: 0xffffefff(slab) raw: 03fffe0000000040 ffff0000133a41c0 fffffdffc0e3a090 fffffdffc1069290 raw: 0000000000000000 0000000000120012 00000001ffffefff ffff0000222a73b1 head: 03fffe0000000040 ffff0000133a41c0 fffffdffc0e3a090 fffffdffc1069290 head: 0000000000000000 0000000000120012 00000001ffffefff ffff0000222a73b1 head: 03fffe0000000001 fffffdffc0251f01 ffffffffffffffff 0000000000000000 head: 0000000000000002 0000000000000000 00000000ffffffff 0000000000000000 page dumped because: kasan: bad access detected Memory state around the buggy address: ffff00000947d280: 00 00 00 00 00 fc fc fc fc fc fc fc fc fc fc fc ffff00000947d300: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc >ffff00000947d380: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb ^ ffff00000947d400: fb fb fb fb fb fb fb fb fb fb fb fb fb fc fc fc ffff00000947d480: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc ================================================================== We considered using the "hci_req_sync_lock" mutex, but concluded it wasn't a good solution due to the increased sleep intervals causing this issue. Instead, we introduced a spinlock member on 'struct hci_dev'. Since applying our patch, we have repeatedly run the same tests in the syzkaller without encountering any issues. Link: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=89a32741f4217856066c198a4a7267bcdd1edd67 Fixes: 45d355a926ab ("Bluetooth: Fix memory leak in hci_req_sync_complete()") Signed-off-by: Levi Yun <yeoreum.yun@arm.com> Signed-off-by: Yunseong Kim <yskelg@gmail.com> --- include/net/bluetooth/hci_core.h | 1 + net/bluetooth/hci_core.c | 1 + net/bluetooth/hci_request.c | 6 ++---- net/bluetooth/hci_request.h | 9 +++++++++ net/bluetooth/hci_sync.c | 13 +++---------- 5 files changed, 16 insertions(+), 14 deletions(-)