Re: [PATCH 1/8] Bluetooth: Make sure the L2CAP FCS is only enabled for ERTM or streaming.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On Mon, 2 Aug 2010, Marcel Holtmann wrote:

Hi Mat,

* Marcel Holtmann <marcel@xxxxxxxxxxxx> [2010-08-02 12:38:32 -0700]:

Hi Mat,

Signed-off-by: Mat Martineau <mathewm@xxxxxxxxxxxxxx>
---
 net/bluetooth/l2cap.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 9ba1e8e..aed72f2 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -3127,8 +3127,10 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 		goto unlock;

 	if (l2cap_pi(sk)->conf_state & L2CAP_CONF_INPUT_DONE) {
-		if (!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
-		    l2cap_pi(sk)->fcs != L2CAP_FCS_NONE)
+		if ((l2cap_pi(sk)->mode == L2CAP_MODE_ERTM ||
+			l2cap_pi(sk)->mode == L2CAP_MODE_STREAMING) &&
+			(!(l2cap_pi(sk)->conf_state & L2CAP_CONF_NO_FCS_RECV) ||
+			l2cap_pi(sk)->fcs != L2CAP_FCS_NONE))
 			l2cap_pi(sk)->fcs = L2CAP_FCS_CRC16;

this becomes unreadable and my brain starts to throw a core dump. So it
clearly needs to be put into a helper inline function.

Actually we don't need that, since the code that deals with Basic Mode
never check  and use the l2cap_pi(sk)->fcs. So we don't care about FCS
value in the Basic Mode.

There isn't currently any Basic Mode code that triggers this latent
bug, but I have a patch coming up that does require this fix.

As it stands, getsockopt() on a connected basic mode socket shows FCS
enabled, so this bug is visible from userspace.

can we just fail the setsockopt() when trying to set basic mode and FCS
off.

It definitely makes sense to have more validation of L2CAP_OPTIONS passed to setsockopt().

And also in case fallback to basic mode happens, then FCS should be set
to be enabled. Since for FCS and basic mode we always have to use FCS.
So that seems just fine to me.

The spec says "The FCS option shall only be used when the mode is being, or is already configured to Enhanced Retransmission mode or Streaming mode." FCS is never used in basic mode (fallback or not).

(Maybe I've misunderstood your point)

Maybe you need to explain a bit more in detail what you are trying to
achieve in conjunction with userspace API.

My goal is to only have l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16 when the FCS option is actually in use. Otherwise, any logic checking for FCS also has to check the L2CAP mode. Might as well check the mode once and set fcs accordingly -- which is what my patch does.

Gustavo is correct that l2cap_pi(sk)->fcs is currently only checked on code paths used with ERTM and streaming mode. However, future code (including a patch I'll be posting soon) will depend on the fcs value being accurate in all modes.

I only mentioned getsockopt() to show that this issue is not completely invisible, and is worth patching.


Regards,

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

--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux