Hi Marcel,
On Fri, Apr 23, 2021 at 5:26 AM Marcel Holtmann <marcel@xxxxxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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 = 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 = 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 = skb->data;
>
> if (skb->len < len) {
> bt_dev_err(hdev, “Malformed ..”);
> return NULL;
> }
>
> skb_pull(skb, len);
> return data;
> }
>
> static void *hci_cc_skb_pull(..)
> {
> void *data = 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’t see how that makes it simpler or cleaner. In the end you just have spaghetti code and don’t win anything in size reduction or complexity.
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[] = {
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_buff *skb)
> > {
> > - __u8 status = *((__u8 *) skb->data);
> > + struct hci_ev_status *ev;
> > struct discovery_state *discov = &hdev->discovery;
> > struct inquiry_entry *e;
> >
> > - BT_DBG("%s status 0x%2.2x", hdev->name, status);
> > + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_INQUIRY_COMPLETE, sizeof(*ev));
> > + if (!ev)
> > + return;
> > +
>
> Now since we also have this pattern:
>
> ev = 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) = skb->data; \
> if (skb->len < sizeof(*(var))) { \
> bt_dev_err(hdev, “Malformed ..”); \
> 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_dbg in the macro.
>
> >
> > hci_conn_check_pending(hdev);
> >
> > @@ -2604,9 +2632,13 @@ static void hci_inquiry_result_evt(struct hci_dev *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 = (void *) skb->data;
> > + struct hci_ev_conn_complete *ev;
> > struct hci_conn *conn;
> >
> > + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_CONN_COMPLETE, sizeof(*ev));
> > + 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
>
--
Luiz Augusto von Dentz
