Hi,
On Mon, Jul 1, 2024 at 3:45 PM Yunseong Kim <yskelg@xxxxxxxxx> 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@xxxxxxx>
> Signed-off-by: Yunseong Kim <yskelg@xxxxxxxxx>
> ---
> 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
>
--
Luiz Augusto von Dentz
