Re: [PATCH v1] Bluetooth: Fix double free in hci_req_sync_complete

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Pauli,

On Sat, Jun 29, 2024 at 6:07 AM Pauli Virtanen <pav@xxxxxx> wrote:
>
> Hi Luiz,
>
> ma, 2024-06-24 kello 10:49 -0400, Luiz Augusto von Dentz kirjoitti:
> > From: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Closes: https://syzkaller.appspot.com/bug?extid=35ebc808442df6420eae
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx>
> > ---
> >  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



-- 
Luiz Augusto von Dentz





[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux