[PATCH] Bluetooth: Fix potential NULL dereference

[PATCH] Bluetooth: Fix potential NULL dereference

[Johan Hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

The bnep_get_device function may be triggered by an ioctl just after a
connection has gone down. In such a case the respective L2CAP chan->conn
pointer will get set to NULL (by l2cap_chan_del). This patch adds a
missing NULL check for this case in the bnep_get_device() function.

Reported-by: Patrik Flykt <patrik.flykt@linux.intel.com>
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 net/bluetooth/bnep/core.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/bnep/core.c b/net/bluetooth/bnep/core.c
index ce82722d049b..05f57e491ccb 100644
--- a/net/bluetooth/bnep/core.c
+++ b/net/bluetooth/bnep/core.c
@@ -511,13 +511,12 @@ static int bnep_session(void *arg)
 
 static struct device *bnep_get_device(struct bnep_session *session)
 {
-	struct hci_conn *conn;
+	struct l2cap_conn *conn = l2cap_pi(session->sock->sk)->chan->conn;
 
-	conn = l2cap_pi(session->sock->sk)->chan->conn->hcon;
-	if (!conn)
+	if (!conn || !conn->hcon)
 		return NULL;
 
-	return &conn->dev;
+	return &conn->hcon->dev;
 }
 
 static struct device_type bnep_type = {
-- 
2.1.0

Re: [PATCH] Bluetooth: Fix potential NULL dereference

[Marcel Holtmann]

Hi Johan,

> The bnep_get_device function may be triggered by an ioctl just after a
> connection has gone down. In such a case the respective L2CAP chan->conn
> pointer will get set to NULL (by l2cap_chan_del). This patch adds a
> missing NULL check for this case in the bnep_get_device() function.
> 
> Reported-by: Patrik Flykt <patrik.flykt@linux.intel.com>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/bnep/core.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel

[PATCH] Bluetooth: Fix potential NULL dereference

[Jaganath Kanakkassery]

addr can be NULL and it should not be dereferenced before NULL checking.

Signed-off-by: Jaganath Kanakkassery <jaganath.k@samsung.com>
---
 net/bluetooth/rfcomm/sock.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
index 825e8fb..e576374 100644
--- a/net/bluetooth/rfcomm/sock.c
+++ b/net/bluetooth/rfcomm/sock.c
@@ -336,14 +336,16 @@ static int rfcomm_sock_bind(struct socket *sock, struct sockaddr *addr, int addr
 {
 	struct sockaddr_rc *sa = (struct sockaddr_rc *) addr;
 	struct sock *sk = sock->sk;
-	int chan = sa->rc_channel;
+	int chan;
 	int err = 0;
 
-	BT_DBG("sk %p %pMR", sk, &sa->rc_bdaddr);
-
 	if (!addr || addr->sa_family != AF_BLUETOOTH)
 		return -EINVAL;
 
+	BT_DBG("sk %p %pMR", sk, &sa->rc_bdaddr);
+
+	chan = sa->rc_channel;
+
 	lock_sock(sk);
 
 	if (sk->sk_state != BT_OPEN) {
-- 
1.7.9.5

Re: [PATCH] Bluetooth: Fix potential NULL dereference

[Marcel Holtmann]

Hi Jaganath,

> addr can be NULL and it should not be dereferenced before NULL checking.
> 
> Signed-off-by: Jaganath Kanakkassery <jaganath.k@samsung.com>
> ---
> net/bluetooth/rfcomm/sock.c |    8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
> index 825e8fb..e576374 100644
> --- a/net/bluetooth/rfcomm/sock.c
> +++ b/net/bluetooth/rfcomm/sock.c
> @@ -336,14 +336,16 @@ static int rfcomm_sock_bind(struct socket *sock, struct sockaddr *addr, int addr
> {
> 	struct sockaddr_rc *sa = (struct sockaddr_rc *) addr;
> 	struct sock *sk = sock->sk;
> -	int chan = sa->rc_channel;
> +	int chan;
> 	int err = 0;
> 
> -	BT_DBG("sk %p %pMR", sk, &sa->rc_bdaddr);
> -
> 	if (!addr || addr->sa_family != AF_BLUETOOTH)
> 		return -EINVAL;
> 
> +	BT_DBG("sk %p %pMR", sk, &sa->rc_bdaddr);
> +
> +	chan = sa->rc_channel;
> +

if we start changing things here, then we better change the code into something that all the other socket handling code is doing anyway. So do the min comparison and copy the data into a local copy of the sockaddr_rc.

And on a side note, I wonder if addr can actually be NULL. It might be interesting to check the generic socket code if this really can happen if you provide no address structure to the bind() system call or if this gets filtered out by the core socket code.

Regards

Marcel

Re: [PATCH] Bluetooth: Fix potential NULL dereference

[Marcel Holtmann]

Hi Jaganath,

>>> addr can be NULL and it should not be dereferenced before NULL checking.
>>> 
>>> Signed-off-by: Jaganath Kanakkassery <jaganath.k@samsung.com>
>>> ---
>> 
>> if we start changing things here, then we better change the code into something that all the other socket handling code is doing anyway>y. So do the min comparison and copy the data into a local copy of the sockaddr_rc.
>> 
>> And on a side note, I wonder if addr can actually be NULL. It might be interesting to check the generic socket code if this really can happe>n if you provide no address structure to the bind() system call or if this gets filtered out by the core socket code.
> 
> I checked generic socket code and it looks like addr will never be NULL when user space calls bind.
> But this can be called from kernel_bind() also which I think will never be called for RFCOMM.
> So this patch is not required? 

that is what I thought. However converting it to the same handling using min and copying into local storage might be a good idea. The more pieces in HCI, L2CAP, SCO and RFCOMM sockets that are similar, the better.

Regards

Marcel

Re: [PATCH] Bluetooth: Fix potential NULL dereference

[Marcel Holtmann]

Hi Jaganath,

> Ping.

so we agreed to not apply this patch. I might have missed your other patch. So just resend that one.

Regards

Marcel

Re: [PATCH] Bluetooth: Fix potential NULL dereference

[chanyeol]

Hi Marcel,

On Thu, 2015-05-14 at 08:34 +0200, Marcel Holtmann wrote:
> Hi Jaganath,
> 
> > > > addr can be NULL and it should not be dereferenced before NULL 
> > > > checking.
> > > > 
> > > > Signed-off-by: Jaganath Kanakkassery <jaganath.k@samsung.com>
> > > > ---
> > > 
> > > if we start changing things here, then we better change the code 
> > > into something that all the other socket handling code is doing 
> > > anyway>y. So do the min comparison and copy the data into a local 
> > > copy of the sockaddr_rc.
> > > 
> > > And on a side note, I wonder if addr can actually be NULL. It 
> > > might be interesting to check the generic socket code if this 
> > > really can happe>n if you provide no address structure to the 
> > > bind() system call or if this gets filtered out by the core 
> > > socket code.
> > 
> > I checked generic socket code and it looks like addr will never be 
> > NULL when user space calls bind.
> > But this can be called from kernel_bind() also which I think will 
> > never be called for RFCOMM.
> > So this patch is not required? 
> 
> that is what I thought. However converting it to the same handling 
> using min and copying into local storage might be a good idea.
Could you tell us why this is good idea? I failed to find it in git
history/mailing list.

In addition to RFCOMM connect that you mentioned, I found out SCO
connect/bind still use the old style in Bluetooth unlikely HCI,L2CAP.

Regards
Chanyeol

>  The more pieces in HCI, L2CAP, SCO and RFCOMM sockets that are 
> similar, the better.
> 
> Regards
> 
> Marcel
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux
> -bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Bluetooth: Fix potential NULL dereference

[Marcel Holtmann]

Hi Chanyeol,

>>>>> addr can be NULL and it should not be dereferenced before NULL 
>>>>> checking.
>>>>> 
>>>>> Signed-off-by: Jaganath Kanakkassery <jaganath.k@samsung.com>
>>>>> ---
>>>> 
>>>> if we start changing things here, then we better change the code 
>>>> into something that all the other socket handling code is doing 
>>>> anyway>y. So do the min comparison and copy the data into a local 
>>>> copy of the sockaddr_rc.
>>>> 
>>>> And on a side note, I wonder if addr can actually be NULL. It 
>>>> might be interesting to check the generic socket code if this 
>>>> really can happe>n if you provide no address structure to the 
>>>> bind() system call or if this gets filtered out by the core 
>>>> socket code.
>>> 
>>> I checked generic socket code and it looks like addr will never be 
>>> NULL when user space calls bind.
>>> But this can be called from kernel_bind() also which I think will 
>>> never be called for RFCOMM.
>>> So this patch is not required? 
>> 
>> that is what I thought. However converting it to the same handling 
>> using min and copying into local storage might be a good idea.
> Could you tell us why this is good idea? I failed to find it in git
> history/mailing list.
> 
> In addition to RFCOMM connect that you mentioned, I found out SCO
> connect/bind still use the old style in Bluetooth unlikely HCI,L2CAP.

it avoids any kind of leakage and allows for an easy extension of the socket data structures if we ever have to.

Regards

Marcel