Gustavo -
On Fri, 27 Apr 2012, Gustavo Padovan wrote:
Hi Mat,
* Mat Martineau <mathewm@xxxxxxxxxxxxxx> [2012-04-27 16:50:51 -0700]:
The L2CAP MTU for incoming data is verified differently depending on
the L2CAP mode, so the check is best performed in a mode-specific
context. Checking the incoming MTU before HCI fragment reassembly is
a layer violation and assumes all bytes after the standard L2CAP
header are L2CAP data.
This approadch causes issues with unsegmented ERTM or streaming mode
Oops, I need to fix this "approadch" typo.
frames, where there are additional enhanced or extended headers before
the data payload and possible FCS bytes after the data payload. A
valid frame could be as many as 10 bytes larger than the MTU.
Removing this code is the best fix, because the MTU is checked later
on for all L2CAP data frames (connectionless, basic, ERTM, and
streaming). This also gets rid of outdated locking (socket instead of
l2cap_chan) and an extra lookup of the channel ID.
I don't think we can remove this code from here. This check is different
from the ones you are talking, those are l2cap packets, here we are still
dealing with ACL fragments. This check is there to avoid accept a first
ACL packet that is too big. See 8979481328d.
One possible solution is to add a slack value to the check, so we can
fit those 10+ bytes packets in it.
If we just add slack, then we're depending on the real MTU checks to
work correctly later. Why bother with this early check at all?
I think there's a misunderstanding about the code that is being
removed. It *is* checking the L2CAP MTU for that channel against the
value in the L2CAP length header before the whole frame is assembled,
which is why it shouldn't be involved with ACL fragments to begin
with. It is the only place in l2cap_recv_acldata that uses
channel-specific information.
This code tries to throw out the first ACL fragment if the L2CAP
payload is longer than than the L2CAP MTU for that channel. The ERTM
and streaming mode MTU has different meaning - it applies to the
reassembled SDU payload, not the size of an individual PDU segment
with the extra headers and FCS bytes. There is already a length check
in the fragment reassembly code that makes sure no reassembled ACL
frame exceeds 65535 bytes (look at how conn->rx_len is used).
The original discussion around this code is here:
http://thread.gmane.org/gmane.linux.bluez.kernel/7505
The purpose was to address a memory allocation failure in
__get_free_pages - which suggests heap corruption. Even if the start
fragment is too large, that shouldn't lead to heap corruption,
especially if the fragment is properly freed. Throwing out this large
packet is just hiding the real bug!
The MTU is checked everywhere that it needs to be for L2CAP data,
after the ACL fragments are reassembled:
* In l2cap_reassemble_sdu for ERTM and Streaming Mode
* In l2cap_data_channel for Basic Mode
* In l2cap_connless_channel for connectionless channels
The code being removed doesn't address the signaling channel, because
that channel is not in the channel list. The spec defines the MTU for
the signaling channel to be "MTUsig", and only requires it to be at
least 48 bytes (672 bytes if extended flowspec is supported). It is
valid for us not to enforce a limit on it other than the maximum L2CAP
frame size.
We removed this code (because it broke AMP) from our Android kernel
back in September 2011, and have been through several qualifications
and rounds of testing since then without problems.
--
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
