On Wed, 2018-03-21 at 10:41 +0200, Luiz Augusto von Dentz wrote:
> 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.
>
Ok, I got it. I will fix it with raw_dump().
> > > > 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.
>
Yes, I know it. I also check the same issue with btmon, and will give
it patch ( if there are the same problem ).
Thanks,
AL
> > > > --
> > > > 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.h
> > > > tml
> > >
> > >
> > >
>
>
>
--
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
