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: <20210419171257.3865181-1-luiz.dentz@gmail.com>
 <20210419171257.3865181-2-luiz.dentz@gmail.com> <25D86C94-89CE-44CC-BB9C-724486444C12@holtmann.org>
In-Reply-To: <25D86C94-89CE-44CC-BB9C-724486444C12@holtmann.org>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Mon, 26 Apr 2021 14:50:00 -0700
Message-ID: <CABBYNZJSRN2AbLqOVEWJ1oJivn0nR59zZad5AG1LCYpghd6CmA@mail.gmail.com>
Subject: Re: [PATCH v2 01/10] Bluetooth: HCI: Use skb_pull to parse BR/EDR events
To:     Marcel Holtmann <marcel@holtmann.org>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Marcel,

On Fri, Apr 23, 2021 at 5:26 AM Marcel Holtmann <marcel@holtmann.org> wrote=
:
>
> Hi Luiz,
>
> > This uses skb_pull to check the BR/EDR events received have the minimum
> > required length.
> >
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > ---
> > include/net/bluetooth/hci.h |   4 +
> > net/bluetooth/hci_event.c   | 272 +++++++++++++++++++++++++++++-------
> > 2 files changed, 229 insertions(+), 47 deletions(-)
> >
> > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > index ea4ae551c426..f1f505355e81 100644
> > --- a/include/net/bluetooth/hci.h
> > +++ b/include/net/bluetooth/hci.h
> > @@ -1894,6 +1894,10 @@ struct hci_cp_le_reject_cis {
> > } __packed;
> >
> > /* ---- HCI Events ---- */
> > +struct hci_ev_status {
> > +     __u8    status;
> > +} __packed;
> > +
> > #define HCI_EV_INQUIRY_COMPLETE               0x01
> >
> > #define HCI_EV_INQUIRY_RESULT         0x02
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index 5e99968939ce..077541fcba41 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -42,6 +42,30 @@
> >
> > /* Handle HCI Event packets */
> >
> > +static void *hci_skb_pull(struct sk_buff *skb, size_t len)
> > +{
> > +     void *data =3D skb->data;
> > +
> > +     if (skb->len < len)
> > +             return NULL;
> > +
> > +     skb_pull(skb, len);
> > +
> > +     return data;
> > +}
> > +
> > +static void *hci_ev_skb_pull(struct hci_dev *hdev, struct sk_buff *skb=
,
> > +                          u8 ev, size_t len)
> > +{
> > +     void *data;
> > +
> > +     data =3D hci_skb_pull(skb, len);
> > +     if (!data)
> > +             bt_dev_err(hdev, "Malformed Event: 0x%2.2x", ev);
> > +
> > +     return data;
> > +}
> > +
>
> so if you do it in one function, this will look like this:
>
>         static void *hci_ev_skb_pull(..)
>         {
>                 void *data =3D skb->data;
>
>                 if (skb->len < len) {
>                         bt_dev_err(hdev, =E2=80=9CMalformed ..=E2=80=9D);
>                         return NULL;
>                 }
>
>                 skb_pull(skb, len);
>                 return data;
>         }
>
>         static void *hci_cc_skb_pull(..)
>         {
>                 void *data =3D skb->data;
>
>                 if (skb->len < len) {
>                         bt_dev_err(..);
>                         return NULL;
>                 }
>
>                 skb_pull(..);
>                 return data;
>         }
>
> I realize that you want to optimize the code with hci_skb_pull, but I don=
=E2=80=99t see how that makes it simpler or cleaner. In the end you just ha=
ve spaghetti code and don=E2=80=99t win anything in size reduction or compl=
exity.

Right, I can either do that or perhaps bite the bullet and convert the
whole hci_event.c to use a function table:

#define HCI_EVENT(_op, _len, _func)...

struct hci_event {
  __u8    op;
  __u16  len;
  func...
} ev_table[] =3D {
  HCI_EVENT(...),
}

But that would pack a lot more changes since we would need to convert
the whole switch to a for loop or alternatively if we don't want do a
lookup we could omit the op and access the table by index if we want
to trade speed over code size, with that we can have just one call to
the likes of hci_ev_skb_pull.

>
>
> > static void hci_cc_inquiry_cancel(struct hci_dev *hdev, struct sk_buff =
*skb,
> >                                 u8 *new_status)
> > {
> > @@ -2507,11 +2531,15 @@ static void hci_cs_switch_role(struct hci_dev *=
hdev, u8 status)
> >
> > static void hci_inquiry_complete_evt(struct hci_dev *hdev, struct sk_bu=
ff *skb)
> > {
> > -     __u8 status =3D *((__u8 *) skb->data);
> > +     struct hci_ev_status *ev;
> >       struct discovery_state *discov =3D &hdev->discovery;
> >       struct inquiry_entry *e;
> >
> > -     BT_DBG("%s status 0x%2.2x", hdev->name, status);
> > +     ev =3D hci_ev_skb_pull(hdev, skb, HCI_EV_INQUIRY_COMPLETE, sizeof=
(*ev));
> > +     if (!ev)
> > +             return;
> > +
>
> Now since we also have this pattern:
>
>         ev =3D hci_ev_skb_pull(..);
>         if (!ev)
>                 return;
>
> The question is now why not just use a #define here.
>
>         hci_ev_skb_pull(HCI_EV_INQUIRY_COMPLETE, ev);
>
> And then the define would look like this (untested):
>
> #define hci_ev_skb_pull(evt, var) do {                    \
>                 (var) =3D skb->data;                        \
>                 if (skb->len < sizeof(*(var))) {          \
>                         bt_dev_err(hdev, =E2=80=9CMalformed ..=E2=80=9D);=
 \
>                         return NULL;                      \
>                 }                                         \
>                 skb_pull(skb, sizeof(*(var)));            \
>         } while (0);
>
>
> > +     BT_DBG("%s status 0x%2.2x", hdev->name, ev->status);
>
> If we have every event with an ev->status, we could even include bt_dev_d=
bg in the macro.
>
> >
> >       hci_conn_check_pending(hdev);
> >
> > @@ -2604,9 +2632,13 @@ static void hci_inquiry_result_evt(struct hci_de=
v *hdev, struct sk_buff *skb)
> >
> > static void hci_conn_complete_evt(struct hci_dev *hdev, struct sk_buff =
*skb)
> > {
> > -     struct hci_ev_conn_complete *ev =3D (void *) skb->data;
> > +     struct hci_ev_conn_complete *ev;
> >       struct hci_conn *conn;
> >
> > +     ev =3D hci_ev_skb_pull(hdev, skb, HCI_EV_CONN_COMPLETE, sizeof(*e=
v));
> > +     if (!ev)
> > +             return;
> > +
> >       BT_DBG("%s", hdev->name);
>
> If you are touching the above part anyway, then move towards bt_dev_dbg()=
 at the same time.
>
> Regards
>
> Marcel
>


--=20
Luiz Augusto von Dentz