[PATCH 0/2] Bug fixes for RFCOMM and L2CAP

[PATCH 0/2] Bug fixes for RFCOMM and L2CAP

[Mat Martineau]

Here are two bug fixes for 
git://git.kernel.org/pub/scm/linux/kernel/git/padovan/bluetooth.git

The first is a pretty obvious fix where an RFCOMM disconnect timer
needs to be cancelled when the disconnect happens earlier.  It's not
clear why the reference counting logic breaks down if this timer is
not cancelled, but it's definitely more correct to cancel the timer.

The L2CAP fix came up due to a buggy PTS test, which was not sending
the L2CAP RFC option in an ERTM config response.  This was causing
access to uninitialized data.

Mat Martineau (2):
  Bluetooth: Clear RFCOMM session timer when disconnecting last channel
  Bluetooth: Prevent uninitialized data access in L2CAP configuration

 net/bluetooth/l2cap_core.c  |   16 +++++++++++++++-
 net/bluetooth/rfcomm/core.c |    1 +
 2 files changed, 16 insertions(+), 1 deletions(-)

-- 
1.7.8

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[PATCH 1/2] Bluetooth: Clear RFCOMM session timer when disconnecting last channel

[Mat Martineau]

When the last RFCOMM data channel is closed, a timer is normally set
up to disconnect the control channel at a later time.  If the control
channel disconnect command is sent with the timer pending, the timer
needs to be cancelled.

If the timer is not cancelled in this situation, the reference
counting logic for the RFCOMM session does not work correctly when the
remote device closes the L2CAP connection.  The session is freed at
the wrong time, leading to a kernel panic.

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 net/bluetooth/rfcomm/core.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
index 4e32e18..2d28dfe 100644
--- a/net/bluetooth/rfcomm/core.c
+++ b/net/bluetooth/rfcomm/core.c
@@ -1146,6 +1146,7 @@ static int rfcomm_recv_ua(struct rfcomm_session *s, u8 dlci)
 			if (list_empty(&s->dlcs)) {
 				s->state = BT_DISCONN;
 				rfcomm_send_disc(s, 0);
+				rfcomm_session_clear_timer(s);
 			}
 
 			break;
-- 
1.7.8

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [PATCH 1/2] Bluetooth: Clear RFCOMM session timer when disconnecting last channel

[Marcel Holtmann]

Hi Mat,

> When the last RFCOMM data channel is closed, a timer is normally set
> up to disconnect the control channel at a later time.  If the control
> channel disconnect command is sent with the timer pending, the timer
> needs to be cancelled.
> 
> If the timer is not cancelled in this situation, the reference
> counting logic for the RFCOMM session does not work correctly when the
> remote device closes the L2CAP connection.  The session is freed at
> the wrong time, leading to a kernel panic.
> 
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  net/bluetooth/rfcomm/core.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
> index 4e32e18..2d28dfe 100644
> --- a/net/bluetooth/rfcomm/core.c
> +++ b/net/bluetooth/rfcomm/core.c
> @@ -1146,6 +1146,7 @@ static int rfcomm_recv_ua(struct rfcomm_session *s, u8 dlci)
>  			if (list_empty(&s->dlcs)) {
>  				s->state = BT_DISCONN;
>  				rfcomm_send_disc(s, 0);
> +				rfcomm_session_clear_timer(s);
>  			}

I am not 100% convinced that this is correct, but going through the code
for over an hour I could not come up with a case where this would cause
problems. It is just a feeling and with RFCOMM that could be just a
bogus feeling ;)

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [PATCH 1/2] Bluetooth: Clear RFCOMM session timer when disconnecting last channel

[Mat Martineau]

On Thu, 8 Dec 2011, Marcel Holtmann wrote:

> Hi Mat,
>
>> When the last RFCOMM data channel is closed, a timer is normally set
>> up to disconnect the control channel at a later time.  If the control
>> channel disconnect command is sent with the timer pending, the timer
>> needs to be cancelled.
>>
>> If the timer is not cancelled in this situation, the reference
>> counting logic for the RFCOMM session does not work correctly when the
>> remote device closes the L2CAP connection.  The session is freed at
>> the wrong time, leading to a kernel panic.
>>
>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
>> ---
>>  net/bluetooth/rfcomm/core.c |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
>> index 4e32e18..2d28dfe 100644
>> --- a/net/bluetooth/rfcomm/core.c
>> +++ b/net/bluetooth/rfcomm/core.c
>> @@ -1146,6 +1146,7 @@ static int rfcomm_recv_ua(struct rfcomm_session *s, u8 dlci)
>>  			if (list_empty(&s->dlcs)) {
>>  				s->state = BT_DISCONN;
>>  				rfcomm_send_disc(s, 0);
>> +				rfcomm_session_clear_timer(s);
>>  			}
>
> I am not 100% convinced that this is correct, but going through the code
> for over an hour I could not come up with a case where this would cause
> problems. It is just a feeling and with RFCOMM that could be just a
> bogus feeling ;)
>
> Acked-by: Marcel Holtmann <marcel@holtmann.org>

The purpose of the timer is to set s->state to BT_DISCONN and call 
rfcomm_send_disc(s, 0) - if those actions have already happened here, 
it doesn't make sense to do so again after a timeout!  100% convinced 
yet?

There may still be a reference counting bug lurking in there when the 
underlying L2CAP socket unexpectedly closes, but this patch fixed a 
very repeatable kernel panic for us.

Thanks for the ack,

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [PATCH 1/2] Bluetooth: Clear RFCOMM session timer when disconnecting last channel

[Marcel Holtmann]

Hi Mat,

> >> When the last RFCOMM data channel is closed, a timer is normally set
> >> up to disconnect the control channel at a later time.  If the control
> >> channel disconnect command is sent with the timer pending, the timer
> >> needs to be cancelled.
> >>
> >> If the timer is not cancelled in this situation, the reference
> >> counting logic for the RFCOMM session does not work correctly when the
> >> remote device closes the L2CAP connection.  The session is freed at
> >> the wrong time, leading to a kernel panic.
> >>
> >> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> >> ---
> >>  net/bluetooth/rfcomm/core.c |    1 +
> >>  1 files changed, 1 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
> >> index 4e32e18..2d28dfe 100644
> >> --- a/net/bluetooth/rfcomm/core.c
> >> +++ b/net/bluetooth/rfcomm/core.c
> >> @@ -1146,6 +1146,7 @@ static int rfcomm_recv_ua(struct rfcomm_session *s, u8 dlci)
> >>  			if (list_empty(&s->dlcs)) {
> >>  				s->state = BT_DISCONN;
> >>  				rfcomm_send_disc(s, 0);
> >> +				rfcomm_session_clear_timer(s);
> >>  			}
> >
> > I am not 100% convinced that this is correct, but going through the code
> > for over an hour I could not come up with a case where this would cause
> > problems. It is just a feeling and with RFCOMM that could be just a
> > bogus feeling ;)
> >
> > Acked-by: Marcel Holtmann <marcel@holtmann.org>
> 
> The purpose of the timer is to set s->state to BT_DISCONN and call 
> rfcomm_send_disc(s, 0) - if those actions have already happened here, 
> it doesn't make sense to do so again after a timeout!  100% convinced 
> yet?

I just close my eyes and believe you ;)

But on a serious note, can you get the BITE tester run the RFCOMM suite
against this change. Just to make sure it did not accidentally mess up
the qualification.

Regards

Marcel

[PATCH 2/2] Bluetooth: Prevent uninitialized data access in L2CAP configuration

[Mat Martineau]

When configuring an ERTM or streaming mode connection, remote devices
are expected to send an RFC option in a successful config response.  A
misbehaving remote device might not send an RFC option, and the L2CAP
code should not access uninitialized data in this case.

Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
 net/bluetooth/l2cap_core.c |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 5ea94a1..49ae7df 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -2152,7 +2152,7 @@ static int l2cap_parse_conf_rsp(struct l2cap_chan *chan, void *rsp, int len, voi
 	void *ptr = req->data;
 	int type, olen;
 	unsigned long val;
-	struct l2cap_conf_rfc rfc;
+	struct l2cap_conf_rfc rfc = { .mode = L2CAP_MODE_BASIC };
 
 	BT_DBG("chan %p, rsp %p, len %d, req %p", chan, rsp, len, data);
 
@@ -2205,6 +2205,9 @@ static int l2cap_parse_conf_rsp(struct l2cap_chan *chan, void *rsp, int len, voi
 			break;
 		case L2CAP_MODE_STREAMING:
 			chan->mps    = le16_to_cpu(rfc.max_pdu_size);
+			break;
+		default:
+			break;
 		}
 	}
 
@@ -2271,6 +2274,14 @@ static void l2cap_conf_rfc_get(struct l2cap_chan *chan, void *rsp, int len)
 		}
 	}
 
+	/* Use sane default values in case a misbehaving remote device
+	 * did not send an RFC option.
+	 */
+	rfc.mode = chan->mode;
+	rfc.retrans_timeout = cpu_to_le16(chan->retrans_timeout);
+	rfc.monitor_timeout = cpu_to_le16(chan->monitor_timeout);
+	rfc.max_pdu_size = cpu_to_le16(chan->mps);
+
 done:
 	switch (rfc.mode) {
 	case L2CAP_MODE_ERTM:
@@ -2280,6 +2291,9 @@ done:
 		break;
 	case L2CAP_MODE_STREAMING:
 		chan->mps    = le16_to_cpu(rfc.max_pdu_size);
+		break;
+	default:
+		break;
 	}
 }
 
-- 
1.7.8

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [PATCH 2/2] Bluetooth: Prevent uninitialized data access in L2CAP configuration

[Marcel Holtmann]

Hi Mat,

> When configuring an ERTM or streaming mode connection, remote devices
> are expected to send an RFC option in a successful config response.  A
> misbehaving remote device might not send an RFC option, and the L2CAP
> code should not access uninitialized data in this case.
> 
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
>  net/bluetooth/l2cap_core.c |   16 +++++++++++++++-
>  1 files changed, 15 insertions(+), 1 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 5ea94a1..49ae7df 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -2152,7 +2152,7 @@ static int l2cap_parse_conf_rsp(struct l2cap_chan *chan, void *rsp, int len, voi
>  	void *ptr = req->data;
>  	int type, olen;
>  	unsigned long val;
> -	struct l2cap_conf_rfc rfc;
> +	struct l2cap_conf_rfc rfc = { .mode = L2CAP_MODE_BASIC };
>  
>  	BT_DBG("chan %p, rsp %p, len %d, req %p", chan, rsp, len, data);
>  
> @@ -2205,6 +2205,9 @@ static int l2cap_parse_conf_rsp(struct l2cap_chan *chan, void *rsp, int len, voi
>  			break;
>  		case L2CAP_MODE_STREAMING:
>  			chan->mps    = le16_to_cpu(rfc.max_pdu_size);
> +			break;
> +		default:
> +			break;

Adding a BT_ERR here would be a good idea.

>  		}
>  	}
>  
> @@ -2271,6 +2274,14 @@ static void l2cap_conf_rfc_get(struct l2cap_chan *chan, void *rsp, int len)
>  		}
>  	}
>  
> +	/* Use sane default values in case a misbehaving remote device
> +	 * did not send an RFC option.
> +	 */
> +	rfc.mode = chan->mode;
> +	rfc.retrans_timeout = cpu_to_le16(chan->retrans_timeout);
> +	rfc.monitor_timeout = cpu_to_le16(chan->monitor_timeout);
> +	rfc.max_pdu_size = cpu_to_le16(chan->mps);
> +
>  done:
>  	switch (rfc.mode) {
>  	case L2CAP_MODE_ERTM:
> @@ -2280,6 +2291,9 @@ done:
>  		break;
>  	case L2CAP_MODE_STREAMING:
>  		chan->mps    = le16_to_cpu(rfc.max_pdu_size);
> +		break;
> +	default:
> +		break;

Also adding a BT_ERR seems like the right thing to do.

Otherwise patch looks good to me.

Regards

Marcel

Re: [PATCH 2/2] Bluetooth: Prevent uninitialized data access in L2CAP configuration

[Mat Martineau]

On Thu, 8 Dec 2011, Marcel Holtmann wrote:

> Hi Mat,
>
>> When configuring an ERTM or streaming mode connection, remote devices
>> are expected to send an RFC option in a successful config response.  A
>> misbehaving remote device might not send an RFC option, and the L2CAP
>> code should not access uninitialized data in this case.
>>
>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
>> ---
>>  net/bluetooth/l2cap_core.c |   16 +++++++++++++++-
>>  1 files changed, 15 insertions(+), 1 deletions(-)
>>
>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>> index 5ea94a1..49ae7df 100644
>> --- a/net/bluetooth/l2cap_core.c
>> +++ b/net/bluetooth/l2cap_core.c
>> @@ -2152,7 +2152,7 @@ static int l2cap_parse_conf_rsp(struct l2cap_chan *chan, void *rsp, int len, voi
>>  	void *ptr = req->data;
>>  	int type, olen;
>>  	unsigned long val;
>> -	struct l2cap_conf_rfc rfc;
>> +	struct l2cap_conf_rfc rfc = { .mode = L2CAP_MODE_BASIC };
>>
>>  	BT_DBG("chan %p, rsp %p, len %d, req %p", chan, rsp, len, data);
>>
>> @@ -2205,6 +2205,9 @@ static int l2cap_parse_conf_rsp(struct l2cap_chan *chan, void *rsp, int len, voi
>>  			break;
>>  		case L2CAP_MODE_STREAMING:
>>  			chan->mps    = le16_to_cpu(rfc.max_pdu_size);
>> +			break;
>> +		default:
>> +			break;
>
> Adding a BT_ERR here would be a good idea.
>

That default case is expected when dealing with L2CAP_CONF_UNACCEPT 
config responses in basic mode, or when an RFC option is returned with 
basic mode.  I'll just remove this part of the patch, since it is 
valid to omit the "default" case.  Initializing rfc.mode above is the 
important part.

If an expected RFC is missing, we'll see the same behavior as if an 
RFC option with basic mode was received.

>>  		}
>>  	}
>>
>> @@ -2271,6 +2274,14 @@ static void l2cap_conf_rfc_get(struct l2cap_chan *chan, void *rsp, int len)
>>  		}
>>  	}
>>
>> +	/* Use sane default values in case a misbehaving remote device
>> +	 * did not send an RFC option.
>> +	 */
>> +	rfc.mode = chan->mode;
>> +	rfc.retrans_timeout = cpu_to_le16(chan->retrans_timeout);
>> +	rfc.monitor_timeout = cpu_to_le16(chan->monitor_timeout);
>> +	rfc.max_pdu_size = cpu_to_le16(chan->mps);
>> +
>>  done:
>>  	switch (rfc.mode) {
>>  	case L2CAP_MODE_ERTM:
>> @@ -2280,6 +2291,9 @@ done:
>>  		break;
>>  	case L2CAP_MODE_STREAMING:
>>  		chan->mps    = le16_to_cpu(rfc.max_pdu_size);
>> +		break;
>> +	default:
>> +		break;
>
> Also adding a BT_ERR seems like the right thing to do.

Ok.

>
> Otherwise patch looks good to me.


--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [PATCH 0/2] Bug fixes for RFCOMM and L2CAP

[Mat Martineau]

Gustavo -

On Tue, 6 Dec 2011, Mat Martineau wrote:

> Here are two bug fixes for
> git://git.kernel.org/pub/scm/linux/kernel/git/padovan/bluetooth.git
>
> The first is a pretty obvious fix where an RFCOMM disconnect timer
> needs to be cancelled when the disconnect happens earlier.  It's not
> clear why the reference counting logic breaks down if this timer is
> not cancelled, but it's definitely more correct to cancel the timer.
>
> The L2CAP fix came up due to a buggy PTS test, which was not sending
> the L2CAP RFC option in an ERTM config response.  This was causing
> access to uninitialized data.
>
> Mat Martineau (2):
>  Bluetooth: Clear RFCOMM session timer when disconnecting last channel
>  Bluetooth: Prevent uninitialized data access in L2CAP configuration
>
> net/bluetooth/l2cap_core.c  |   16 +++++++++++++++-
> net/bluetooth/rfcomm/core.c |    1 +
> 2 files changed, 16 insertions(+), 1 deletions(-)
>
> -- 
> 1.7.8

Did you get a chance to look at these patches?  Note that there is a 
v2 patch for the L2CAP fix.  They fix uninitialized data access and an 
RFCOMM panic.

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum