Skip to contentSkip to conversation

a bunch of old bluetooth fixes

PATCH In review Al Viro 5 participants 12 messages Dec 19, 2014 2 messages not in the archive

Subsystems (4)
Networking Network drivers Bluetooth drivers Bluetooth
lore
Share & tools
lore

Al Viro

author
#0 lore raw

PATCH 1/1 a bunch of old bluetooth fixes

This stuff has been sitting in my queue since March; basically,
several places in net/bluetooth assume that they are dealing with l2cap sockets, while it is possible to get an arbitrary socket to those. Results are not pretty.
* HIDPCONNADD gets an arbitrary user-supplied socket; the code
it calls (hidp_connection_add()) verifies that the socket is l2cap one, but before doing so it finds l2cap_pi(ctrl_sock->sk)->chan. It's not that big a deal (it's only 5 words past the end of struct sock), but it's trivial to avoid and, in theory, we might end up oopsing here if we are very unlucky and it happens to hit an unmapped page just past the actual object ctrl_sock->sk sits in.
* CMTP counterpart of that doesn't validate the socket at all.
It proceeds to

        s = __cmtp_get_session(&l2cap_pi(sock->sk)->chan->dst);

which can very easily oops - here ->chan is already garbage and we proceed to dereference that. As with HIDP, one needs CAP_NET_ADMIN to trigger that, but it's really a clear bug. The only sanity check we do is verifying that nsock->sk->sk_state is equal to BT_CONNECTED, which is not unique to bluetooth, to put it mildly. It's just 1, so a TCP_ESTABLISHED tcp socket will pass that check just fune. The fix is trivial...
* BNEP situation is identical to CMTP one.

I've sent these patches back then (March 10), but they seem to have fallen through the cracks. The bugs are still there and the fixes still apply. If you would prefer me to resend them after -rc1, just tell...

Anyway, patches follow

#1 lore raw

PATCH 1/3 bluetooth: hidp_connection_add() unsafe use of l2cap_pi()

From: Al Viro <[email protected]>

it's OK after we'd verified the sockets, but not before that.

Signed-off-by Al Viro <[email protected]>
---
 net/bluetooth/hidp/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
net/bluetooth/hidp/core.c +2 -1
index cc25d0b..07348e1 100644
@@ -1314,13 +1314,14 @@ int hidp_connection_add(struct hidp_connadd_req *req,
 {
 	struct hidp_session *session;
 	struct l2cap_conn *conn;
-	struct l2cap_chan *chan = l2cap_pi(ctrl_sock->sk)->chan;
+	struct l2cap_chan *chan;
 	int ret;
 
 	ret = hidp_verify_sockets(ctrl_sock, intr_sock);
 	if (ret)
 		return ret;
 
+	chan = l2cap_pi(ctrl_sock->sk)->chan;
 	conn = NULL;
 	l2cap_chan_lock(chan);
 	if (chan->conn)
Signature · 1 line
2.1.3
#3 lore raw

PATCH 2/3 cmtp: cmtp_add_connection() should verify that it's dealing with l2cap socket

In reply to Al Viro

From: Al Viro <[email protected]>

... rather than relying on ciptool(8) never passing it anything else. Give it e.g. an AF_UNIX connected socket (from socketpair(2)) and it'll oops, trying to evaluate &l2cap_pi(sock->sk)->chan->dst...

Signed-off-by Al Viro <[email protected]>
---
 net/bluetooth/cmtp/core.c | 2 ++
 1 file changed, 2 insertions(+)
net/bluetooth/cmtp/core.c +2 -0
index 67fe5e8..fd57db8 100644
@@ -333,6 +333,8 @@ int cmtp_add_connection(struct cmtp_connadd_req *req, struct socket *sock)
 	int i, err;
 
 	BT_DBG("");
+	if (!l2cap_is_socket(sock))
+		return -EBADFD;
 
 	session = kzalloc(sizeof(struct cmtp_session), GFP_KERNEL);
 	if (!session)
Signature · 1 line
2.1.3
#5 lore raw

PATCH 3/3 bnep: bnep_add_connection() should verify that it's dealing with l2cap socket

In reply to Al Viro

From: Al Viro <[email protected]>

same story as cmtp

Signed-off-by Al Viro <[email protected]>
---
 net/bluetooth/bnep/core.c | 3 +++
 1 file changed, 3 insertions(+)
net/bluetooth/bnep/core.c +3 -0
index 85bcc21..ce82722d0 100644
@@ -533,6 +533,9 @@ int bnep_add_connection(struct bnep_connadd_req *req, struct socket *sock)
 
 	BT_DBG("");
 
+	if (!l2cap_is_socket(sock))
+		return -EBADFD;
+
 	baswap((void *) dst, &l2cap_pi(sock->sk)->chan->dst);
 	baswap((void *) src, &l2cap_pi(sock->sk)->chan->src);
Signature · 1 line
2.1.3
maintainer
#7 lore raw
In reply to Al Viro

Hi Al,

Quoted 23 lines

This stuff has been sitting in my queue since March; basically,
several places in net/bluetooth assume that they are dealing with l2cap sockets, while it is possible to get an arbitrary socket to those. Results are not pretty.
* HIDPCONNADD gets an arbitrary user-supplied socket; the code
it calls (hidp_connection_add()) verifies that the socket is l2cap one, but before doing so it finds l2cap_pi(ctrl_sock->sk)->chan. It's not that big a deal (it's only 5 words past the end of struct sock), but it's trivial to avoid and, in theory, we might end up oopsing here if we are very unlucky and it happens to hit an unmapped page just past the actual object ctrl_sock->sk sits in.
* CMTP counterpart of that doesn't validate the socket at all.
It proceeds to

       s = __cmtp_get_session(&l2cap_pi(sock->sk)->chan->dst);

which can very easily oops - here ->chan is already garbage and we proceed to dereference that. As with HIDP, one needs CAP_NET_ADMIN to trigger that, but it's really a clear bug. The only sanity check we do is verifying that nsock->sk->sk_state is equal to BT_CONNECTED, which is not unique to bluetooth, to put it mildly. It's just 1, so a TCP_ESTABLISHED tcp socket will pass that check just fune. The fix is trivial...
* BNEP situation is identical to CMTP one.

Quoted 3 lines

I've sent these patches back then (March 10), but they seem to have fallen through the cracks. The bugs are still there and the fixes still apply. If you would prefer me to resend them after -rc1, just tell...

they must have really fallen through the cracks since I do not even remember them.

My take is that these should all go in before -rc1 and preferable also make it into stable. While you need CAP_NET_ADMIN capability, there are clear stupid bugs on our side.

Dave, we can prepare a pull request for these or do you want to take them directly into net tree?

Regards

Marcel

maintainer Acked-by
#8 lore raw

Hi Dave,

Quoted 27 lines
Quoted 23 lines

This stuff has been sitting in my queue since March; basically,
several places in net/bluetooth assume that they are dealing with l2cap sockets, while it is possible to get an arbitrary socket to those. Results are not pretty.
* HIDPCONNADD gets an arbitrary user-supplied socket; the code
it calls (hidp_connection_add()) verifies that the socket is l2cap one, but before doing so it finds l2cap_pi(ctrl_sock->sk)->chan. It's not that big a deal (it's only 5 words past the end of struct sock), but it's trivial to avoid and, in theory, we might end up oopsing here if we are very unlucky and it happens to hit an unmapped page just past the actual object ctrl_sock->sk sits in.
* CMTP counterpart of that doesn't validate the socket at all.
It proceeds to

      s = __cmtp_get_session(&l2cap_pi(sock->sk)->chan->dst);

which can very easily oops - here ->chan is already garbage and we proceed to dereference that. As with HIDP, one needs CAP_NET_ADMIN to trigger that, but it's really a clear bug. The only sanity check we do is verifying that nsock->sk->sk_state is equal to BT_CONNECTED, which is not unique to bluetooth, to put it mildly. It's just 1, so a TCP_ESTABLISHED tcp socket will pass that check just fune. The fix is trivial...
* BNEP situation is identical to CMTP one.

Quoted 3 lines

I've sent these patches back then (March 10), but they seem to have fallen through the cracks. The bugs are still there and the fixes still apply. If you would prefer me to resend them after -rc1, just tell...

Quoted 5 lines

they must have really fallen through the cracks since I do not even remember them.

My take is that these should all go in before -rc1 and preferable also make it into stable. While you need CAP_NET_ADMIN capability, there are clear stupid bugs on our side.

Dave, we can prepare a pull request for these or do you want to take them directly into net tree?

and in case you decide to take them directly.

Acked-by Marcel Holtmann <[email protected]>

Regards

Marcel

maintainer
#4 lore raw

Re: [PATCH 2/3] cmtp: cmtp_add_connection() should verify that it's dealing with l2cap socket

In reply to Al Viro

Hi Al,

Quoted 10 lines

From: Al Viro <[email protected]>

... rather than relying on ciptool(8) never passing it anything else. Give it e.g. an AF_UNIX connected socket (from socketpair(2)) and it'll oops, trying to evaluate &l2cap_pi(sock->sk)->chan->dst...

Signed-off-by Al Viro <[email protected]>

---
net/bluetooth/cmtp/core.c | 2 ++

1 file changed, 2 insertions(+)

patch has been applied to bluetooth tree.

Regards

Marcel

maintainer
#6 lore raw

Re: [PATCH 3/3] bnep: bnep_add_connection() should verify that it's dealing with l2cap socket

In reply to Al Viro

Hi Al,

Quoted 6 lines

same story as cmtp

Signed-off-by Al Viro <[email protected]>

---
net/bluetooth/bnep/core.c | 3 +++

1 file changed, 3 insertions(+)

patch has been applied to bluetooth tree.

Regards

Marcel

maintainer
#2 lore raw

Re: [PATCH 1/3] bluetooth: hidp_connection_add() unsafe use of l2cap_pi()

In reply to Al Viro

Hi Al,

Quoted 6 lines

it's OK after we'd verified the sockets, but not before that.

Signed-off-by Al Viro <[email protected]>

---
net/bluetooth/hidp/core.c | 3 ++-

1 file changed, 2 insertions(+), 1 deletion(-)

patch has been applied to bluetooth tree.

Regards

Marcel

#13 lore raw
In reply to a message not in the archive

Hi Dave,

Quoted 27 lines
Quoted 23 lines

This stuff has been sitting in my queue since March; basically,
several places in net/bluetooth assume that they are dealing with l2cap sockets, while it is possible to get an arbitrary socket to those. Results are not pretty.
* HIDPCONNADD gets an arbitrary user-supplied socket; the code
it calls (hidp_connection_add()) verifies that the socket is l2cap one, but before doing so it finds l2cap_pi(ctrl_sock->sk)->chan. It's not that big a deal (it's only 5 words past the end of struct sock), but it's trivial to avoid and, in theory, we might end up oopsing here if we are very unlucky and it happens to hit an unmapped page just past the actual object ctrl_sock->sk sits in.
* CMTP counterpart of that doesn't validate the socket at all.
It proceeds to

      s = __cmtp_get_session(&l2cap_pi(sock->sk)->chan->dst);

which can very easily oops - here ->chan is already garbage and we proceed to dereference that. As with HIDP, one needs CAP_NET_ADMIN to trigger that, but it's really a clear bug. The only sanity check we do is verifying that nsock->sk->sk_state is equal to BT_CONNECTED, which is not unique to bluetooth, to put it mildly. It's just 1, so a TCP_ESTABLISHED tcp socket will pass that check just fune. The fix is trivial...
* BNEP situation is identical to CMTP one.

Quoted 3 lines

I've sent these patches back then (March 10), but they seem to have fallen through the cracks. The bugs are still there and the fixes still apply. If you would prefer me to resend them after -rc1, just tell...

Quoted 5 lines

they must have really fallen through the cracks since I do not even remember them.

My take is that these should all go in before -rc1 and preferable also make it into stable. While you need CAP_NET_ADMIN capability, there are clear stupid bugs on our side.

Dave, we can prepare a pull request for these or do you want to take them directly into net tree?

never mind that. We have another bug in 6LoWPAN with wrongly freeing a skb. I took all 3 of Al's patches now and I will ask Johan to send you a pull request for the whole set for net tree inclusion.

Regards

Marcel

maintainer
#11 lore raw

Hi Dave,

Quoted 7 lines
Quoted 5 lines
Quoted 1 line

Dave, we can prepare a pull request for these or do you want to take them directly into net tree?

and in case you decide to take them directly.

Acked-by Marcel Holtmann <[email protected]>

Feel free to create a pull request for me, that works best.

actually Johan already sent one. Should be in your inbox and on netdev.

Regards

Marcel