From: Luiz Augusto von Dentz <[email protected]>
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 <[email protected]>
Tested-by: syzbot <[email protected]>
Fixes: e305509e678b3a4a ("Bluetooth: use correct lock to prevent UAF of hdev object")
Signed-off-by: Luiz Augusto von Dentz <[email protected]>
---
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
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=516913
---Test result---
Test Summary:
CheckPatch FAIL 1.11 seconds
GitLint FAIL 0.12 seconds
BuildKernel PASS 680.73 seconds
TestRunner: Setup PASS 445.11 seconds
TestRunner: l2cap-tester PASS 3.15 seconds
TestRunner: bnep-tester PASS 2.21 seconds
TestRunner: mgmt-tester PASS 35.57 seconds
TestRunner: rfcomm-tester PASS 2.48 seconds
TestRunner: sco-tester PASS 2.44 seconds
TestRunner: smp-tester FAIL 2.50 seconds
TestRunner: userchan-tester PASS 2.32 seconds
Details
##############################
Test: CheckPatch - FAIL - 1.11 seconds
Run checkpatch.pl script with rule in .checkpatch.conf
Bluetooth: hci_sock: Fix calling lock_sock when handling HCI_DEV_UNREG
WARNING: Unknown commit id 'e305509e678b3a4a', maybe rebased or not pulled?
#18:
Fixes: e305509e678b3a4a ("Bluetooth: use correct lock to prevent UAF of hdev object")
total: 0 errors, 1 warnings, 0 checks, 389 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
"[PATCH] Bluetooth: hci_sock: Fix calling lock_sock when handling" has style problems, please review.
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
##############################
Test: GitLint - FAIL - 0.12 seconds
Run gitlint with rule in .gitlint
Bluetooth: hci_sock: Fix calling lock_sock when handling HCI_DEV_UNREG
14: B1 Line exceeds max length (85>80): "Fixes: e305509e678b3a4a ("Bluetooth: use correct lock to prevent UAF of hdev object")"
##############################
Test: BuildKernel - PASS - 680.73 seconds
Build Kernel with minimal configuration supports Bluetooth
##############################
Test: TestRunner: Setup - PASS - 445.11 seconds
Setup environment for running Test Runner
##############################
Test: TestRunner: l2cap-tester - PASS - 3.15 seconds
Run test-runner with l2cap-tester
Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0
##############################
Test: TestRunner: bnep-tester - PASS - 2.21 seconds
Run test-runner with bnep-tester
Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0
##############################
Test: TestRunner: mgmt-tester - PASS - 35.57 seconds
Run test-runner with mgmt-tester
Total: 446, Passed: 443 (99.3%), Failed: 0, Not Run: 3
##############################
Test: TestRunner: rfcomm-tester - PASS - 2.48 seconds
Run test-runner with rfcomm-tester
Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0
##############################
Test: TestRunner: sco-tester - PASS - 2.44 seconds
Run test-runner with sco-tester
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0
##############################
Test: TestRunner: smp-tester - FAIL - 2.50 seconds
Run test-runner with smp-tester
Total: 8, Passed: 7 (87.5%), Failed: 1, Not Run: 0
Failed Test Cases
SMP Client - SC Request 2 Failed 0.033 seconds
##############################
Test: TestRunner: userchan-tester - PASS - 2.32 seconds
Run test-runner with userchan-tester
Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0
---
Regards,
Linux Bluetooth
Luiz Augusto von Dentz wrote:
> 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.
I'm not convinced that this change is safe.
vhci_release() (which will call hci_unregister_dev(hdev)) is called when
refcount to /dev/vchi dropped to 0. That is, vhci_release() might be called
while e.g. hci_sock_bound_ioctl() is in progress.
Since hci_unregister_dev(hdev) calls list_del(&hdev->list) with hci_dev_list_lock
held for write, hci_dev_get(hci_pi(sk)->dev) which scans hci_dev_list with
hci_dev_list_lock held for read will start returning NULL. But I think that
this change introduces two race windows.
Since hci_unregister_dev(hdev) then calls hci_sock_dev_event(hdev, HCI_DEV_UNREG)
and finally calls ida_simple_remove(&hci_index_ida, id), subsequent
hci_register_dev(struct hci_dev *hdev) call can re-assign the id which
hci_pi(sk)->dev is tracking, by calling ida_simple_get() and finally calling
list_add(&hdev->list, &hci_dev_list) with hci_dev_list_lock held for write.
Therefore, I think that first race window is that
+ /* 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;
causes hci_sock_bound_ioctl() to check flags on an intended hdev and
e.g. hci_sock_reject_list_add() to operate on an unintended hdev.
Also, even if hci_sock_bound_ioctl() and hci_sock_reject_list_add() reached
the same hdev, I think that there still is second race window that
hci_unregister_dev() { hci_sock_reject_list_add() {
hdev = hci_dev_get(hci_pi(sk)->dev);
write_lock(&hci_dev_list_lock);
list_del(&hdev->list);
write_unlock(&hci_dev_list_lock);
hci_sock_dev_event(hdev, HCI_DEV_UNREG);
hci_dev_lock(hdev);
hci_bdaddr_list_clear(&hdev->reject_list);
hci_dev_unlock(hdev);
hci_dev_lock(hdev);
err = hci_bdaddr_list_add(&hdev->reject_list, &bdaddr, BDADDR_BREDR); // <= Adding after clear all; at least memory leak.
hci_dev_unlock(hdev);
hci_dev_put(hdev);
}
. That is, an attempt to replace pointer reference with index number is racy.
After all, hci_sock_dev_event(hdev, HCI_DEV_UNREG) is responsible for
waiting for already started e.g. hci_sock_reject_list_add().
Hi Tetsuo,
On Sat, Jul 17, 2021 at 7:22 PM Tetsuo Handa
<[email protected]> wrote:
>
> Luiz Augusto von Dentz wrote:
> > 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.
>
> I'm not convinced that this change is safe.
>
> vhci_release() (which will call hci_unregister_dev(hdev)) is called when
> refcount to /dev/vchi dropped to 0. That is, vhci_release() might be called
> while e.g. hci_sock_bound_ioctl() is in progress.
>
> Since hci_unregister_dev(hdev) calls list_del(&hdev->list) with hci_dev_list_lock
> held for write, hci_dev_get(hci_pi(sk)->dev) which scans hci_dev_list with
> hci_dev_list_lock held for read will start returning NULL. But I think that
> this change introduces two race windows.
>
> Since hci_unregister_dev(hdev) then calls hci_sock_dev_event(hdev, HCI_DEV_UNREG)
> and finally calls ida_simple_remove(&hci_index_ida, id), subsequent
> hci_register_dev(struct hci_dev *hdev) call can re-assign the id which
> hci_pi(sk)->dev is tracking, by calling ida_simple_get() and finally calling
> list_add(&hdev->list, &hci_dev_list) with hci_dev_list_lock held for write.
We can perform a pointer comparison if that makes you happy. Anyway I
don't know if you guys have realized already but this is probably
impossible to reproduce with real hardware since the
registration/unregistration comes for other buses no memfault would
hold the thread that long for unplugging and replugging a device on
the bus, vhci is usually only used for emulation/testing/CI, not sure
who got the idea that finding vulnerabilities on our emulator would be
a great feat.
> Therefore, I think that first race window is that
>
> + /* 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;
>
> causes hci_sock_bound_ioctl() to check flags on an intended hdev and
> e.g. hci_sock_reject_list_add() to operate on an unintended hdev.
>
> Also, even if hci_sock_bound_ioctl() and hci_sock_reject_list_add() reached
> the same hdev, I think that there still is second race window that
>
> hci_unregister_dev() { hci_sock_reject_list_add() {
> hdev = hci_dev_get(hci_pi(sk)->dev);
> write_lock(&hci_dev_list_lock);
> list_del(&hdev->list);
> write_unlock(&hci_dev_list_lock);
>
> hci_sock_dev_event(hdev, HCI_DEV_UNREG);
>
> hci_dev_lock(hdev);
> hci_bdaddr_list_clear(&hdev->reject_list);
> hci_dev_unlock(hdev);
> hci_dev_lock(hdev);
> err = hci_bdaddr_list_add(&hdev->reject_list, &bdaddr, BDADDR_BREDR); // <= Adding after clear all; at least memory leak.
> hci_dev_unlock(hdev);
> hci_dev_put(hdev);
> }
>
> . That is, an attempt to replace pointer reference with index number is racy.
>
> After all, hci_sock_dev_event(hdev, HCI_DEV_UNREG) is responsible for
> waiting for already started e.g. hci_sock_reject_list_add().
Both blocks do require hci_dev_lock, so if the second block had
acquired the lock isn't it obvious that the first won't execute until
hci_dev_unlock is complete? Anyway after all these discussion Im even
more convinced that the real problem lies in hci_dev_get/hold, after
all references are usually used to prevent the objects to be freed but
in this case it doesn't and no locking will gonna fix that.
--
Luiz Augusto von Dentz
On 2021/07/18 14:16, Luiz Augusto von Dentz wrote:
> Anyway after all these discussion Im even
> more convinced that the real problem lies in hci_dev_get/hold, after
> all references are usually used to prevent the objects to be freed but
> in this case it doesn't and no locking will gonna fix that.
If hci_dev_hold() calls atomic_long_add_unless(&file->f_count, 1, 0) under RCU,
vhci_release(file) would not be called until all sockets using that hdev drops
the reference, and hci_sock_dev_event(hdev, HCI_DEV_UNREG) no longer needs to
traverse sockets on hci_sk_list.head list. This requires adding "struct file *" to
"struct hci_dev". My patch keeps changes be confined to only hci_sock_dev_event().
Hi Tetsuo,
On Sat, Jul 17, 2021 at 11:22 PM Tetsuo Handa
<[email protected]> wrote:
>
> On 2021/07/18 14:16, Luiz Augusto von Dentz wrote:
> > Anyway after all these discussion Im even
> > more convinced that the real problem lies in hci_dev_get/hold, after
> > all references are usually used to prevent the objects to be freed but
> > in this case it doesn't and no locking will gonna fix that.
>
> If hci_dev_hold() calls atomic_long_add_unless(&file->f_count, 1, 0) under RCU,
> vhci_release(file) would not be called until all sockets using that hdev drops
> the reference, and hci_sock_dev_event(hdev, HCI_DEV_UNREG) no longer needs to
> traverse sockets on hci_sk_list.head list. This requires adding "struct file *" to
> "struct hci_dev". My patch keeps changes be confined to only hci_sock_dev_event().
Being confined doesn't mean that it simple, your changes are doing a
loop locking, and I also didn't touch hci_dev_hold because it would
affect all drivers but if there is a way to do it by all means we
should do it, but notice that we do need a way to cleanup if the
device is unregistered so I don't think holding the file directly
would be a good idea since it prevents release but it would also
prevent cleanup, in other words if the process which open the vhci
terminates or close, all bluetooth sockets should receive a proper
error so we cannot really change this behavior. From the brief look at
it I think we should remove the function hci_dev_free and leave
hci_dev_unregister to cleanup everything, but I'm afraid there could
be extra references that are not being cleanup properly and finding
out where could take a lot more time, well even with your suggestion
that could be a problem since we also would need to inspect every time
we hold a reference in the same manner.
--
Luiz Augusto von Dentz
Apart from my "[PATCH v3] Bluetooth: call lock_sock() outside of spinlock section",
I propose below change in order to make sure that hci_sock_bound_ioctl() will not
be blocked with sock lock held due to userfaultfd mechanism. This is a portion of
improvement I commented
After lock_sock() became free from delay caused by pagefault handling
at https://lore.kernel.org/linux-bluetooth/[email protected]/ .
include/net/bluetooth/hci_core.h | 3
net/bluetooth/hci_conn.c | 50 -----------
net/bluetooth/hci_sock.c | 163 ++++++++++++++++++++++++---------------
3 files changed, 106 insertions(+), 110 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index a53e94459ecd..d9e55682b908 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1261,8 +1261,7 @@ 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);
+u32 get_link_mode(struct hci_conn *conn);
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 2b5059a56cda..41af11fadb74 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -1626,7 +1626,7 @@ void hci_conn_check_pending(struct hci_dev *hdev)
hci_dev_unlock(hdev);
}
-static u32 get_link_mode(struct hci_conn *conn)
+u32 get_link_mode(struct hci_conn *conn)
{
u32 link_mode = 0;
@@ -1701,54 +1701,6 @@ int hci_get_conn_list(void __user *arg)
return err ? -EFAULT : 0;
}
-int hci_get_conn_info(struct hci_dev *hdev, void __user *arg)
-{
- 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;
-
- hci_dev_lock(hdev);
- conn = hci_conn_hash_lookup_ba(hdev, req.type, &req.bdaddr);
- if (conn) {
- bacpy(&ci.bdaddr, &conn->dst);
- ci.handle = conn->handle;
- ci.type = conn->type;
- ci.out = conn->out;
- ci.state = conn->state;
- ci.link_mode = get_link_mode(conn);
- }
- hci_dev_unlock(hdev);
-
- if (!conn)
- return -ENOENT;
-
- return copy_to_user(ptr, &ci, sizeof(ci)) ? -EFAULT : 0;
-}
-
-int hci_get_auth_info(struct hci_dev *hdev, void __user *arg)
-{
- 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);
- if (conn)
- req.type = conn->auth_type;
- hci_dev_unlock(hdev);
-
- if (!conn)
- return -ENOENT;
-
- return copy_to_user(arg, &req, sizeof(req)) ? -EFAULT : 0;
-}
-
struct hci_chan *hci_chan_create(struct hci_conn *conn)
{
struct hci_dev *hdev = conn->hdev;
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index b04a5a02ecf3..2b166a269712 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -892,82 +892,134 @@ 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 hci_dev *hdev, bdaddr_t *bdaddr)
{
- bdaddr_t bdaddr;
- int err;
-
- if (copy_from_user(&bdaddr, arg, sizeof(bdaddr)))
- return -EFAULT;
-
- hci_dev_lock(hdev);
-
- err = hci_bdaddr_list_add(&hdev->reject_list, &bdaddr, BDADDR_BREDR);
-
- hci_dev_unlock(hdev);
-
- return err;
+ return hci_bdaddr_list_add(&hdev->reject_list, bdaddr, BDADDR_BREDR);
}
-static int hci_sock_reject_list_del(struct hci_dev *hdev, void __user *arg)
+static int hci_sock_reject_list_del(struct hci_dev *hdev, bdaddr_t *bdaddr)
{
- bdaddr_t bdaddr;
- int err;
-
- if (copy_from_user(&bdaddr, arg, sizeof(bdaddr)))
- return -EFAULT;
-
- hci_dev_lock(hdev);
+ return hci_bdaddr_list_del(&hdev->reject_list, bdaddr, BDADDR_BREDR);
+}
- err = hci_bdaddr_list_del(&hdev->reject_list, &bdaddr, BDADDR_BREDR);
+static int hci_get_conn_info(struct hci_dev *hdev, struct hci_conn_info_req *req,
+ struct hci_conn_info *ci)
+{
+ struct hci_conn *conn;
+
+ conn = hci_conn_hash_lookup_ba(hdev, req->type, &req->bdaddr);
+ if (!conn)
+ return -ENOENT;
+ bacpy(&ci->bdaddr, &conn->dst);
+ ci->handle = conn->handle;
+ ci->type = conn->type;
+ ci->out = conn->out;
+ ci->state = conn->state;
+ ci->link_mode = get_link_mode(conn);
+ return 0;
+}
- hci_dev_unlock(hdev);
+static int hci_get_auth_info(struct hci_dev *hdev, struct hci_auth_info_req *req)
+{
+ struct hci_conn *conn;
- return err;
+ conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &req->bdaddr);
+ if (!conn)
+ return -ENOENT;
+ req->type = conn->auth_type;
+ return 0;
}
/* Ioctls that require bound socket */
-static int hci_sock_bound_ioctl(struct sock *sk, unsigned int cmd,
- unsigned long arg)
+static int hci_sock_bound_ioctl(struct sock *sk, unsigned int cmd, void __user *arg)
{
- struct hci_dev *hdev = hci_pi(sk)->hdev;
+ struct hci_dev *hdev;
+ union {
+ bdaddr_t bdaddr;
+ struct hci_conn_info_req conn_req;
+ struct hci_auth_info_req auth_req;
+ } u;
+ struct hci_conn_info ci;
+ int err;
- if (!hdev)
- return -EBADFD;
+ if (cmd == HCIBLOCKADDR || cmd == HCIUNBLOCKADDR) {
+ if (copy_from_user(&u.bdaddr, arg, sizeof(u.bdaddr)))
+ return -EFAULT;
+ } else if (cmd == HCIGETCONNINFO) {
+ if (copy_from_user(&u.conn_req, arg, sizeof(u.conn_req)))
+ return -EFAULT;
+ } else if (cmd == HCIGETAUTHINFO) {
+ if (copy_from_user(&u.auth_req, arg, sizeof(u.auth_req)))
+ return -EFAULT;
+ }
- if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL))
- return -EBUSY;
+ lock_sock(sk);
- if (hci_dev_test_flag(hdev, HCI_UNCONFIGURED))
- return -EOPNOTSUPP;
+ hdev = hci_pi(sk)->hdev;
+ if (!hdev) {
+ err = -EBADFD;
+ goto out;
+ }
- if (hdev->dev_type != HCI_PRIMARY)
- return -EOPNOTSUPP;
+ if (hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) {
+ err = -EBUSY;
+ goto out;
+ }
+
+ if (hci_dev_test_flag(hdev, HCI_UNCONFIGURED)) {
+ err = -EOPNOTSUPP;
+ goto out;
+ }
+
+ if (hdev->dev_type != HCI_PRIMARY) {
+ err = -EOPNOTSUPP;
+ goto out;
+ }
+ hci_dev_lock(hdev);
switch (cmd) {
case HCISETRAW:
if (!capable(CAP_NET_ADMIN))
- return -EPERM;
- return -EOPNOTSUPP;
-
+ err = -EPERM;
+ else
+ err = -EOPNOTSUPP;
+ break;
case HCIGETCONNINFO:
- return hci_get_conn_info(hdev, (void __user *)arg);
-
+ err = hci_get_conn_info(hdev, &u.conn_req, &ci);
+ break;
case HCIGETAUTHINFO:
- return hci_get_auth_info(hdev, (void __user *)arg);
-
+ err = hci_get_auth_info(hdev, &u.auth_req);
+ break;
case HCIBLOCKADDR:
if (!capable(CAP_NET_ADMIN))
- return -EPERM;
- return hci_sock_reject_list_add(hdev, (void __user *)arg);
-
+ err = -EPERM;
+ else
+ err = hci_sock_reject_list_add(hdev, &u.bdaddr);
+ break;
case HCIUNBLOCKADDR:
if (!capable(CAP_NET_ADMIN))
- return -EPERM;
- return hci_sock_reject_list_del(hdev, (void __user *)arg);
+ err = -EPERM;
+ else
+ err = hci_sock_reject_list_del(hdev, &u.bdaddr);
+ break;
+ default:
+ err = -ENOIOCTLCMD;
}
+ hci_dev_unlock(hdev);
+
+ out:
+ release_sock(sk);
- return -ENOIOCTLCMD;
+ if (!err) {
+ if (cmd == HCIGETCONNINFO) {
+ if (copy_to_user(arg + sizeof(u.conn_req), &ci, sizeof(ci)))
+ err = -EFAULT;
+ } else if (cmd == HCIGETAUTHINFO) {
+ if (copy_to_user(arg, &u.auth_req, sizeof(u.auth_req)))
+ err = -EFAULT;
+ }
+ }
+ return err;
}
static int hci_sock_ioctl(struct socket *sock, unsigned int cmd,
@@ -975,15 +1027,14 @@ static int hci_sock_ioctl(struct socket *sock, unsigned int cmd,
{
void __user *argp = (void __user *)arg;
struct sock *sk = sock->sk;
- int err;
BT_DBG("cmd %x arg %lx", cmd, arg);
lock_sock(sk);
if (hci_pi(sk)->channel != HCI_CHANNEL_RAW) {
- err = -EBADFD;
- goto done;
+ release_sock(sk);
+ return -EBADFD;
}
/* When calling an ioctl on an unbound raw socket, then ensure
@@ -1055,13 +1106,7 @@ static int hci_sock_ioctl(struct socket *sock, unsigned int cmd,
return hci_inquiry(argp);
}
- lock_sock(sk);
-
- err = hci_sock_bound_ioctl(sk, cmd, arg);
-
-done:
- release_sock(sk);
- return err;
+ return hci_sock_bound_ioctl(sk, cmd, (void __user *)arg);
}
#ifdef CONFIG_COMPAT