Bug fixes for RFCOMM and L2CAP

PATCH In review Mat Martineau 2 participants 9 messages Dec 7, 2011 – Dec 16, 2011

Subsystems (2)

Bluetooth drivers Bluetooth

lore

Share & tools

lore

Mat Martineau

author

2011-12-07 00:23 UTC #0 lore raw

cover Bug fixes for RFCOMM and L2CAP

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(-)

Signature · 6 lines

1.7.8

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

Mat Martineau

author

2011-12-07 00:23 UTC #1 lore raw

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

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 <[email protected]>

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

net/bluetooth/rfcomm/core.c +1 -0

index 4e32e18..2d28dfe 100644
@@ -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;

Signature · 6 lines

1.7.8

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

Marcel Holtmann

maintainer Acked-by on v1 1/2

2011-12-08 08:25 UTC #2 lore raw

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

Hi Mat,

Quoted 20 lines

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 <[email protected]>
---
 net/bluetooth/rfcomm/core.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
net/bluetooth/rfcomm/core.c +0 -0
index 4e32e18..2d28dfe 100644

net/bluetooth/rfcomm/core.c static int rfcomm_recv_ua(struct rfcomm_session *s, u8 dlci) · line 1150

Quoted 5 lines

net/bluetooth/rfcomm/core.c +1 -0

@@ -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 <[email protected]>

Regards

Marcel

Mat Martineau

author

2011-12-08 16:57 UTC #3 lore raw

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

Quoted 33 lines · Marcel Holtmann

Hi Mat,
Quoted 20 lines
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 <[email protected]>
---
 net/bluetooth/rfcomm/core.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
net/bluetooth/rfcomm/core.c +0 -0
index 4e32e18..2d28dfe 100644
net/bluetooth/rfcomm/core.c static int rfcomm_recv_ua(struct rfcomm_session *s, u8 dlci) · line 1150
Quoted 5 lines
net/bluetooth/rfcomm/core.c +1 -0
@@ -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 ;)

Quoted 1 line

Acked-by Marcel Holtmann <[email protected]>

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

Marcel Holtmann

maintainer

2011-12-08 22:13 UTC #4 lore raw

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

Hi Mat,

Quoted 31 lines

Quoted 31 lines
Quoted 20 lines
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 <[email protected]>
---
 net/bluetooth/rfcomm/core.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
net/bluetooth/rfcomm/core.c +0 -0
index 4e32e18..2d28dfe 100644
net/bluetooth/rfcomm/core.c static int rfcomm_recv_ua(struct rfcomm_session *s, u8 dlci) · line 1150
Quoted 5 lines
net/bluetooth/rfcomm/core.c +1 -0
@@ -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 ;)

Quoted 6 lines

Quoted 1 line
Acked-by Marcel Holtmann <[email protected]>
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

Mat Martineau

author

2011-12-07 00:23 UTC #5 lore raw

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

In reply to 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 <[email protected]>

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

net/bluetooth/l2cap_core.c +15 -1

index 5ea94a1..49ae7df 100644
@@ -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;
 	}
 }

Signature · 6 lines

1.7.8

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

Marcel Holtmann

maintainer

2011-12-08 08:29 UTC #6 lore raw

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

Hi Mat,

Quoted 24 lines

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 <[email protected]>
---
 net/bluetooth/l2cap_core.c |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)
net/bluetooth/l2cap_core.c +1 -1
index 5ea94a1..49ae7df 100644
@@ -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);
 

net/bluetooth/l2cap_core.c static int l2cap_parse_conf_rsp(struct l2cap_chan *chan, void *rsp, int len, voi · line 2210

Quoted 6 lines

net/bluetooth/l2cap_core.c +3 -0

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

Quoted 19 lines

net/bluetooth/l2cap_core.c +8 -0

 		}
 	}

@@ -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:

net/bluetooth/l2cap_core.c done: · line 2296

Quoted 6 lines

net/bluetooth/l2cap_core.c +3 -0

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

Mat Martineau

author

2011-12-08 21:32 UTC #7 lore raw

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

Quoted 25 lines · Marcel Holtmann

Hi Mat,
Quoted 23 lines
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 <[email protected]>
---
 net/bluetooth/l2cap_core.c |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)
net/bluetooth/l2cap_core.c +1 -1
index 5ea94a1..49ae7df 100644
@@ -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);
 

Quoted 10 lines

net/bluetooth/l2cap_core.c static int l2cap_parse_conf_rsp(struct l2cap_chan *chan, void *rsp, int len, voi · line 2210
Quoted 7 lines
net/bluetooth/l2cap_core.c +3 -0
@@ -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.

Quoted 19 lines

net/bluetooth/l2cap_core.c +8 -0

 		}
 	}

@@ -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:

Quoted 8 lines

net/bluetooth/l2cap_core.c static int l2cap_parse_conf_rsp(struct l2cap_chan *chan, void *rsp, int len, voi · line 2210
Quoted 6 lines
net/bluetooth/l2cap_core.c +3 -0
 		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.

Quoted 2 lines

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

Mat Martineau

author

2011-12-16 23:58 UTC #8 lore raw

In reply to Mat Martineau

Gustavo -

Quoted 12 lines · 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.

Quoted 10 lines

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(-)
Signature · 1 line
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