| Message ID | 20240624144911.3817479-1-luiz.dentz@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] Bluetooth: Fix double free in hci_req_sync_complete | expand |
Hello: This patch was applied to bluetooth/bluetooth-next.git (master) by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>: On Mon, 24 Jun 2024 10:49:11 -0400 you wrote: > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > This fixes the following race: > > cpu1 cpu2 > ==== ==== > sock_ioctl > sock_do_ioctl > hci_sock_ioctl > hci_rx_work hci_dev_cmd > hci_event_packet hci_req_sync > req_complete_skb __hci_req_sync > hci_req_sync_complete > > [...] Here is the summary with links: - [v1] Bluetooth: Fix double free in hci_req_sync_complete https://git.kernel.org/bluetooth/bluetooth-next/c/bdde736f4d2d You are awesome, thank you!
Hi Luiz, ma, 2024-06-24 kello 10:49 -0400, Luiz Augusto von Dentz kirjoitti: > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > This fixes the following race: > > cpu1 cpu2 > ==== ==== > sock_ioctl > sock_do_ioctl > hci_sock_ioctl > hci_rx_work hci_dev_cmd > hci_event_packet hci_req_sync > req_complete_skb __hci_req_sync > hci_req_sync_complete > > If hci_rx_work executes before __hci_req_sync releases req_skb, everything > is normal, otherwise it will result in double free of req_skb. > > This replaces the usage of hci_req_sync with hci_cmd_sync_status. > > Fixes: 45d355a926ab ("Bluetooth: Fix memory leak in hci_req_sync_complete()") > Reported-and-tested-by: syzbot+35ebc808442df6420eae@syzkaller.appspotmail.com > Closes: https://syzkaller.appspot.com/bug?extid=35ebc808442df6420eae > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > --- > include/net/bluetooth/hci_sync.h | 2 + > net/bluetooth/hci_core.c | 72 ++++++++------------------------ > net/bluetooth/hci_sync.c | 13 ++++++ > 3 files changed, 33 insertions(+), 54 deletions(-) > > diff --git a/include/net/bluetooth/hci_sync.h b/include/net/bluetooth/hci_sync.h > index b49057bbdf23..20168732f20e 100644 > --- a/include/net/bluetooth/hci_sync.h > +++ b/include/net/bluetooth/hci_sync.h > @@ -38,6 +38,8 @@ int __hci_cmd_sync_status(struct hci_dev *hdev, u16 opcode, u32 plen, > int __hci_cmd_sync_status_sk(struct hci_dev *hdev, u16 opcode, u32 plen, > const void *param, u8 event, u32 timeout, > struct sock *sk); > +int hci_cmd_sync_status(struct hci_dev *hdev, u16 opcode, u32 plen, > + const void *param, u32 timeout); In this patch, this function is unused? > > void hci_cmd_sync_init(struct hci_dev *hdev); > void hci_cmd_sync_clear(struct hci_dev *hdev); > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c > index d541cbb991eb..144e85ebfbdb 100644 > --- a/net/bluetooth/hci_core.c > +++ b/net/bluetooth/hci_core.c > @@ -63,50 +63,6 @@ DEFINE_MUTEX(hci_cb_list_lock); > /* HCI ID Numbering */ > static DEFINE_IDA(hci_index_ida); > > -static int hci_scan_req(struct hci_request *req, unsigned long opt) > -{ > - __u8 scan = opt; > - > - BT_DBG("%s %x", req->hdev->name, scan); > - > - /* Inquiry and Page scans */ > - hci_req_add(req, HCI_OP_WRITE_SCAN_ENABLE, 1, &scan); > - return 0; > -} > - > -static int hci_auth_req(struct hci_request *req, unsigned long opt) > -{ > - __u8 auth = opt; > - > - BT_DBG("%s %x", req->hdev->name, auth); > - > - /* Authentication */ > - hci_req_add(req, HCI_OP_WRITE_AUTH_ENABLE, 1, &auth); > - return 0; > -} > - > -static int hci_encrypt_req(struct hci_request *req, unsigned long opt) > -{ > - __u8 encrypt = opt; > - > - BT_DBG("%s %x", req->hdev->name, encrypt); > - > - /* Encryption */ > - hci_req_add(req, HCI_OP_WRITE_ENCRYPT_MODE, 1, &encrypt); > - return 0; > -} > - > -static int hci_linkpol_req(struct hci_request *req, unsigned long opt) > -{ > - __le16 policy = cpu_to_le16(opt); > - > - BT_DBG("%s %x", req->hdev->name, policy); > - > - /* Default link policy */ > - hci_req_add(req, HCI_OP_WRITE_DEF_LINK_POLICY, 2, &policy); > - return 0; > -} > - > /* Get HCI device by index. > * Device is held on return. */ > struct hci_dev *hci_dev_get(int index) > @@ -735,6 +691,7 @@ int hci_dev_cmd(unsigned int cmd, void __user *arg) > { > struct hci_dev *hdev; > struct hci_dev_req dr; > + __le16 policy; > int err = 0; > > if (copy_from_user(&dr, arg, sizeof(dr))) > @@ -761,8 +718,8 @@ int hci_dev_cmd(unsigned int cmd, void __user *arg) > > switch (cmd) { > case HCISETAUTH: > - err = hci_req_sync(hdev, hci_auth_req, dr.dev_opt, > - HCI_INIT_TIMEOUT, NULL); > + err = __hci_cmd_sync_status(hdev, HCI_OP_WRITE_AUTH_ENABLE, > + 1, &dr.dev_opt, HCI_CMD_TIMEOUT); These probably were intended to use hci_cmd_sync_status that has the locking? > break; > > case HCISETENCRYPT: > @@ -773,19 +730,23 @@ int hci_dev_cmd(unsigned int cmd, void __user *arg) > > if (!test_bit(HCI_AUTH, &hdev->flags)) { > /* Auth must be enabled first */ > - err = hci_req_sync(hdev, hci_auth_req, dr.dev_opt, > - HCI_INIT_TIMEOUT, NULL); > + err = __hci_cmd_sync_status(hdev, > + HCI_OP_WRITE_AUTH_ENABLE, > + 1, &dr.dev_opt, > + HCI_CMD_TIMEOUT); > if (err) > break; > } > > - err = hci_req_sync(hdev, hci_encrypt_req, dr.dev_opt, > - HCI_INIT_TIMEOUT, NULL); > + err = __hci_cmd_sync_status(hdev, HCI_OP_WRITE_ENCRYPT_MODE, > + 1, &dr.dev_opt, > + HCI_CMD_TIMEOUT); > break; > > case HCISETSCAN: > - err = hci_req_sync(hdev, hci_scan_req, dr.dev_opt, > - HCI_INIT_TIMEOUT, NULL); > + err = __hci_cmd_sync_status(hdev, HCI_OP_WRITE_SCAN_ENABLE, > + 1, &dr.dev_opt, > + HCI_CMD_TIMEOUT); > > /* Ensure that the connectable and discoverable states > * get correctly modified as this was a non-mgmt change. > @@ -795,8 +756,11 @@ int hci_dev_cmd(unsigned int cmd, void __user *arg) > break; > > case HCISETLINKPOL: > - err = hci_req_sync(hdev, hci_linkpol_req, dr.dev_opt, > - HCI_INIT_TIMEOUT, NULL); > + policy = cpu_to_le16(dr.dev_opt); > + > + err = __hci_cmd_sync_status(hdev, HCI_OP_WRITE_DEF_LINK_POLICY, > + 2, &policy, > + HCI_CMD_TIMEOUT); > break; > > case HCISETLINKMODE: > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c > index eff648853ae1..ccad43f10415 100644 > --- a/net/bluetooth/hci_sync.c > +++ b/net/bluetooth/hci_sync.c > @@ -280,6 +280,19 @@ int __hci_cmd_sync_status(struct hci_dev *hdev, u16 opcode, u32 plen, > } > EXPORT_SYMBOL(__hci_cmd_sync_status); > > +int hci_cmd_sync_status(struct hci_dev *hdev, u16 opcode, u32 plen, > + const void *param, u32 timeout) > +{ > + int err; > + > + hci_req_sync_lock(hdev); > + err = __hci_cmd_sync_status(hdev, opcode, plen, param, timeout); > + hci_req_sync_unlock(hdev); > + > + return err; > +} > +EXPORT_SYMBOL(hci_cmd_sync_status); > + > static void hci_cmd_sync_work(struct work_struct *work) > { > struct hci_dev *hdev = container_of(work, struct hci_dev, cmd_sync_work);
Hi Pauli, On Sat, Jun 29, 2024 at 6:07 AM Pauli Virtanen <pav@iki.fi> wrote: > > Hi Luiz, > > ma, 2024-06-24 kello 10:49 -0400, Luiz Augusto von Dentz kirjoitti: > > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > > > This fixes the following race: > > > > cpu1 cpu2 > > ==== ==== > > sock_ioctl > > sock_do_ioctl > > hci_sock_ioctl > > hci_rx_work hci_dev_cmd > > hci_event_packet hci_req_sync > > req_complete_skb __hci_req_sync > > hci_req_sync_complete > > > > If hci_rx_work executes before __hci_req_sync releases req_skb, everything > > is normal, otherwise it will result in double free of req_skb. > > > > This replaces the usage of hci_req_sync with hci_cmd_sync_status. > > > > Fixes: 45d355a926ab ("Bluetooth: Fix memory leak in hci_req_sync_complete()") > > Reported-and-tested-by: syzbot+35ebc808442df6420eae@syzkaller.appspotmail.com > > Closes: https://syzkaller.appspot.com/bug?extid=35ebc808442df6420eae > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > --- > > include/net/bluetooth/hci_sync.h | 2 + > > net/bluetooth/hci_core.c | 72 ++++++++------------------------ > > net/bluetooth/hci_sync.c | 13 ++++++ > > 3 files changed, 33 insertions(+), 54 deletions(-) > > > > diff --git a/include/net/bluetooth/hci_sync.h b/include/net/bluetooth/hci_sync.h > > index b49057bbdf23..20168732f20e 100644 > > --- a/include/net/bluetooth/hci_sync.h > > +++ b/include/net/bluetooth/hci_sync.h > > @@ -38,6 +38,8 @@ int __hci_cmd_sync_status(struct hci_dev *hdev, u16 opcode, u32 plen, > > int __hci_cmd_sync_status_sk(struct hci_dev *hdev, u16 opcode, u32 plen, > > const void *param, u8 event, u32 timeout, > > struct sock *sk); > > +int hci_cmd_sync_status(struct hci_dev *hdev, u16 opcode, u32 plen, > > + const void *param, u32 timeout); > > In this patch, this function is unused? > > > > > void hci_cmd_sync_init(struct hci_dev *hdev); > > void hci_cmd_sync_clear(struct hci_dev *hdev); > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c > > index d541cbb991eb..144e85ebfbdb 100644 > > --- a/net/bluetooth/hci_core.c > > +++ b/net/bluetooth/hci_core.c > > @@ -63,50 +63,6 @@ DEFINE_MUTEX(hci_cb_list_lock); > > /* HCI ID Numbering */ > > static DEFINE_IDA(hci_index_ida); > > > > -static int hci_scan_req(struct hci_request *req, unsigned long opt) > > -{ > > - __u8 scan = opt; > > - > > - BT_DBG("%s %x", req->hdev->name, scan); > > - > > - /* Inquiry and Page scans */ > > - hci_req_add(req, HCI_OP_WRITE_SCAN_ENABLE, 1, &scan); > > - return 0; > > -} > > - > > -static int hci_auth_req(struct hci_request *req, unsigned long opt) > > -{ > > - __u8 auth = opt; > > - > > - BT_DBG("%s %x", req->hdev->name, auth); > > - > > - /* Authentication */ > > - hci_req_add(req, HCI_OP_WRITE_AUTH_ENABLE, 1, &auth); > > - return 0; > > -} > > - > > -static int hci_encrypt_req(struct hci_request *req, unsigned long opt) > > -{ > > - __u8 encrypt = opt; > > - > > - BT_DBG("%s %x", req->hdev->name, encrypt); > > - > > - /* Encryption */ > > - hci_req_add(req, HCI_OP_WRITE_ENCRYPT_MODE, 1, &encrypt); > > - return 0; > > -} > > - > > -static int hci_linkpol_req(struct hci_request *req, unsigned long opt) > > -{ > > - __le16 policy = cpu_to_le16(opt); > > - > > - BT_DBG("%s %x", req->hdev->name, policy); > > - > > - /* Default link policy */ > > - hci_req_add(req, HCI_OP_WRITE_DEF_LINK_POLICY, 2, &policy); > > - return 0; > > -} > > - > > /* Get HCI device by index. > > * Device is held on return. */ > > struct hci_dev *hci_dev_get(int index) > > @@ -735,6 +691,7 @@ int hci_dev_cmd(unsigned int cmd, void __user *arg) > > { > > struct hci_dev *hdev; > > struct hci_dev_req dr; > > + __le16 policy; > > int err = 0; > > > > if (copy_from_user(&dr, arg, sizeof(dr))) > > @@ -761,8 +718,8 @@ int hci_dev_cmd(unsigned int cmd, void __user *arg) > > > > switch (cmd) { > > case HCISETAUTH: > > - err = hci_req_sync(hdev, hci_auth_req, dr.dev_opt, > > - HCI_INIT_TIMEOUT, NULL); > > + err = __hci_cmd_sync_status(hdev, HCI_OP_WRITE_AUTH_ENABLE, > > + 1, &dr.dev_opt, HCI_CMD_TIMEOUT); > > These probably were intended to use hci_cmd_sync_status that has the > locking? Yep, looks like I used the wrong version, I will need to fix that, thanks for spotting it. > > break; > > > > case HCISETENCRYPT: > > @@ -773,19 +730,23 @@ int hci_dev_cmd(unsigned int cmd, void __user *arg) > > > > if (!test_bit(HCI_AUTH, &hdev->flags)) { > > /* Auth must be enabled first */ > > - err = hci_req_sync(hdev, hci_auth_req, dr.dev_opt, > > - HCI_INIT_TIMEOUT, NULL); > > + err = __hci_cmd_sync_status(hdev, > > + HCI_OP_WRITE_AUTH_ENABLE, > > + 1, &dr.dev_opt, > > + HCI_CMD_TIMEOUT); > > if (err) > > break; > > } > > > > - err = hci_req_sync(hdev, hci_encrypt_req, dr.dev_opt, > > - HCI_INIT_TIMEOUT, NULL); > > + err = __hci_cmd_sync_status(hdev, HCI_OP_WRITE_ENCRYPT_MODE, > > + 1, &dr.dev_opt, > > + HCI_CMD_TIMEOUT); > > break; > > > > case HCISETSCAN: > > - err = hci_req_sync(hdev, hci_scan_req, dr.dev_opt, > > - HCI_INIT_TIMEOUT, NULL); > > + err = __hci_cmd_sync_status(hdev, HCI_OP_WRITE_SCAN_ENABLE, > > + 1, &dr.dev_opt, > > + HCI_CMD_TIMEOUT); > > > > /* Ensure that the connectable and discoverable states > > * get correctly modified as this was a non-mgmt change. > > @@ -795,8 +756,11 @@ int hci_dev_cmd(unsigned int cmd, void __user *arg) > > break; > > > > case HCISETLINKPOL: > > - err = hci_req_sync(hdev, hci_linkpol_req, dr.dev_opt, > > - HCI_INIT_TIMEOUT, NULL); > > + policy = cpu_to_le16(dr.dev_opt); > > + > > + err = __hci_cmd_sync_status(hdev, HCI_OP_WRITE_DEF_LINK_POLICY, > > + 2, &policy, > > + HCI_CMD_TIMEOUT); > > break; > > > > case HCISETLINKMODE: > > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c > > index eff648853ae1..ccad43f10415 100644 > > --- a/net/bluetooth/hci_sync.c > > +++ b/net/bluetooth/hci_sync.c > > @@ -280,6 +280,19 @@ int __hci_cmd_sync_status(struct hci_dev *hdev, u16 opcode, u32 plen, > > } > > EXPORT_SYMBOL(__hci_cmd_sync_status); > > > > +int hci_cmd_sync_status(struct hci_dev *hdev, u16 opcode, u32 plen, > > + const void *param, u32 timeout) > > +{ > > + int err; > > + > > + hci_req_sync_lock(hdev); > > + err = __hci_cmd_sync_status(hdev, opcode, plen, param, timeout); > > + hci_req_sync_unlock(hdev); > > + > > + return err; > > +} > > +EXPORT_SYMBOL(hci_cmd_sync_status); > > + > > static void hci_cmd_sync_work(struct work_struct *work) > > { > > struct hci_dev *hdev = container_of(work, struct hci_dev, cmd_sync_work); > > -- > Pauli Virtanen
diff --git a/include/net/bluetooth/hci_sync.h b/include/net/bluetooth/hci_sync.h index b49057bbdf23..20168732f20e 100644 --- a/include/net/bluetooth/hci_sync.h +++ b/include/net/bluetooth/hci_sync.h @@ -38,6 +38,8 @@ int __hci_cmd_sync_status(struct hci_dev *hdev, u16 opcode, u32 plen, int __hci_cmd_sync_status_sk(struct hci_dev *hdev, u16 opcode, u32 plen, const void *param, u8 event, u32 timeout, struct sock *sk); +int hci_cmd_sync_status(struct hci_dev *hdev, u16 opcode, u32 plen, + const void *param, u32 timeout); void hci_cmd_sync_init(struct hci_dev *hdev); void hci_cmd_sync_clear(struct hci_dev *hdev); diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index d541cbb991eb..144e85ebfbdb 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -63,50 +63,6 @@ DEFINE_MUTEX(hci_cb_list_lock); /* HCI ID Numbering */ static DEFINE_IDA(hci_index_ida); -static int hci_scan_req(struct hci_request *req, unsigned long opt) -{ - __u8 scan = opt; - - BT_DBG("%s %x", req->hdev->name, scan); - - /* Inquiry and Page scans */ - hci_req_add(req, HCI_OP_WRITE_SCAN_ENABLE, 1, &scan); - return 0; -} - -static int hci_auth_req(struct hci_request *req, unsigned long opt) -{ - __u8 auth = opt; - - BT_DBG("%s %x", req->hdev->name, auth); - - /* Authentication */ - hci_req_add(req, HCI_OP_WRITE_AUTH_ENABLE, 1, &auth); - return 0; -} - -static int hci_encrypt_req(struct hci_request *req, unsigned long opt) -{ - __u8 encrypt = opt; - - BT_DBG("%s %x", req->hdev->name, encrypt); - - /* Encryption */ - hci_req_add(req, HCI_OP_WRITE_ENCRYPT_MODE, 1, &encrypt); - return 0; -} - -static int hci_linkpol_req(struct hci_request *req, unsigned long opt) -{ - __le16 policy = cpu_to_le16(opt); - - BT_DBG("%s %x", req->hdev->name, policy); - - /* Default link policy */ - hci_req_add(req, HCI_OP_WRITE_DEF_LINK_POLICY, 2, &policy); - return 0; -} - /* Get HCI device by index. * Device is held on return. */ struct hci_dev *hci_dev_get(int index) @@ -735,6 +691,7 @@ int hci_dev_cmd(unsigned int cmd, void __user *arg) { struct hci_dev *hdev; struct hci_dev_req dr; + __le16 policy; int err = 0; if (copy_from_user(&dr, arg, sizeof(dr))) @@ -761,8 +718,8 @@ int hci_dev_cmd(unsigned int cmd, void __user *arg) switch (cmd) { case HCISETAUTH: - err = hci_req_sync(hdev, hci_auth_req, dr.dev_opt, - HCI_INIT_TIMEOUT, NULL); + err = __hci_cmd_sync_status(hdev, HCI_OP_WRITE_AUTH_ENABLE, + 1, &dr.dev_opt, HCI_CMD_TIMEOUT); break; case HCISETENCRYPT: @@ -773,19 +730,23 @@ int hci_dev_cmd(unsigned int cmd, void __user *arg) if (!test_bit(HCI_AUTH, &hdev->flags)) { /* Auth must be enabled first */ - err = hci_req_sync(hdev, hci_auth_req, dr.dev_opt, - HCI_INIT_TIMEOUT, NULL); + err = __hci_cmd_sync_status(hdev, + HCI_OP_WRITE_AUTH_ENABLE, + 1, &dr.dev_opt, + HCI_CMD_TIMEOUT); if (err) break; } - err = hci_req_sync(hdev, hci_encrypt_req, dr.dev_opt, - HCI_INIT_TIMEOUT, NULL); + err = __hci_cmd_sync_status(hdev, HCI_OP_WRITE_ENCRYPT_MODE, + 1, &dr.dev_opt, + HCI_CMD_TIMEOUT); break; case HCISETSCAN: - err = hci_req_sync(hdev, hci_scan_req, dr.dev_opt, - HCI_INIT_TIMEOUT, NULL); + err = __hci_cmd_sync_status(hdev, HCI_OP_WRITE_SCAN_ENABLE, + 1, &dr.dev_opt, + HCI_CMD_TIMEOUT); /* Ensure that the connectable and discoverable states * get correctly modified as this was a non-mgmt change. @@ -795,8 +756,11 @@ int hci_dev_cmd(unsigned int cmd, void __user *arg) break; case HCISETLINKPOL: - err = hci_req_sync(hdev, hci_linkpol_req, dr.dev_opt, - HCI_INIT_TIMEOUT, NULL); + policy = cpu_to_le16(dr.dev_opt); + + err = __hci_cmd_sync_status(hdev, HCI_OP_WRITE_DEF_LINK_POLICY, + 2, &policy, + HCI_CMD_TIMEOUT); break; case HCISETLINKMODE: diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c index eff648853ae1..ccad43f10415 100644 --- a/net/bluetooth/hci_sync.c +++ b/net/bluetooth/hci_sync.c @@ -280,6 +280,19 @@ int __hci_cmd_sync_status(struct hci_dev *hdev, u16 opcode, u32 plen, } EXPORT_SYMBOL(__hci_cmd_sync_status); +int hci_cmd_sync_status(struct hci_dev *hdev, u16 opcode, u32 plen, + const void *param, u32 timeout) +{ + int err; + + hci_req_sync_lock(hdev); + err = __hci_cmd_sync_status(hdev, opcode, plen, param, timeout); + hci_req_sync_unlock(hdev); + + return err; +} +EXPORT_SYMBOL(hci_cmd_sync_status); + static void hci_cmd_sync_work(struct work_struct *work) { struct hci_dev *hdev = container_of(work, struct hci_dev, cmd_sync_work);