| Message ID | 20240731190330.1619718-1-cascardo@igalia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Bluetooth: btmtk: avoid UAF in btmtk_process_coredump | expand |
On Wed, Jul 31, 2024 at 04:03:30PM -0300, Thadeu Lima de Souza Cascardo wrote: > hci_devcd_append may lead to the release of the skb, so it cannot be > accessed once it is called. > > ================================================================== > BUG: KASAN: slab-use-after-free in btmtk_process_coredump+0x2a7/0x2d0 [btmtk] > Read of size 4 at addr ffff888033cfabb0 by task kworker/0:3/82 > > CPU: 0 PID: 82 Comm: kworker/0:3 Tainted: G U 6.6.40-lockdep-03464-g1d8b4eb3060e #1 b0b3c1cc0c842735643fb411799d97921d1f688c > Hardware name: Google Yaviks_Ufs/Yaviks_Ufs, BIOS Google_Yaviks_Ufs.15217.552.0 05/07/2024 > Workqueue: events btusb_rx_work [btusb] > Call Trace: > <TASK> > dump_stack_lvl+0xfd/0x150 > print_report+0x131/0x780 > kasan_report+0x177/0x1c0 > btmtk_process_coredump+0x2a7/0x2d0 [btmtk 03edd567dd71a65958807c95a65db31d433e1d01] > btusb_recv_acl_mtk+0x11c/0x1a0 [btusb 675430d1e87c4f24d0c1f80efe600757a0f32bec] > btusb_rx_work+0x9e/0xe0 [btusb 675430d1e87c4f24d0c1f80efe600757a0f32bec] > worker_thread+0xe44/0x2cc0 > kthread+0x2ff/0x3a0 > ret_from_fork+0x51/0x80 > ret_from_fork_asm+0x1b/0x30 > </TASK> > > Allocated by task 82: > stack_trace_save+0xdc/0x190 > kasan_set_track+0x4e/0x80 > __kasan_slab_alloc+0x4e/0x60 > kmem_cache_alloc+0x19f/0x360 > skb_clone+0x132/0xf70 > btusb_recv_acl_mtk+0x104/0x1a0 [btusb] > btusb_rx_work+0x9e/0xe0 [btusb] > worker_thread+0xe44/0x2cc0 > kthread+0x2ff/0x3a0 > ret_from_fork+0x51/0x80 > ret_from_fork_asm+0x1b/0x30 > > Freed by task 1733: > stack_trace_save+0xdc/0x190 > kasan_set_track+0x4e/0x80 > kasan_save_free_info+0x28/0xb0 > ____kasan_slab_free+0xfd/0x170 > kmem_cache_free+0x183/0x3f0 > hci_devcd_rx+0x91a/0x2060 [bluetooth] > worker_thread+0xe44/0x2cc0 > kthread+0x2ff/0x3a0 > ret_from_fork+0x51/0x80 > ret_from_fork_asm+0x1b/0x30 > > The buggy address belongs to the object at ffff888033cfab40 > which belongs to the cache skbuff_head_cache of size 232 > The buggy address is located 112 bytes inside of > freed 232-byte region [ffff888033cfab40, ffff888033cfac28) > > The buggy address belongs to the physical page: > page:00000000a174ba93 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x33cfa > head:00000000a174ba93 order:1 entire_mapcount:0 nr_pages_mapped:0 pincount:0 > anon flags: 0x4000000000000840(slab|head|zone=1) > page_type: 0xffffffff() > raw: 4000000000000840 ffff888100848a00 0000000000000000 0000000000000001 > raw: 0000000000000000 0000000080190019 00000001ffffffff 0000000000000000 > page dumped because: kasan: bad access detected > > Memory state around the buggy address: > ffff888033cfaa80: fb fb fb fb fb fb fb fb fb fb fb fb fb fc fc fc > ffff888033cfab00: fc fc fc fc fc fc fc fc fa fb fb fb fb fb fb fb > >ffff888033cfab80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > ^ > ffff888033cfac00: fb fb fb fb fb fc fc fc fc fc fc fc fc fc fc fc > ffff888033cfac80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > ================================================================== > > Check if we need to call hci_devcd_complete before calling > hci_devcd_append. That requires that we check data->cd_info.cnt >= > MTK_COREDUMP_NUM instead of data->cd_info.cnt > MTK_COREDUMP_NUM, as we > increment data->cd_info.cnt only once the call to hci_devcd_append > succeeds. > > Fixes: 0b7015132878 ("Bluetooth: btusb: mediatek: add MediaTek devcoredump support") > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com> > --- > drivers/bluetooth/btmtk.c | 20 ++++++++++++-------- > 1 file changed, 12 insertions(+), 8 deletions(-) > > diff --git a/drivers/bluetooth/btmtk.c b/drivers/bluetooth/btmtk.c > index b7c348687a77..46f605249df7 100644 > --- a/drivers/bluetooth/btmtk.c > +++ b/drivers/bluetooth/btmtk.c > @@ -395,6 +395,7 @@ int btmtk_process_coredump(struct hci_dev *hdev, struct sk_buff *skb) > { > struct btmtk_data *data = hci_get_priv(hdev); > int err; > + bool complete = false; > > if (!IS_ENABLED(CONFIG_DEV_COREDUMP)) { > kfree_skb(skb); > @@ -416,19 +417,22 @@ int btmtk_process_coredump(struct hci_dev *hdev, struct sk_buff *skb) > fallthrough; > case HCI_DEVCOREDUMP_ACTIVE: > default: > + /* Mediatek coredump data would be more than MTK_COREDUMP_NUM */ > + if (data->cd_info.cnt >= MTK_COREDUMP_NUM && > + skb->len > MTK_COREDUMP_END_LEN) > + if (!memcmp((char *)&skb->data[skb->len - MTK_COREDUMP_END_LEN], > + MTK_COREDUMP_END, MTK_COREDUMP_END_LEN - 1)) > + complete = true; > + > err = hci_devcd_append(hdev, skb); > if (err < 0) > break; > data->cd_info.cnt++; > > - /* Mediatek coredump data would be more than MTK_COREDUMP_NUM */ > - if (data->cd_info.cnt > MTK_COREDUMP_NUM && > - skb->len > MTK_COREDUMP_END_LEN) > - if (!memcmp((char *)&skb->data[skb->len - MTK_COREDUMP_END_LEN], > - MTK_COREDUMP_END, MTK_COREDUMP_END_LEN - 1)) { > - bt_dev_info(hdev, "Mediatek coredump end"); > - hci_devcd_complete(hdev); > - } > + if (complete) { > + bt_dev_info(hdev, "Mediatek coredump end"); > + hci_devcd_complete(hdev); > + } > > break; > } > -- > 2.34.1 > > Hi, anyone can help review this? Thanks. Cascardo.
On Thu, 2024-08-22 at 06:57 -0300, Thadeu Lima de Souza Cascardo wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On Wed, Jul 31, 2024 at 04:03:30PM -0300, Thadeu Lima de Souza > Cascardo wrote: > > hci_devcd_append may lead to the release of the skb, so it cannot > be > > accessed once it is called. > > > > ================================================================== > > BUG: KASAN: slab-use-after-free in > btmtk_process_coredump+0x2a7/0x2d0 [btmtk] > > Read of size 4 at addr ffff888033cfabb0 by task kworker/0:3/82 > > > > CPU: 0 PID: 82 Comm: kworker/0:3 Tainted: > G U 6.6.40-lockdep-03464-g1d8b4eb3060e #1 > b0b3c1cc0c842735643fb411799d97921d1f688c > > Hardware name: Google Yaviks_Ufs/Yaviks_Ufs, BIOS > Google_Yaviks_Ufs.15217.552.0 05/07/2024 > > Workqueue: events btusb_rx_work [btusb] > > Call Trace: > > <TASK> > > dump_stack_lvl+0xfd/0x150 > > print_report+0x131/0x780 > > kasan_report+0x177/0x1c0 > > btmtk_process_coredump+0x2a7/0x2d0 [btmtk > 03edd567dd71a65958807c95a65db31d433e1d01] > > btusb_recv_acl_mtk+0x11c/0x1a0 [btusb > 675430d1e87c4f24d0c1f80efe600757a0f32bec] > > btusb_rx_work+0x9e/0xe0 [btusb > 675430d1e87c4f24d0c1f80efe600757a0f32bec] > > worker_thread+0xe44/0x2cc0 > > kthread+0x2ff/0x3a0 > > ret_from_fork+0x51/0x80 > > ret_from_fork_asm+0x1b/0x30 > > </TASK> > > > > Allocated by task 82: > > stack_trace_save+0xdc/0x190 > > kasan_set_track+0x4e/0x80 > > __kasan_slab_alloc+0x4e/0x60 > > kmem_cache_alloc+0x19f/0x360 > > skb_clone+0x132/0xf70 > > btusb_recv_acl_mtk+0x104/0x1a0 [btusb] > > btusb_rx_work+0x9e/0xe0 [btusb] > > worker_thread+0xe44/0x2cc0 > > kthread+0x2ff/0x3a0 > > ret_from_fork+0x51/0x80 > > ret_from_fork_asm+0x1b/0x30 > > > > Freed by task 1733: > > stack_trace_save+0xdc/0x190 > > kasan_set_track+0x4e/0x80 > > kasan_save_free_info+0x28/0xb0 > > ____kasan_slab_free+0xfd/0x170 > > kmem_cache_free+0x183/0x3f0 > > hci_devcd_rx+0x91a/0x2060 [bluetooth] > > worker_thread+0xe44/0x2cc0 > > kthread+0x2ff/0x3a0 > > ret_from_fork+0x51/0x80 > > ret_from_fork_asm+0x1b/0x30 > > > > The buggy address belongs to the object at ffff888033cfab40 > > which belongs to the cache skbuff_head_cache of size 232 > > The buggy address is located 112 bytes inside of > > freed 232-byte region [ffff888033cfab40, ffff888033cfac28) > > > > The buggy address belongs to the physical page: > > page:00000000a174ba93 refcount:1 mapcount:0 > mapping:0000000000000000 index:0x0 pfn:0x33cfa > > head:00000000a174ba93 order:1 entire_mapcount:0 nr_pages_mapped:0 > pincount:0 > > anon flags: 0x4000000000000840(slab|head|zone=1) > > page_type: 0xffffffff() > > raw: 4000000000000840 ffff888100848a00 0000000000000000 > 0000000000000001 > > raw: 0000000000000000 0000000080190019 00000001ffffffff > 0000000000000000 > > page dumped because: kasan: bad access detected > > > > Memory state around the buggy address: > > ffff888033cfaa80: fb fb fb fb fb fb fb fb fb fb fb fb fb fc fc fc > > ffff888033cfab00: fc fc fc fc fc fc fc fc fa fb fb fb fb fb fb fb > > >ffff888033cfab80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > > ^ > > ffff888033cfac00: fb fb fb fb fb fc fc fc fc fc fc fc fc fc fc fc > > ffff888033cfac80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb > > ================================================================== > > > > Check if we need to call hci_devcd_complete before calling > > hci_devcd_append. That requires that we check data->cd_info.cnt >= > > MTK_COREDUMP_NUM instead of data->cd_info.cnt > MTK_COREDUMP_NUM, > as we > > increment data->cd_info.cnt only once the call to hci_devcd_append > > succeeds. > > > > Fixes: 0b7015132878 ("Bluetooth: btusb: mediatek: add MediaTek > devcoredump support") > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com> > > --- > > drivers/bluetooth/btmtk.c | 20 ++++++++++++-------- > > 1 file changed, 12 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/bluetooth/btmtk.c b/drivers/bluetooth/btmtk.c > > index b7c348687a77..46f605249df7 100644 > > --- a/drivers/bluetooth/btmtk.c > > +++ b/drivers/bluetooth/btmtk.c > > @@ -395,6 +395,7 @@ int btmtk_process_coredump(struct hci_dev > *hdev, struct sk_buff *skb) > > { > > struct btmtk_data *data = hci_get_priv(hdev); > > int err; > > +bool complete = false; > > > > if (!IS_ENABLED(CONFIG_DEV_COREDUMP)) { > > kfree_skb(skb); > > @@ -416,19 +417,22 @@ int btmtk_process_coredump(struct hci_dev > *hdev, struct sk_buff *skb) > > fallthrough; > > case HCI_DEVCOREDUMP_ACTIVE: > > default: > > +/* Mediatek coredump data would be more than MTK_COREDUMP_NUM */ > > +if (data->cd_info.cnt >= MTK_COREDUMP_NUM && > > + skb->len > MTK_COREDUMP_END_LEN) > > +if (!memcmp((char *)&skb->data[skb->len - MTK_COREDUMP_END_LEN], > > + MTK_COREDUMP_END, MTK_COREDUMP_END_LEN - 1)) > > +complete = true; > > + > > err = hci_devcd_append(hdev, skb); > > if (err < 0) > > break; > > data->cd_info.cnt++; > > > > -/* Mediatek coredump data would be more than MTK_COREDUMP_NUM */ > > -if (data->cd_info.cnt > MTK_COREDUMP_NUM && > > - skb->len > MTK_COREDUMP_END_LEN) > > -if (!memcmp((char *)&skb->data[skb->len - MTK_COREDUMP_END_LEN], > > - MTK_COREDUMP_END, MTK_COREDUMP_END_LEN - 1)) { > > -bt_dev_info(hdev, "Mediatek coredump end"); > > -hci_devcd_complete(hdev); > > -} > > +if (complete) { > > +bt_dev_info(hdev, "Mediatek coredump end"); > > +hci_devcd_complete(hdev); > > +} > > > > break; > > } > > -- > > 2.34.1 > > > > > > Hi, anyone can help review this? > > Thanks. Hi Thadeu, Sorry for late reply. I've also verified on the platform which supports devcoredump feature that Bluetooth can recover after triggerring devcoredump. LGTM. It may need maintainer's approval for the next step. Thanks a lot! Chris > Cascardo.
diff --git a/drivers/bluetooth/btmtk.c b/drivers/bluetooth/btmtk.c index b7c348687a77..46f605249df7 100644 --- a/drivers/bluetooth/btmtk.c +++ b/drivers/bluetooth/btmtk.c @@ -395,6 +395,7 @@ int btmtk_process_coredump(struct hci_dev *hdev, struct sk_buff *skb) { struct btmtk_data *data = hci_get_priv(hdev); int err; + bool complete = false; if (!IS_ENABLED(CONFIG_DEV_COREDUMP)) { kfree_skb(skb); @@ -416,19 +417,22 @@ int btmtk_process_coredump(struct hci_dev *hdev, struct sk_buff *skb) fallthrough; case HCI_DEVCOREDUMP_ACTIVE: default: + /* Mediatek coredump data would be more than MTK_COREDUMP_NUM */ + if (data->cd_info.cnt >= MTK_COREDUMP_NUM && + skb->len > MTK_COREDUMP_END_LEN) + if (!memcmp((char *)&skb->data[skb->len - MTK_COREDUMP_END_LEN], + MTK_COREDUMP_END, MTK_COREDUMP_END_LEN - 1)) + complete = true; + err = hci_devcd_append(hdev, skb); if (err < 0) break; data->cd_info.cnt++; - /* Mediatek coredump data would be more than MTK_COREDUMP_NUM */ - if (data->cd_info.cnt > MTK_COREDUMP_NUM && - skb->len > MTK_COREDUMP_END_LEN) - if (!memcmp((char *)&skb->data[skb->len - MTK_COREDUMP_END_LEN], - MTK_COREDUMP_END, MTK_COREDUMP_END_LEN - 1)) { - bt_dev_info(hdev, "Mediatek coredump end"); - hci_devcd_complete(hdev); - } + if (complete) { + bt_dev_info(hdev, "Mediatek coredump end"); + hci_devcd_complete(hdev); + } break; }