[patches] a bunch of old bluetooth fixes

[patches] a bunch of old bluetooth fixes

[Al Viro]

	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

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

[Al Viro]

From: Al Viro <viro@zeniv.linux.org.uk>

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

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 net/bluetooth/hidp/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index cc25d0b..07348e1 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -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)
-- 
2.1.3

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

[Marcel Holtmann]

Hi Al,

> it's OK after we'd verified the sockets, but not before that.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
> net/bluetooth/hidp/core.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)

patch has been applied to bluetooth tree.

Regards

Marcel

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

[Al Viro]

From: Al Viro <viro@zeniv.linux.org.uk>

... 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 <viro@zeniv.linux.org.uk>
---
 net/bluetooth/cmtp/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/bluetooth/cmtp/core.c b/net/bluetooth/cmtp/core.c
index 67fe5e8..fd57db8 100644
--- a/net/bluetooth/cmtp/core.c
+++ b/net/bluetooth/cmtp/core.c
@@ -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)
-- 
2.1.3

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

[Marcel Holtmann]

Hi Al,

> From: Al Viro <viro@zeniv.linux.org.uk>
> 
> ... 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 <viro@zeniv.linux.org.uk>
> ---
> net/bluetooth/cmtp/core.c | 2 ++
> 1 file changed, 2 insertions(+)

patch has been applied to bluetooth tree.

Regards

Marcel

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

[Al Viro]

From: Al Viro <viro@zeniv.linux.org.uk>

same story as cmtp

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 net/bluetooth/bnep/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/bluetooth/bnep/core.c b/net/bluetooth/bnep/core.c
index 85bcc21..ce82722d0 100644
--- a/net/bluetooth/bnep/core.c
+++ b/net/bluetooth/bnep/core.c
@@ -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);
 
-- 
2.1.3

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

[Marcel Holtmann]

Hi Al,

> same story as cmtp
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
> net/bluetooth/bnep/core.c | 3 +++
> 1 file changed, 3 insertions(+)

patch has been applied to bluetooth tree.

Regards

Marcel

Re: [patches] a bunch of old bluetooth fixes

[Marcel Holtmann]

Hi Al,

> 	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...

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

Re: [patches] a bunch of old bluetooth fixes

[Marcel Holtmann]

Hi Dave,

>> 	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...
> 
> 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 <marcel@holtmann.org>

Regards

Marcel

Re: [patches] a bunch of old bluetooth fixes

[David Miller]

From: Marcel Holtmann <marcel-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org>
Date: Fri, 19 Dec 2014 11:30:38 +0100

>> 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 <marcel-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org>

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

Thanks.

Re: [patches] a bunch of old bluetooth fixes

[Marcel Holtmann]

Hi Dave,

>>> 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 <marcel@holtmann.org>
> 
> 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

Re: [patches] a bunch of old bluetooth fixes

[Marcel Holtmann]

Hi Dave,

>> 	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...
> 
> 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