Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210511152807.Bluez.v1.1.I6d2ab6907d9a84fa62ac8a39daef5bef7ff545d5@changeid>
 <CABBYNZ+JiqjRR0p6LSFr8MZ6i2pojDDjmh5X5n8MCRih06WEUQ@mail.gmail.com>
In-Reply-To: <CABBYNZ+JiqjRR0p6LSFr8MZ6i2pojDDjmh5X5n8MCRih06WEUQ@mail.gmail.com>
From:   Yun-hao Chung <howardchung@google.com>
Date:   Wed, 12 May 2021 10:22:54 +0800
Message-ID: <CAPHZWUdfWV3PnTWzYVeSbSR6qZesyn7PJuEMNFUsg+Qv07Ngpw@mail.gmail.com>
Subject: Re: [Bluez PATCH v1] monitor: Fix possible crash of rfcomm packet
To:     Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        ChromeOS Bluetooth Upstreaming 
        <chromeos-bluetooth-upstreaming@chromium.org>,
        Yun-Hao Chung <howardchung@chromium.org>,
        Archie Pusaka <apusaka@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, May 12, 2021 at 2:04 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Howard,
>
> On Tue, May 11, 2021 at 12:28 AM Howard Chung <howardchung@google.com> wrote:
> >
> > From: Yun-Hao Chung <howardchung@chromium.org>
> >
> > When RFCOMM_TEST_EA returns false, btmon assumes packet data has at
> > least 5 bytes long. If that assumption fails, btmon could crash when
> > trying to read the next byte.
> > This patch fix it by checking the remaining size before reading the last
> > byte.
> >
> > Reviewed-by: apusaka@chromium.org
> > ---
> >
> >  monitor/rfcomm.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/monitor/rfcomm.c b/monitor/rfcomm.c
> > index 9b88a3440e31..76b1123bb23d 100644
> > --- a/monitor/rfcomm.c
> > +++ b/monitor/rfcomm.c
> > @@ -452,6 +452,9 @@ void rfcomm_packet(const struct l2cap_frame *frame)
> >                 hdr.length = GET_LEN16(hdr.length);
> >         }
> >
> > +       if (l2cap_frame->size == 0)
> > +               goto fail;
> > +
>
> if (!l2cap_frame->size)

will do.
>
>
>
> >         l2cap_frame_pull(&tmp_frame, l2cap_frame, l2cap_frame->size-1);
>
> Or perhaps we can make l2cap_frame_pull check if it can really pull
> the frame and return false if it doesn't just as get_*.

IMO, this might not be the best solution. Since |len|l in
2cap_frame_pull is uint16_t, when l2cap_frame->size-1 overflows it
might confuse people.

>
>
>
> >         if (!l2cap_frame_get_u8(&tmp_frame, &hdr.fcs))
> > --
> > 2.31.1.607.g51e8a6a459-goog
> >
>
>
> --
> Luiz Augusto von Dentz