Hi,
On Wed, Mar 21, 2018 at 8:58 AM, AL Yu-Chen Cho <acho@xxxxxxxx> wrote:
> Hi,
>
> On Tue, 2018-03-20 at 11:11 +0200, Luiz Augusto von Dentz wrote:
>> Hi,
>>
>> On Tue, Mar 20, 2018 at 9:07 AM, Cho, Yu-Chen <acho@xxxxxxxx> wrote:
>> > Fix memory leak with malformed packet.
>> > ---
>> > tools/parser/l2cap.c | 6 ++++++
>> > 1 file changed, 6 insertions(+)
>> >
>> > diff --git a/tools/parser/l2cap.c b/tools/parser/l2cap.c
>> > index a05796482..8b550f270 100644
>> > --- a/tools/parser/l2cap.c
>> > +++ b/tools/parser/l2cap.c
>> > @@ -1557,6 +1557,12 @@ void l2cap_dump(int level, struct frame
>> > *frm)
>> > hdr = frm->ptr;
>> > dlen = btohs(hdr->len);
>> >
>> > + if ((dlen + L2CAP_HDR_SIZE) > (int) frm->len) {
>> > + /* malformed frame */
>> > + perror("Read Error");
>> > + return;
>> > + }
>> > +
>>
>> So how about we fix this on the next statement which is about the
>> same
>> check but passes the invalid frame to raw_dump.
>>
>
> I don't think it's a good idea.
> From the Bluetooth 5.0 spec
> HCI ACL Data Packets:
> Hosts and Controllers shall be able to accept HCI ACL Data Packets with
> up to 27 bytes of data excluding the HCI ACL Data Packet header on
> Connection_Handles associated with an LE-U logical link.The HCI ACL
> Data Packet header is the first 4 octets of the packet.
> and Host Buffer Size Command:
> The Host_Buffer_Size command is used by the Host to notify the
> Controller about the maximum size of the data portion of HCI ACL and
> synchronous Data Packets sent from the Controller to the Host.
> So we should not allow to read more than the buffer size.
> If (dlen+L2CAP_HDR_SIZE) is large then frm->len, that would be a
> malformed frame.
And hcidump being a tool used for debugging such problem shall be able
to print out an error along with the contents of the malformed data so
it may give a clue to what is the malformed frame. Perhaps you are
saying that we need to adjust the frm->len since that is invalid and
would lead to raw_dump to crash, that is a fair point but my comment
was about leaving the check below which is basically a dead code.
>> > if (dlen + L2CAP_HDR_SIZE < (int) frm->len) {
>> > /* invalid frame */
>> > raw_dump(level,frm);
So Id print an error before the raw_dump saying there is a length
mismatch and proceed adjust the frm->len so it can actually be printed
with raw_dump.
Also please consider using btmon over hcidump, hcidump is a deprecated
tool that we might remove at some point.
>> > --
>> > 2.16.2
>> >
>> > --
>> > 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
>>
>>
>>
--
Luiz Augusto von Dentz
--
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
