From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
To: linux-bluetooth@vger.kernel.org
Subject: [RFC] Bluetooth: hci_sock: Fix calling lock_sock when handling HCI_DEV_UNREG
Date: Fri, 16 Jul 2021 17:07:31 -0700 [thread overview]
Message-ID: <20210717000731.3836303-1-luiz.dentz@gmail.com> (raw)
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
This removes the reference of hci_dev to hci_pinfo since the reference
cannot prevent hdev to be freed hci_pinfo only keeps the index so in
case the device is unregistered and freed hci_dev_get will return NULL
thus prevent UAF.
On top of it commands cases where copy_from_user needs to be used are
now done without helding a reference to the hci_dev.
Link: https://syzkaller.appspot.com/bug?extid=a5df189917e79d5e59c9 [1]
Reported-by: syzbot <syzbot+a5df189917e79d5e59c9@syzkaller.appspotmail.com>
Tested-by: syzbot <syzbot+a5df189917e79d5e59c9@syzkaller.appspotmail.com>
Fixes: e305509e678b3a4a ("Bluetooth: use correct lock to prevent UAF of hdev object")
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
include/net/bluetooth/hci_core.h | 6 +-
net/bluetooth/hci_conn.c | 24 ++---
net/bluetooth/hci_sock.c | 162 +++++++++++++++++++++++--------
3 files changed, 137 insertions(+), 55 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 8b63ef2ec31a..ddac4be91525 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1266,8 +1266,10 @@ int hci_dev_cmd(unsigned int cmd, void __user *arg);
int hci_get_dev_list(void __user *arg);
int hci_get_dev_info(void __user *arg);
int hci_get_conn_list(void __user *arg);
-int hci_get_conn_info(struct hci_dev *hdev, void __user *arg);
-int hci_get_auth_info(struct hci_dev *hdev, void __user *arg);
+int hci_get_conn_info(struct hci_dev *hdev, void __user *arg,
+ struct hci_conn_info_req *req);
+int hci_get_auth_info(struct hci_dev *hdev, void __user *arg,
+ struct hci_auth_info_req *req);
int hci_inquiry(void __user *arg);
struct bdaddr_list *hci_bdaddr_list_lookup(struct list_head *list,
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index b9fbab563362..4345bcc05778 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -1701,18 +1701,15 @@ int hci_get_conn_list(void __user *arg)
return err ? -EFAULT : 0;
}
-int hci_get_conn_info(struct hci_dev *hdev, void __user *arg)
+int hci_get_conn_info(struct hci_dev *hdev, void __user *arg,
+ struct hci_conn_info_req *req)
{
- struct hci_conn_info_req req;
struct hci_conn_info ci;
struct hci_conn *conn;
- char __user *ptr = arg + sizeof(req);
-
- if (copy_from_user(&req, arg, sizeof(req)))
- return -EFAULT;
+ char __user *ptr = (char *)arg + sizeof(*req);
hci_dev_lock(hdev);
- conn = hci_conn_hash_lookup_ba(hdev, req.type, &req.bdaddr);
+ conn = hci_conn_hash_lookup_ba(hdev, req->type, &req->bdaddr);
if (conn) {
bacpy(&ci.bdaddr, &conn->dst);
ci.handle = conn->handle;
@@ -1729,24 +1726,21 @@ int hci_get_conn_info(struct hci_dev *hdev, void __user *arg)
return copy_to_user(ptr, &ci, sizeof(ci)) ? -EFAULT : 0;
}
-int hci_get_auth_info(struct hci_dev *hdev, void __user *arg)
+int hci_get_auth_info(struct hci_dev *hdev, void __user *arg,
+ struct hci_auth_info_req *req)
{
- struct hci_auth_info_req req;
struct hci_conn *conn;
- if (copy_from_user(&req, arg, sizeof(req)))
- return -EFAULT;
-
hci_dev_lock(hdev);
- conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &req.bdaddr);
+ conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &req->bdaddr);
if (conn)
- req.type = conn->auth_type;
+ req->type = conn->auth_type;
hci_dev_unlock(hdev);
if (!conn)
return -ENOENT;
- return copy_to_user(arg, &req, sizeof(req)) ? -EFAULT : 0;
+ return copy_to_user(arg, req, sizeof(*req)) ? -EFAULT : 0;
}
struct hci_chan *hci_chan_create(struct hci_conn *conn)
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index b04a5a02ecf3..3f104a3aca7e 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -50,7 +50,7 @@ static atomic_t monitor_promisc = ATOMIC_INIT(0);
struct hci_pinfo {
struct bt_sock bt;
- struct hci_dev *hdev;
+ int dev;
struct hci_filter filter;
__u8 cmsg_mask;
unsigned short channel;
@@ -200,7 +200,8 @@ void hci_send_to_sock(struct hci_dev *hdev, struct sk_buff *skb)
sk_for_each(sk, &hci_sk_list.head) {
struct sk_buff *nskb;
- if (sk->sk_state != BT_BOUND || hci_pi(sk)->hdev != hdev)
+ if (sk->sk_state != BT_BOUND ||
+ (hdev && hci_pi(sk)->dev != hdev->id))
continue;
/* Don't send frame to the socket it came from */
@@ -536,8 +537,8 @@ static struct sk_buff *create_monitor_ctrl_open(struct sock *sk)
hdr = skb_push(skb, HCI_MON_HDR_SIZE);
hdr->opcode = cpu_to_le16(HCI_MON_CTRL_OPEN);
- if (hci_pi(sk)->hdev)
- hdr->index = cpu_to_le16(hci_pi(sk)->hdev->id);
+ if (hci_pi(sk)->dev >= 0)
+ hdr->index = cpu_to_le16(hci_pi(sk)->dev);
else
hdr->index = cpu_to_le16(HCI_DEV_NONE);
hdr->len = cpu_to_le16(skb->len - HCI_MON_HDR_SIZE);
@@ -574,8 +575,8 @@ static struct sk_buff *create_monitor_ctrl_close(struct sock *sk)
hdr = skb_push(skb, HCI_MON_HDR_SIZE);
hdr->opcode = cpu_to_le16(HCI_MON_CTRL_CLOSE);
- if (hci_pi(sk)->hdev)
- hdr->index = cpu_to_le16(hci_pi(sk)->hdev->id);
+ if (hci_pi(sk)->dev >= 0)
+ hdr->index = cpu_to_le16(hci_pi(sk)->dev);
else
hdr->index = cpu_to_le16(HCI_DEV_NONE);
hdr->len = cpu_to_le16(skb->len - HCI_MON_HDR_SIZE);
@@ -762,16 +763,13 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event)
/* Detach sockets from device */
read_lock(&hci_sk_list.lock);
sk_for_each(sk, &hci_sk_list.head) {
- lock_sock(sk);
- if (hci_pi(sk)->hdev == hdev) {
- hci_pi(sk)->hdev = NULL;
+ bh_lock_sock_nested(sk);
+ if (hci_pi(sk)->dev == hdev->id) {
sk->sk_err = EPIPE;
sk->sk_state = BT_OPEN;
sk->sk_state_change(sk);
-
- hci_dev_put(hdev);
}
- release_sock(sk);
+ bh_unlock_sock(sk);
}
read_unlock(&hci_sk_list.lock);
}
@@ -861,7 +859,7 @@ static int hci_sock_release(struct socket *sock)
bt_sock_unlink(&hci_sk_list, sk);
- hdev = hci_pi(sk)->hdev;
+ hdev = hci_dev_get(hci_pi(sk)->dev);
if (hdev) {
if (hci_pi(sk)->channel == HCI_CHANNEL_USER) {
/* When releasing a user channel exclusive access,
@@ -892,82 +890,161 @@ static int hci_sock_release(struct socket *sock)
return 0;
}
-static int hci_sock_reject_list_add(struct hci_dev *hdev, void __user *arg)
+static int hci_sock_reject_list_add(struct sock *sk, void __user *arg)
{
+ struct hci_dev *hdev;
bdaddr_t bdaddr;
int err;
if (copy_from_user(&bdaddr, arg, sizeof(bdaddr)))
return -EFAULT;
+ hdev = hci_dev_get(hci_pi(sk)->dev);
+ if (!hdev)
+ return -EBADF;
+
hci_dev_lock(hdev);
err = hci_bdaddr_list_add(&hdev->reject_list, &bdaddr, BDADDR_BREDR);
hci_dev_unlock(hdev);
+ hci_dev_put(hdev);
return err;
}
-static int hci_sock_reject_list_del(struct hci_dev *hdev, void __user *arg)
+static int hci_sock_reject_list_del(struct sock *sk, void __user *arg)
{
+ struct hci_dev *hdev;
bdaddr_t bdaddr;
int err;
if (copy_from_user(&bdaddr, arg, sizeof(bdaddr)))
return -EFAULT;
+ hdev = hci_dev_get(hci_pi(sk)->dev);
+ if (!hdev)
+ return -EBADF;
+
hci_dev_lock(hdev);
err = hci_bdaddr_list_del(&hdev->reject_list, &bdaddr, BDADDR_BREDR);
hci_dev_unlock(hdev);
+ hci_dev_put(hdev);
return err;
}
+static int hci_sock_get_conn_info(struct sock *sk, void __user *arg)
+{
+ struct hci_dev *hdev;
+ struct hci_conn_info_req req;
+ int ret;
+
+ if (copy_from_user(&req, arg, sizeof(req)))
+ return -EFAULT;
+
+ hdev = hci_dev_get(hci_pi(sk)->dev);
+ if (!hdev)
+ return -EBADF;
+
+ ret = hci_get_conn_info(hdev, arg, &req);
+
+ hci_dev_put(hdev);
+
+ return ret;
+}
+
+static int hci_sock_get_auth_info(struct sock *sk, void __user *arg)
+{
+ struct hci_dev *hdev;
+ struct hci_auth_info_req req;
+ int ret;
+
+ if (copy_from_user(&req, arg, sizeof(req)))
+ return -EFAULT;
+
+ hdev = hci_dev_get(hci_pi(sk)->dev);
+ if (!hdev)
+ return -EBADF;
+
+ ret = hci_get_auth_info(hdev, arg, &req);
+
+ hci_dev_put(hdev);
+
+ return ret;
+}
+
/* Ioctls that require bound socket */
static int hci_sock_bound_ioctl(struct sock *sk, unsigned int cmd,
unsigned long arg)
{
- struct hci_dev *hdev = hci_pi(sk)->hdev;
+ struct hci_dev *hdev = hci_dev_get(hci_pi(sk)->dev);
+ int ret;
if (!hdev)
return -EBADFD;
- if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL))
- return -EBUSY;
+ if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) {
+ ret = -EBUSY;
+ goto done;
+ }
- if (hci_dev_test_flag(hdev, HCI_UNCONFIGURED))
- return -EOPNOTSUPP;
+ if (hci_dev_test_flag(hdev, HCI_UNCONFIGURED)) {
+ ret = -EOPNOTSUPP;
+ goto done;
+ }
- if (hdev->dev_type != HCI_PRIMARY)
- return -EOPNOTSUPP;
+ if (hdev->dev_type != HCI_PRIMARY) {
+ ret = -EOPNOTSUPP;
+ goto done;
+ }
+
+ /* Commands may use copy_from_user which is unsafe while holding hdev as
+ * it could be unregistered in the meantime.
+ */
+ hci_dev_put(hdev);
+ hdev = NULL;
switch (cmd) {
case HCISETRAW:
if (!capable(CAP_NET_ADMIN))
- return -EPERM;
- return -EOPNOTSUPP;
+ ret = -EPERM;
+ else
+ ret = -EOPNOTSUPP;
+ break;
case HCIGETCONNINFO:
- return hci_get_conn_info(hdev, (void __user *)arg);
+ ret = hci_sock_get_conn_info(sk, (void __user *)arg);
+ break;
case HCIGETAUTHINFO:
- return hci_get_auth_info(hdev, (void __user *)arg);
+ ret = hci_sock_get_auth_info(sk, (void __user *)arg);
+ break;
case HCIBLOCKADDR:
if (!capable(CAP_NET_ADMIN))
- return -EPERM;
- return hci_sock_reject_list_add(hdev, (void __user *)arg);
+ ret = -EPERM;
+ else
+ ret = hci_sock_reject_list_add(sk, (void __user *)arg);
+ break;
case HCIUNBLOCKADDR:
if (!capable(CAP_NET_ADMIN))
- return -EPERM;
- return hci_sock_reject_list_del(hdev, (void __user *)arg);
+ ret = -EPERM;
+ else
+ ret = hci_sock_reject_list_del(sk, (void __user *)arg);
+ break;
+ default:
+ ret = -ENOIOCTLCMD;
}
- return -ENOIOCTLCMD;
+done:
+ if (hdev)
+ hci_dev_put(hdev);
+
+ return ret;
}
static int hci_sock_ioctl(struct socket *sock, unsigned int cmd,
@@ -1110,7 +1187,7 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr,
switch (haddr.hci_channel) {
case HCI_CHANNEL_RAW:
- if (hci_pi(sk)->hdev) {
+ if (hci_pi(sk)->dev >= 0) {
err = -EALREADY;
goto done;
}
@@ -1145,7 +1222,10 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr,
if (capable(CAP_NET_ADMIN))
hci_sock_set_flag(sk, HCI_SOCK_TRUSTED);
- hci_pi(sk)->hdev = hdev;
+ if (hdev) {
+ hci_pi(sk)->dev = hdev->id;
+ hci_dev_put(hdev);
+ }
/* Send event to monitor */
skb = create_monitor_ctrl_open(sk);
@@ -1157,7 +1237,7 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr,
break;
case HCI_CHANNEL_USER:
- if (hci_pi(sk)->hdev) {
+ if (hci_pi(sk)->dev >= 0) {
err = -EALREADY;
goto done;
}
@@ -1236,7 +1316,8 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr,
*/
hci_sock_set_flag(sk, HCI_SOCK_TRUSTED);
- hci_pi(sk)->hdev = hdev;
+ hci_pi(sk)->dev = hdev->id;
+ hci_dev_put(hdev);
/* Send event to monitor */
skb = create_monitor_ctrl_open(sk);
@@ -1379,7 +1460,7 @@ static int hci_sock_getname(struct socket *sock, struct sockaddr *addr,
lock_sock(sk);
- hdev = hci_pi(sk)->hdev;
+ hdev = hci_dev_get(hci_pi(sk)->dev);
if (!hdev) {
err = -EBADFD;
goto done;
@@ -1389,6 +1470,7 @@ static int hci_sock_getname(struct socket *sock, struct sockaddr *addr,
haddr->hci_dev = hdev->id;
haddr->hci_channel= hci_pi(sk)->channel;
err = sizeof(*haddr);
+ hci_dev_put(hdev);
done:
release_sock(sk);
@@ -1703,7 +1785,7 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,
{
struct sock *sk = sock->sk;
struct hci_mgmt_chan *chan;
- struct hci_dev *hdev;
+ struct hci_dev *hdev = NULL;
struct sk_buff *skb;
int err;
@@ -1743,7 +1825,7 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,
goto done;
}
- hdev = hci_pi(sk)->hdev;
+ hdev = hci_dev_get(hci_pi(sk)->dev);
if (!hdev) {
err = -EBADFD;
goto done;
@@ -1832,6 +1914,9 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,
err = len;
done:
+ if (hdev)
+ hci_dev_put(hdev);
+
release_sock(sk);
return err;
@@ -2049,6 +2134,7 @@ static int hci_sock_create(struct net *net, struct socket *sock, int protocol,
sock->state = SS_UNCONNECTED;
sk->sk_state = BT_OPEN;
+ hci_pi(sk)->dev = -1;
bt_sock_link(&hci_sk_list, sk);
return 0;
}
--
2.31.1
next reply other threads:[~2021-07-17 0:07 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-17 0:07 Luiz Augusto von Dentz [this message]
2021-07-17 2:04 ` [RFC] Bluetooth: hci_sock: Fix calling lock_sock when handling HCI_DEV_UNREG bluez.test.bot
2021-07-18 2:22 ` Tetsuo Handa
2021-07-18 5:16 ` Luiz Augusto von Dentz
2021-07-18 6:22 ` Tetsuo Handa
2021-07-18 6:51 ` Luiz Augusto von Dentz
2021-07-18 14:56 ` Tetsuo Handa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210717000731.3836303-1-luiz.dentz@gmail.com \
--to=luiz.dentz@gmail.com \
--cc=linux-bluetooth@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).