| Message ID | 20210419171257.3865181-2-luiz.dentz@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Bluetooth: HCI: Use skb_pull to parse events | expand |
This is automated email and please do not reply to this email! Dear submitter, Thank you for submitting the patches to the linux bluetooth mailing list. This is a CI test results with your patch series: PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=469803 ---Test result--- ############################## Test: CheckPatch - PASS ############################## Test: CheckGitLint - FAIL Bluetooth: HCI: Use skb_pull to parse LE Extended Advertising Report event 1: T1 Title exceeds max length (74>72): "Bluetooth: HCI: Use skb_pull to parse LE Extended Advertising Report event" ############################## Test: CheckBuildK - PASS ############################## Test: CheckTestRunner: Setup - PASS ############################## Test: CheckTestRunner: l2cap-tester - PASS Total: 40, Passed: 34 (85.0%), Failed: 0, Not Run: 6 ############################## Test: CheckTestRunner: bnep-tester - PASS Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0 ############################## Test: CheckTestRunner: mgmt-tester - PASS Total: 416, Passed: 402 (96.6%), Failed: 0, Not Run: 14 ############################## Test: CheckTestRunner: rfcomm-tester - PASS Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0 ############################## Test: CheckTestRunner: sco-tester - PASS Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0 ############################## Test: CheckTestRunner: smp-tester - PASS Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0 ############################## Test: CheckTestRunner: userchan-tester - PASS Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0 --- Regards, Linux Bluetooth
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 = 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. > 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
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 = 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 >
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 = 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. looping through a table is not ideal. There are too many events that you can receive and if we end up looping for every LE advertising report it would be rather silly. And of course a static table is possible since event numbers are assigned in order, but it also means some sort of overhead since we don’t parse each event. In addition to that dilemma, you have the problem that not all events are fixed size. So you end up indicating that similar to how we do it in btmon which will increase the table size again. Maybe that is actually ok since two giant switch statements also take time and code size. So our currently max event code is 0x57 for Authenticated Payload Timeout and the max LE event code is 0x3e for BIG Info Advertising Report. Meaning table one would have 87 entries and table would have two 63 entries. If we do min_len,max_len as uint8 then we have 2 octets and a function pointer. Maybe that is not too bad since that is all static const data. Regards Marcel
Hi Marcel, On Mon, Apr 26, 2021 at 11:07 PM 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 = 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. > > looping through a table is not ideal. There are too many events that you can receive and if we end up looping for every LE advertising report it would be rather silly. And of course a static table is possible since event numbers are assigned in order, but it also means some sort of overhead since we don’t parse each event. > > In addition to that dilemma, you have the problem that not all events are fixed size. So you end up indicating that similar to how we do it in btmon which will increase the table size again. Maybe that is actually ok since two giant switch statements also take time and code size. > > So our currently max event code is 0x57 for Authenticated Payload Timeout and the max LE event code is 0x3e for BIG Info Advertising Report. Meaning table one would have 87 entries and table would have two 63 entries. If we do min_len,max_len as uint8 then we have 2 octets and a function pointer. Maybe that is not too bad since that is all static const data. Yep, even if we have to have NOP for those event we don't handle I don't think it is such a big deal and accessing by index should be comparable in terms of speed to a switch statement, that said we may still need to do some checks on the callbacks for those events that have variable size I was only intending to do minimal size checks but perhaps you are saying it might be better to have a bool/flag saying that the event has variable size which matters when we are checking the length to either use == or <. > Regards > > Marcel >
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 = 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. >> >> looping through a table is not ideal. There are too many events that you can receive and if we end up looping for every LE advertising report it would be rather silly. And of course a static table is possible since event numbers are assigned in order, but it also means some sort of overhead since we don’t parse each event. >> >> In addition to that dilemma, you have the problem that not all events are fixed size. So you end up indicating that similar to how we do it in btmon which will increase the table size again. Maybe that is actually ok since two giant switch statements also take time and code size. >> >> So our currently max event code is 0x57 for Authenticated Payload Timeout and the max LE event code is 0x3e for BIG Info Advertising Report. Meaning table one would have 87 entries and table would have two 63 entries. If we do min_len,max_len as uint8 then we have 2 octets and a function pointer. Maybe that is not too bad since that is all static const data. > > Yep, even if we have to have NOP for those event we don't handle I > don't think it is such a big deal and accessing by index should be > comparable in terms of speed to a switch statement, that said we may > still need to do some checks on the callbacks for those events that > have variable size I was only intending to do minimal size checks but > perhaps you are saying it might be better to have a bool/flag saying > that the event has variable size which matters when we are checking > the length to either use == or <. I actually meant min_len + max_len. So you have for example HCI_EVENT(..) that sets min_len = x, max_len = x. HCI_EVENT_VAR(..) that sets min_len = x and max_len = 253. No need for extra flags then. Regards Marcel
Hi Marcel, On Tue, Apr 27, 2021 at 11:37 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 = 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. > >> > >> looping through a table is not ideal. There are too many events that you can receive and if we end up looping for every LE advertising report it would be rather silly. And of course a static table is possible since event numbers are assigned in order, but it also means some sort of overhead since we don’t parse each event. > >> > >> In addition to that dilemma, you have the problem that not all events are fixed size. So you end up indicating that similar to how we do it in btmon which will increase the table size again. Maybe that is actually ok since two giant switch statements also take time and code size. > >> > >> So our currently max event code is 0x57 for Authenticated Payload Timeout and the max LE event code is 0x3e for BIG Info Advertising Report. Meaning table one would have 87 entries and table would have two 63 entries. If we do min_len,max_len as uint8 then we have 2 octets and a function pointer. Maybe that is not too bad since that is all static const data. > > > > Yep, even if we have to have NOP for those event we don't handle I > > don't think it is such a big deal and accessing by index should be > > comparable in terms of speed to a switch statement, that said we may > > still need to do some checks on the callbacks for those events that > > have variable size I was only intending to do minimal size checks but > > perhaps you are saying it might be better to have a bool/flag saying > > that the event has variable size which matters when we are checking > > the length to either use == or <. > > I actually meant min_len + max_len. > > So you have for example > > HCI_EVENT(..) that sets min_len = x, max_len = x. > HCI_EVENT_VAR(..) that sets min_len = x and max_len = 253. > > No need for extra flags then. Ah got it, is there really a max_len for variable size though? Or you mean to say that should be limited to event buffer size? That Im afraid we only discover at runtime, anyway usually for variable size we should be using flex_array_size but that requires accessing the received skb so we can't really tell at build time and it will be likely left for the callback to figure out it has received enough data or not. > > Regards > > Marcel >
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 = 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. >>>> >>>> looping through a table is not ideal. There are too many events that you can receive and if we end up looping for every LE advertising report it would be rather silly. And of course a static table is possible since event numbers are assigned in order, but it also means some sort of overhead since we don’t parse each event. >>>> >>>> In addition to that dilemma, you have the problem that not all events are fixed size. So you end up indicating that similar to how we do it in btmon which will increase the table size again. Maybe that is actually ok since two giant switch statements also take time and code size. >>>> >>>> So our currently max event code is 0x57 for Authenticated Payload Timeout and the max LE event code is 0x3e for BIG Info Advertising Report. Meaning table one would have 87 entries and table would have two 63 entries. If we do min_len,max_len as uint8 then we have 2 octets and a function pointer. Maybe that is not too bad since that is all static const data. >>> >>> Yep, even if we have to have NOP for those event we don't handle I >>> don't think it is such a big deal and accessing by index should be >>> comparable in terms of speed to a switch statement, that said we may >>> still need to do some checks on the callbacks for those events that >>> have variable size I was only intending to do minimal size checks but >>> perhaps you are saying it might be better to have a bool/flag saying >>> that the event has variable size which matters when we are checking >>> the length to either use == or <. >> >> I actually meant min_len + max_len. >> >> So you have for example >> >> HCI_EVENT(..) that sets min_len = x, max_len = x. >> HCI_EVENT_VAR(..) that sets min_len = x and max_len = 253. >> >> No need for extra flags then. > > Ah got it, is there really a max_len for variable size though? Or you > mean to say that should be limited to event buffer size? That Im > afraid we only discover at runtime, anyway usually for variable size > we should be using flex_array_size but that requires accessing the > received skb so we can't really tell at build time and it will be > likely left for the callback to figure out it has received enough data > or not. using max_len will make it flexible enough since in HCI event the max len is 253 as far as I recall. Or it was 255, but I don’t remember from the top of my head. It also really doesn’t matter since if you need to change something, you just change the #define HCI_EVENT_VAR and that is it. I prefer to not make it more complicated than needed. The flex_array_size is nice, but I don’t know how much use that is in a static const structure array. Regards Marcel
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; +} + 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; + + BT_DBG("%s status 0x%2.2x", hdev->name, ev->status); 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); hci_dev_lock(hdev); @@ -2728,12 +2760,16 @@ static void hci_reject_conn(struct hci_dev *hdev, bdaddr_t *bdaddr) static void hci_conn_request_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_conn_request *ev = (void *) skb->data; + struct hci_ev_conn_request *ev; int mask = hdev->link_mode; struct inquiry_entry *ie; struct hci_conn *conn; __u8 flags = 0; + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_CONN_REQUEST, sizeof(*ev)); + if (!ev) + return; + BT_DBG("%s bdaddr %pMR type 0x%x", hdev->name, &ev->bdaddr, ev->link_type); @@ -2839,13 +2875,17 @@ static u8 hci_to_mgmt_reason(u8 err) static void hci_disconn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_disconn_complete *ev = (void *) skb->data; + struct hci_ev_disconn_complete *ev; u8 reason; struct hci_conn_params *params; struct hci_conn *conn; bool mgmt_connected; u8 type; + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_DISCONN_COMPLETE, sizeof(*ev)); + if (!ev) + return; + BT_DBG("%s status 0x%2.2x", hdev->name, ev->status); hci_dev_lock(hdev); @@ -2931,9 +2971,13 @@ static void hci_disconn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb) static void hci_auth_complete_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_auth_complete *ev = (void *) skb->data; + struct hci_ev_auth_complete *ev; struct hci_conn *conn; + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_AUTH_COMPLETE, sizeof(*ev)); + if (!ev) + return; + BT_DBG("%s status 0x%2.2x", hdev->name, ev->status); hci_dev_lock(hdev); @@ -3001,9 +3045,13 @@ static void hci_auth_complete_evt(struct hci_dev *hdev, struct sk_buff *skb) static void hci_remote_name_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_remote_name *ev = (void *) skb->data; + struct hci_ev_remote_name *ev; struct hci_conn *conn; + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_REMOTE_NAME, sizeof(*ev)); + if (!ev) + return; + BT_DBG("%s", hdev->name); hci_conn_check_pending(hdev); @@ -3084,9 +3132,13 @@ static void read_enc_key_size_complete(struct hci_dev *hdev, u8 status, static void hci_encrypt_change_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_encrypt_change *ev = (void *) skb->data; + struct hci_ev_encrypt_change *ev; struct hci_conn *conn; + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_ENCRYPT_CHANGE, sizeof(*ev)); + if (!ev) + return; + BT_DBG("%s status 0x%2.2x", hdev->name, ev->status); hci_dev_lock(hdev); @@ -3199,9 +3251,14 @@ static void hci_encrypt_change_evt(struct hci_dev *hdev, struct sk_buff *skb) static void hci_change_link_key_complete_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_change_link_key_complete *ev = (void *) skb->data; + struct hci_ev_change_link_key_complete *ev; struct hci_conn *conn; + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_CHANGE_LINK_KEY_COMPLETE, + sizeof(*ev)); + if (!ev) + return; + BT_DBG("%s status 0x%2.2x", hdev->name, ev->status); hci_dev_lock(hdev); @@ -3222,9 +3279,13 @@ static void hci_change_link_key_complete_evt(struct hci_dev *hdev, static void hci_remote_features_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_remote_features *ev = (void *) skb->data; + struct hci_ev_remote_features *ev; struct hci_conn *conn; + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_REMOTE_FEATURES, sizeof(*ev)); + if (!ev) + return; + BT_DBG("%s status 0x%2.2x", hdev->name, ev->status); hci_dev_lock(hdev); @@ -3654,9 +3715,11 @@ static void hci_cmd_status_evt(struct hci_dev *hdev, struct sk_buff *skb, hci_req_complete_t *req_complete, hci_req_complete_skb_t *req_complete_skb) { - struct hci_ev_cmd_status *ev = (void *) skb->data; + struct hci_ev_cmd_status *ev; - skb_pull(skb, sizeof(*ev)); + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_CMD_STATUS, sizeof(*ev)); + if (!ev) + return; *opcode = __le16_to_cpu(ev->opcode); *status = ev->status; @@ -3764,7 +3827,11 @@ static void hci_cmd_status_evt(struct hci_dev *hdev, struct sk_buff *skb, static void hci_hardware_error_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_hardware_error *ev = (void *) skb->data; + struct hci_ev_hardware_error *ev; + + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_HARDWARE_ERROR, sizeof(*ev)); + if (!ev) + return; hdev->hw_error_code = ev->code; @@ -3773,9 +3840,13 @@ static void hci_hardware_error_evt(struct hci_dev *hdev, struct sk_buff *skb) static void hci_role_change_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_role_change *ev = (void *) skb->data; + struct hci_ev_role_change *ev; struct hci_conn *conn; + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_ROLE_CHANGE, sizeof(*ev)); + if (!ev) + return; + BT_DBG("%s status 0x%2.2x", hdev->name, ev->status); hci_dev_lock(hdev); @@ -3883,17 +3954,19 @@ static struct hci_conn *__hci_conn_lookup_handle(struct hci_dev *hdev, static void hci_num_comp_blocks_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_num_comp_blocks *ev = (void *) skb->data; + struct hci_ev_num_comp_blocks *ev; int i; - if (hdev->flow_ctl_mode != HCI_FLOW_CTL_MODE_BLOCK_BASED) { - bt_dev_err(hdev, "wrong event for mode %d", hdev->flow_ctl_mode); + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_NUM_COMP_BLOCKS, sizeof(*ev)); + if (!ev) return; - } - if (skb->len < sizeof(*ev) || - skb->len < struct_size(ev, handles, ev->num_hndl)) { - BT_DBG("%s bad parameters", hdev->name); + if (!hci_ev_skb_pull(hdev, skb, HCI_EV_NUM_COMP_BLOCKS, + flex_array_size(ev, handles, ev->num_hndl))) + return; + + if (hdev->flow_ctl_mode != HCI_FLOW_CTL_MODE_BLOCK_BASED) { + bt_dev_err(hdev, "wrong event for mode %d", hdev->flow_ctl_mode); return; } @@ -3934,9 +4007,13 @@ static void hci_num_comp_blocks_evt(struct hci_dev *hdev, struct sk_buff *skb) static void hci_mode_change_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_mode_change *ev = (void *) skb->data; + struct hci_ev_mode_change *ev; struct hci_conn *conn; + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_MODE_CHANGE, sizeof(*ev)); + if (!ev) + return; + BT_DBG("%s status 0x%2.2x", hdev->name, ev->status); hci_dev_lock(hdev); @@ -3962,9 +4039,13 @@ static void hci_mode_change_evt(struct hci_dev *hdev, struct sk_buff *skb) static void hci_pin_code_request_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_pin_code_req *ev = (void *) skb->data; + struct hci_ev_pin_code_req *ev; struct hci_conn *conn; + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_PIN_CODE_REQ, sizeof(*ev)); + if (!ev) + return; + BT_DBG("%s", hdev->name); hci_dev_lock(hdev); @@ -4032,11 +4113,15 @@ static void conn_set_key(struct hci_conn *conn, u8 key_type, u8 pin_len) static void hci_link_key_request_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_link_key_req *ev = (void *) skb->data; + struct hci_ev_link_key_req *ev; struct hci_cp_link_key_reply cp; struct hci_conn *conn; struct link_key *key; + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_LINK_KEY_REQ, sizeof(*ev)); + if (!ev) + return; + BT_DBG("%s", hdev->name); if (!hci_dev_test_flag(hdev, HCI_MGMT)) @@ -4092,12 +4177,16 @@ static void hci_link_key_request_evt(struct hci_dev *hdev, struct sk_buff *skb) static void hci_link_key_notify_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_link_key_notify *ev = (void *) skb->data; + struct hci_ev_link_key_notify *ev; struct hci_conn *conn; struct link_key *key; bool persistent; u8 pin_len = 0; + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_LINK_KEY_NOTIFY, sizeof(*ev)); + if (!ev) + return; + BT_DBG("%s", hdev->name); hci_dev_lock(hdev); @@ -4152,9 +4241,13 @@ static void hci_link_key_notify_evt(struct hci_dev *hdev, struct sk_buff *skb) static void hci_clock_offset_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_clock_offset *ev = (void *) skb->data; + struct hci_ev_clock_offset *ev; struct hci_conn *conn; + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_CLOCK_OFFSET, sizeof(*ev)); + if (!ev) + return; + BT_DBG("%s status 0x%2.2x", hdev->name, ev->status); hci_dev_lock(hdev); @@ -4175,9 +4268,13 @@ static void hci_clock_offset_evt(struct hci_dev *hdev, struct sk_buff *skb) static void hci_pkt_type_change_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_pkt_type_change *ev = (void *) skb->data; + struct hci_ev_pkt_type_change *ev; struct hci_conn *conn; + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_PKT_TYPE_CHANGE, sizeof(*ev)); + if (!ev) + return; + BT_DBG("%s status 0x%2.2x", hdev->name, ev->status); hci_dev_lock(hdev); @@ -4191,9 +4288,13 @@ static void hci_pkt_type_change_evt(struct hci_dev *hdev, struct sk_buff *skb) static void hci_pscan_rep_mode_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_pscan_rep_mode *ev = (void *) skb->data; + struct hci_ev_pscan_rep_mode *ev; struct inquiry_entry *ie; + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_PSCAN_REP_MODE, sizeof(*ev)); + if (!ev) + return; + BT_DBG("%s", hdev->name); hci_dev_lock(hdev); @@ -4281,9 +4382,14 @@ static void hci_inquiry_result_with_rssi_evt(struct hci_dev *hdev, static void hci_remote_ext_features_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_remote_ext_features *ev = (void *) skb->data; + struct hci_ev_remote_ext_features *ev; struct hci_conn *conn; + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_REMOTE_EXT_FEATURES, + sizeof(*ev)); + if (!ev) + return; + BT_DBG("%s", hdev->name); hci_dev_lock(hdev); @@ -4345,9 +4451,13 @@ static void hci_remote_ext_features_evt(struct hci_dev *hdev, static void hci_sync_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_sync_conn_complete *ev = (void *) skb->data; + struct hci_ev_sync_conn_complete *ev; struct hci_conn *conn; + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_SYNC_CONN_COMPLETE, sizeof(*ev)); + if (!ev) + return; + BT_DBG("%s status 0x%2.2x", hdev->name, ev->status); hci_dev_lock(hdev); @@ -4493,9 +4603,14 @@ static void hci_extended_inquiry_result_evt(struct hci_dev *hdev, static void hci_key_refresh_complete_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_key_refresh_complete *ev = (void *) skb->data; + struct hci_ev_key_refresh_complete *ev; struct hci_conn *conn; + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_KEY_REFRESH_COMPLETE, + sizeof(*ev)); + if (!ev) + return; + BT_DBG("%s status 0x%2.2x handle 0x%4.4x", hdev->name, ev->status, __le16_to_cpu(ev->handle)); @@ -4602,9 +4717,13 @@ static u8 bredr_oob_data_present(struct hci_conn *conn) static void hci_io_capa_request_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_io_capa_request *ev = (void *) skb->data; + struct hci_ev_io_capa_request *ev; struct hci_conn *conn; + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_IO_CAPA_REQUEST, sizeof(*ev)); + if (!ev) + return; + BT_DBG("%s", hdev->name); hci_dev_lock(hdev); @@ -4671,9 +4790,13 @@ static void hci_io_capa_request_evt(struct hci_dev *hdev, struct sk_buff *skb) static void hci_io_capa_reply_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_io_capa_reply *ev = (void *) skb->data; + struct hci_ev_io_capa_reply *ev; struct hci_conn *conn; + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_IO_CAPA_REPLY, sizeof(*ev)); + if (!ev) + return; + BT_DBG("%s", hdev->name); hci_dev_lock(hdev); @@ -4692,10 +4815,15 @@ static void hci_io_capa_reply_evt(struct hci_dev *hdev, struct sk_buff *skb) static void hci_user_confirm_request_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_user_confirm_req *ev = (void *) skb->data; + struct hci_ev_user_confirm_req *ev; int loc_mitm, rem_mitm, confirm_hint = 0; struct hci_conn *conn; + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_USER_CONFIRM_REQUEST, + sizeof(*ev)); + if (!ev) + return; + BT_DBG("%s", hdev->name); hci_dev_lock(hdev); @@ -4777,7 +4905,12 @@ static void hci_user_confirm_request_evt(struct hci_dev *hdev, static void hci_user_passkey_request_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_user_passkey_req *ev = (void *) skb->data; + struct hci_ev_user_passkey_req *ev; + + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_USER_PASSKEY_REQUEST, + sizeof(*ev)); + if (!ev) + return; BT_DBG("%s", hdev->name); @@ -4788,9 +4921,14 @@ static void hci_user_passkey_request_evt(struct hci_dev *hdev, static void hci_user_passkey_notify_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_user_passkey_notify *ev = (void *) skb->data; + struct hci_ev_user_passkey_notify *ev; struct hci_conn *conn; + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_USER_PASSKEY_NOTIFY, + sizeof(*ev)); + if (!ev) + return; + BT_DBG("%s", hdev->name); conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &ev->bdaddr); @@ -4808,9 +4946,13 @@ static void hci_user_passkey_notify_evt(struct hci_dev *hdev, static void hci_keypress_notify_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_keypress_notify *ev = (void *) skb->data; + struct hci_ev_keypress_notify *ev; struct hci_conn *conn; + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_KEYPRESS_NOTIFY, sizeof(*ev)); + if (!ev) + return; + BT_DBG("%s", hdev->name); conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &ev->bdaddr); @@ -4847,9 +4989,14 @@ static void hci_keypress_notify_evt(struct hci_dev *hdev, struct sk_buff *skb) static void hci_simple_pair_complete_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_simple_pair_complete *ev = (void *) skb->data; + struct hci_ev_simple_pair_complete *ev; struct hci_conn *conn; + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_SIMPLE_PAIR_COMPLETE, + sizeof(*ev)); + if (!ev) + return; + BT_DBG("%s", hdev->name); hci_dev_lock(hdev); @@ -4878,10 +5025,15 @@ static void hci_simple_pair_complete_evt(struct hci_dev *hdev, static void hci_remote_host_features_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_remote_host_features *ev = (void *) skb->data; + struct hci_ev_remote_host_features *ev; struct inquiry_entry *ie; struct hci_conn *conn; + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_REMOTE_HOST_FEATURES, + sizeof(*ev)); + if (!ev) + return; + BT_DBG("%s", hdev->name); hci_dev_lock(hdev); @@ -4900,9 +5052,14 @@ static void hci_remote_host_features_evt(struct hci_dev *hdev, static void hci_remote_oob_data_request_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_remote_oob_data_request *ev = (void *) skb->data; + struct hci_ev_remote_oob_data_request *ev; struct oob_data *data; + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_REMOTE_OOB_DATA_REQUEST, + sizeof(*ev)); + if (!ev) + return; + BT_DBG("%s", hdev->name); hci_dev_lock(hdev); @@ -4954,12 +5111,14 @@ static void hci_remote_oob_data_request_evt(struct hci_dev *hdev, #if IS_ENABLED(CONFIG_BT_HS) static void hci_chan_selected_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_channel_selected *ev = (void *)skb->data; + struct hci_ev_channel_selected *ev; struct hci_conn *hcon; - BT_DBG("%s handle 0x%2.2x", hdev->name, ev->phy_handle); + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_CHANNEL_SELECTED, sizeof(*ev)); + if (!ev) + return; - skb_pull(skb, sizeof(*ev)); + BT_DBG("%s handle 0x%2.2x", hdev->name, ev->phy_handle); hcon = hci_conn_hash_lookup_handle(hdev, ev->phy_handle); if (!hcon) @@ -4971,9 +5130,13 @@ static void hci_chan_selected_evt(struct hci_dev *hdev, struct sk_buff *skb) static void hci_phy_link_complete_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_phy_link_complete *ev = (void *) skb->data; + struct hci_ev_phy_link_complete *ev; struct hci_conn *hcon, *bredr_hcon; + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_PHY_LINK_COMPLETE, sizeof(*ev)); + if (!ev) + return; + BT_DBG("%s handle 0x%2.2x status 0x%2.2x", hdev->name, ev->phy_handle, ev->status); @@ -5011,11 +5174,16 @@ static void hci_phy_link_complete_evt(struct hci_dev *hdev, static void hci_loglink_complete_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_logical_link_complete *ev = (void *) skb->data; + struct hci_ev_logical_link_complete *ev; struct hci_conn *hcon; struct hci_chan *hchan; struct amp_mgr *mgr; + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_LOGICAL_LINK_COMPLETE, + sizeof(*ev)); + if (!ev) + return; + BT_DBG("%s log_handle 0x%4.4x phy_handle 0x%2.2x status 0x%2.2x", hdev->name, le16_to_cpu(ev->handle), ev->phy_handle, ev->status); @@ -5051,9 +5219,14 @@ static void hci_loglink_complete_evt(struct hci_dev *hdev, struct sk_buff *skb) static void hci_disconn_loglink_complete_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_disconn_logical_link_complete *ev = (void *) skb->data; + struct hci_ev_disconn_logical_link_complete *ev; struct hci_chan *hchan; + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_DISCONN_LOGICAL_LINK_COMPLETE, + sizeof(*ev)); + if (!ev) + return; + BT_DBG("%s log handle 0x%4.4x status 0x%2.2x", hdev->name, le16_to_cpu(ev->handle), ev->status); @@ -5075,9 +5248,14 @@ static void hci_disconn_loglink_complete_evt(struct hci_dev *hdev, static void hci_disconn_phylink_complete_evt(struct hci_dev *hdev, struct sk_buff *skb) { - struct hci_ev_disconn_phy_link_complete *ev = (void *) skb->data; + struct hci_ev_disconn_phy_link_complete *ev; struct hci_conn *hcon; + ev = hci_ev_skb_pull(hdev, skb, HCI_EV_DISCONN_PHY_LINK_COMPLETE, + sizeof(*ev)); + if (!ev) + return; + BT_DBG("%s status 0x%2.2x", hdev->name, ev->status); if (ev->status)