Re: [PATCH v2 01/10] Bluetooth: HCI: Use skb_pull to parse BR/EDR events
From: Marcel Holtmann <marcel@holtmann.org>
Date: 2021-04-23 12:26:25
Hi Luiz,
quoted hunk ↗ jump to hunk
This uses skb_pull to check the BR/EDR events received have the minimum required length. Signed-off-by: Luiz Augusto von Dentz <redacted> --- 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 0x02diff --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.
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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