| Message ID | 20230502145737.140856-1-lrh2000@pku.edu.cn |
|---|---|
| Headers | show |
| Series | Bluetooth: Fix potential double free caused by hci_conn_unlink | expand |
Hi Ruihan, On Tue, May 2, 2023 at 7:57 AM Ruihan Li <lrh2000@pku.edu.cn> wrote: > > Previously, hci_conn_unlink was implemented as a recursion function. To > unlink physical connections (e.g. ACL/LE), it calls itself to unlink all > its logical channels (e.g. SCO/eSCO/ISO). > > Recursion is not required. This patch refactors hci_conn_unlink into two > functions, where hci_conn_unlink_parent takes a physical connection, > checks out all its logical channels, and calls hci_conn_unlink_child for > each logical channel to unlink it. > > Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn> > --- > net/bluetooth/hci_conn.c | 55 +++++++++++++++++++++++----------------- > 1 file changed, 32 insertions(+), 23 deletions(-) > > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c > index de553e062..243d68a64 100644 > --- a/net/bluetooth/hci_conn.c > +++ b/net/bluetooth/hci_conn.c > @@ -1074,34 +1074,13 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst, > return conn; > } > > -static void hci_conn_unlink(struct hci_conn *conn) > +static void hci_conn_unlink_parent(struct hci_conn *conn) > { > struct hci_dev *hdev = conn->hdev; > > bt_dev_dbg(hdev, "hcon %p", conn); > > - if (!conn->parent) { > - struct hci_link *link, *t; > - > - list_for_each_entry_safe(link, t, &conn->link_list, list) { > - struct hci_conn *child = link->conn; > - > - hci_conn_unlink(child); > - > - /* Due to race, SCO connection might be not established > - * yet at this point. Delete it now, otherwise it is > - * possible for it to be stuck and can't be deleted. > - */ > - if ((child->type == SCO_LINK || > - child->type == ESCO_LINK) && > - child->handle == HCI_CONN_HANDLE_UNSET) > - hci_conn_del(child); > - } > - > - return; > - } > - > - if (!conn->link) > + if (WARN_ON(!conn->link)) > return; > > list_del_rcu(&conn->link->list); > @@ -1115,6 +1094,36 @@ static void hci_conn_unlink(struct hci_conn *conn) > conn->link = NULL; > } > > +static void hci_conn_unlink_children(struct hci_conn *conn) > +{ > + struct hci_dev *hdev = conn->hdev; > + struct hci_link *link, *t; > + > + bt_dev_dbg(hdev, "hcon %p", conn); > + > + list_for_each_entry_safe(link, t, &conn->link_list, list) { > + struct hci_conn *child = link->conn; > + > + hci_conn_unlink_parent(child); > + > + /* Due to race, SCO connection might be not established > + * yet at this point. Delete it now, otherwise it is > + * possible for it to be stuck and can't be deleted. > + */ > + if (child->type == SCO_LINK || child->type == ESCO_LINK) > + if (child->handle == HCI_CONN_HANDLE_UNSET) > + hci_conn_del(child); > + } This is not quite right, when we are unlinking the children's hci_conn it shall only unlink itself from the parent not everything. > +} > + > +static void hci_conn_unlink(struct hci_conn *conn) > +{ > + if (conn->parent) > + hci_conn_unlink_parent(conn); > + else > + hci_conn_unlink_children(conn); > +} > + > void hci_conn_del(struct hci_conn *conn) > { > struct hci_dev *hdev = conn->hdev; > -- > 2.40.0 >
Hi Luiz, On Tue, May 02, 2023 at 09:18:02AM -0700, Luiz Augusto von Dentz wrote: > Hi Ruihan, > > On Tue, May 2, 2023 at 7:57 AM Ruihan Li <lrh2000@pku.edu.cn> wrote: > > > > Previously, hci_conn_unlink was implemented as a recursion function. To > > unlink physical connections (e.g. ACL/LE), it calls itself to unlink all > > its logical channels (e.g. SCO/eSCO/ISO). > > > > Recursion is not required. This patch refactors hci_conn_unlink into two > > functions, where hci_conn_unlink_parent takes a physical connection, > > checks out all its logical channels, and calls hci_conn_unlink_child for > > each logical channel to unlink it. > > > > Signed-off-by: Ruihan Li <lrh2000@pku.edu.cn> > > --- > > net/bluetooth/hci_conn.c | 55 +++++++++++++++++++++++----------------- > > 1 file changed, 32 insertions(+), 23 deletions(-) > > > > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c > > index de553e062..243d68a64 100644 > > --- a/net/bluetooth/hci_conn.c > > +++ b/net/bluetooth/hci_conn.c > > @@ -1074,34 +1074,13 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst, > > return conn; > > } > > > > -static void hci_conn_unlink(struct hci_conn *conn) > > +static void hci_conn_unlink_parent(struct hci_conn *conn) > > { > > struct hci_dev *hdev = conn->hdev; > > > > bt_dev_dbg(hdev, "hcon %p", conn); > > > > - if (!conn->parent) { > > - struct hci_link *link, *t; > > - > > - list_for_each_entry_safe(link, t, &conn->link_list, list) { > > - struct hci_conn *child = link->conn; > > - > > - hci_conn_unlink(child); > > - > > - /* Due to race, SCO connection might be not established > > - * yet at this point. Delete it now, otherwise it is > > - * possible for it to be stuck and can't be deleted. > > - */ > > - if ((child->type == SCO_LINK || > > - child->type == ESCO_LINK) && > > - child->handle == HCI_CONN_HANDLE_UNSET) > > - hci_conn_del(child); > > - } > > - > > - return; > > - } > > - > > - if (!conn->link) > > + if (WARN_ON(!conn->link)) > > return; > > > > list_del_rcu(&conn->link->list); > > @@ -1115,6 +1094,36 @@ static void hci_conn_unlink(struct hci_conn *conn) > > conn->link = NULL; > > } > > > > +static void hci_conn_unlink_children(struct hci_conn *conn) > > +{ > > + struct hci_dev *hdev = conn->hdev; > > + struct hci_link *link, *t; > > + > > + bt_dev_dbg(hdev, "hcon %p", conn); > > + > > + list_for_each_entry_safe(link, t, &conn->link_list, list) { > > + struct hci_conn *child = link->conn; > > + > > + hci_conn_unlink_parent(child); > > + > > + /* Due to race, SCO connection might be not established > > + * yet at this point. Delete it now, otherwise it is > > + * possible for it to be stuck and can't be deleted. > > + */ > > + if (child->type == SCO_LINK || child->type == ESCO_LINK) > > + if (child->handle == HCI_CONN_HANDLE_UNSET) > > + hci_conn_del(child); > > + } > > This is not quite right, when we are unlinking the children's hci_conn > it shall only unlink itself from the parent not everything. My assumption is that each hci_conn is either * a logical channel, which implies that it has a parent and no children, or * a physical link, which means that it has no parent and possibly some children. So here, as children of physical links, they must be logical channels and thus cannot have more children, so just unlinking them from the parent should be ok. We can add some assertions like WARN_ON(!conn->parent || !conn->link) // conn has a parent WARN_ON(!list_empty(&conn->link_list)) // conn has no children in hci_conn_unlink_parent. Do you think this assumption is wrong, or could become wrong in the future? Otherwise, I don't think there are correctness problems. In my opinion, separating the functions for logical channels and physical links makes the code a bit cleaner. But it's my opinion, if you think it actually makes the code harder to understand, I won't insist. > > > +} > > + > > +static void hci_conn_unlink(struct hci_conn *conn) > > +{ > > + if (conn->parent) > > + hci_conn_unlink_parent(conn); > > + else > > + hci_conn_unlink_children(conn); > > +} > > + > > void hci_conn_del(struct hci_conn *conn) > > { > > struct hci_dev *hdev = conn->hdev; > > -- > > 2.40.0 > > > > > -- > Luiz Augusto von Dentz Thanks, Ruihan Li
This patch series contains six fixes related to hci_conn_unlink. The purpose is to prevent merge conflicts between each other. I'm not intentially linking them together. So if any patch is not suitable, please just let me know (I'll be grateful if you can explain the reason). The first three patches are the most important, each fixing a triggerable use-after-free bug (see the report URL for details). And the fourth through sixth patches are a bit more minor, containing mostly tweaks and refactorings. Changes since v2: * Put all fixes, adjustments, and refactorings about hci_conn_unlink in one patch series. Link to v2: * https://lore.kernel.org/linux-bluetooth/20230430180535.168270-1-lrh2000@pku.edu.cn/ See also: * https://lore.kernel.org/linux-bluetooth/20230430171847.156825-1-lrh2000@pku.edu.cn/ Changes since v1: * Resolve merge conflicts. Link to v1: * https://lore.kernel.org/linux-bluetooth/20230430172937.157999-1-lrh2000@pku.edu.cn/ Ruihan Li (6): Bluetooth: Fix potential double free caused by hci_conn_unlink Bluetooth: Refcnt drop must be placed last in hci_conn_unlink Bluetooth: Fix UAF in hci_conn_hash_flush again Bluetooth: Perform hci_conn_drop in hci_conn_unlink Bluetooth: Unlink CISes when LE disconnects in hci_conn_del Bluetooth: Avoid recursion in hci_conn_unlink include/net/bluetooth/hci_core.h | 2 +- net/bluetooth/hci_conn.c | 96 ++++++++++++++++++-------------- 2 files changed, 54 insertions(+), 44 deletions(-)