[v6,2/5] Bluetooth: aosp: surface AOSP quality report through mgmt

Commit Message

Joseph Hwang May 26, 2022, 11:21 a.m. UTC

When receiving a HCI vendor event, the kernel checks if it is an
AOSP bluetooth quality report. If yes, the event is sent to bluez
user space through the mgmt socket.

Reported-by: kernel test robot <[e-mail]>

Signed-off-by: Joseph Hwang <[e-mail]>
Reviewed-by: Archie Pusaka <[e-mail]>
---

Changes in v6:
- Fixed a sparse check warning about using static for evt_prefixes.

Changes in v5:
- Define "struct ext_vendor_prefix" to replace "struct vendor_prefix"
  so that extended vendor prefix = prefix + 1-octet subcode
- Define aosp_ext_prefix to provide AOSP extended prefix which is
  returned by aosp_get_ext_prefix().
- Redefine struct ext_vendor_event_prefix such that
  . it uses get_ext_vendor_prefix to get prefix and subcodes where
    the prefix and the prefix length may be variable and are not
    unknown until run time;
  . it uses vendor_func to handle a vendor event
  This table handles vendor events in a generic way.
- Rewrite hci_vendor_evt() so that it compares both vendor prefix
  and subcode to match a vendor event.
- Define set_ext_prefix() to create MSFT extended vendor prefix
  which is returned by msft_get_ext_prefix().
- Do not EXPORT_SYMBOL(mgmt_quality_report).
- Keep msft_get_ext_prefix in msft instead of hci_dev since it is
  not used by any drivers.

Changes in v3:
- Rebase to resolve the code conflict.
- Move aosp_quality_report_evt() from hci_event.c to aosp.c.
- A new patch (3/3) is added to enable the quality report feature.

Changes in v2:
- Scrap the two structures defined in aosp.c and use constants for
  size check.
- Do a basic size check about the quality report event. Do not pull
  data from the event in which the kernel has no interest.
- Define vendor event prefixes with which vendor events of distinct
  vendor specifications can be clearly differentiated.
- Use mgmt helpers to add the header and data to a mgmt skb.

 include/net/bluetooth/hci_core.h | 12 +++++++
 include/net/bluetooth/mgmt.h     |  7 +++++
 net/bluetooth/aosp.c             | 50 +++++++++++++++++++++++++++++
 net/bluetooth/aosp.h             | 18 +++++++++++
 net/bluetooth/hci_event.c        | 54 +++++++++++++++++++++++++++++++-
 net/bluetooth/mgmt.c             | 19 +++++++++++
 net/bluetooth/msft.c             | 28 ++++++++++++++++-
 net/bluetooth/msft.h             | 12 +++++--
 8 files changed, 195 insertions(+), 5 deletions(-)

Comments

Luiz Augusto von Dentz May 26, 2022, 8:25 p.m. UTC | #1

Hi Joseph,

On Thu, May 26, 2022 at 4:21 AM Joseph Hwang <[e-mail]> wrote:
>
> When receiving a HCI vendor event, the kernel checks if it is an
> AOSP bluetooth quality report. If yes, the event is sent to bluez
> user space through the mgmt socket.
>
> Reported-by: kernel test robot <[e-mail]>
>
> Signed-off-by: Joseph Hwang <[e-mail]>
> Reviewed-by: Archie Pusaka <[e-mail]>
> ---
>
> Changes in v6:
> - Fixed a sparse check warning about using static for evt_prefixes.
>
> Changes in v5:
> - Define "struct ext_vendor_prefix" to replace "struct vendor_prefix"
>   so that extended vendor prefix = prefix + 1-octet subcode
> - Define aosp_ext_prefix to provide AOSP extended prefix which is
>   returned by aosp_get_ext_prefix().
> - Redefine struct ext_vendor_event_prefix such that
>   . it uses get_ext_vendor_prefix to get prefix and subcodes where
>     the prefix and the prefix length may be variable and are not
>     unknown until run time;
>   . it uses vendor_func to handle a vendor event
>   This table handles vendor events in a generic way.
> - Rewrite hci_vendor_evt() so that it compares both vendor prefix
>   and subcode to match a vendor event.
> - Define set_ext_prefix() to create MSFT extended vendor prefix
>   which is returned by msft_get_ext_prefix().
> - Do not EXPORT_SYMBOL(mgmt_quality_report).
> - Keep msft_get_ext_prefix in msft instead of hci_dev since it is
>   not used by any drivers.
>
> Changes in v3:
> - Rebase to resolve the code conflict.
> - Move aosp_quality_report_evt() from hci_event.c to aosp.c.
> - A new patch (3/3) is added to enable the quality report feature.
>
> Changes in v2:
> - Scrap the two structures defined in aosp.c and use constants for
>   size check.
> - Do a basic size check about the quality report event. Do not pull
>   data from the event in which the kernel has no interest.
> - Define vendor event prefixes with which vendor events of distinct
>   vendor specifications can be clearly differentiated.
> - Use mgmt helpers to add the header and data to a mgmt skb.
>
>  include/net/bluetooth/hci_core.h | 12 +++++++
>  include/net/bluetooth/mgmt.h     |  7 +++++
>  net/bluetooth/aosp.c             | 50 +++++++++++++++++++++++++++++
>  net/bluetooth/aosp.h             | 18 +++++++++++
>  net/bluetooth/hci_event.c        | 54 +++++++++++++++++++++++++++++++-
>  net/bluetooth/mgmt.c             | 19 +++++++++++
>  net/bluetooth/msft.c             | 28 ++++++++++++++++-
>  net/bluetooth/msft.h             | 12 +++++--
>  8 files changed, 195 insertions(+), 5 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 64d3a63759a8..f89738c6b973 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -328,6 +328,13 @@ struct amp_assoc {
>
>  #define HCI_MAX_PAGES  3
>
> +struct ext_vendor_prefix {
> +       __u8 *prefix;
> +       __u8 prefix_len;
> +       __u8 *subcodes;
> +       __u8 subcodes_len;
> +};
> +
>  struct hci_dev {
>         struct list_head list;
>         struct mutex    lock;
> @@ -1876,6 +1883,8 @@ int mgmt_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status);
>  int mgmt_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status);
>  void mgmt_adv_monitor_device_lost(struct hci_dev *hdev, u16 handle,
>                                   bdaddr_t *bdaddr, u8 addr_type);
> +int mgmt_quality_report(struct hci_dev *hdev, void *data, u32 data_len,
> +                       u8 quality_spec);
>
>  u8 hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max, u16 latency,
>                       u16 to_multiplier);
> @@ -1894,4 +1903,7 @@ void hci_copy_identity_address(struct hci_dev *hdev, bdaddr_t *bdaddr,
>
>  #define TRANSPORT_TYPE_MAX     0x04
>
> +#define QUALITY_SPEC_AOSP_BQR          0x0
> +#define QUALITY_SPEC_INTEL_TELEMETRY   0x1
> +
>  #endif /* __HCI_CORE_H */
> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> index c1c2fd72d9e3..6ccd0067c295 100644
> --- a/include/net/bluetooth/mgmt.h
> +++ b/include/net/bluetooth/mgmt.h
> @@ -1127,3 +1127,10 @@ struct mgmt_ev_adv_monitor_device_lost {
>         __le16 monitor_handle;
>         struct mgmt_addr_info addr;
>  } __packed;
> +
> +#define MGMT_EV_QUALITY_REPORT                 0x0031
> +struct mgmt_ev_quality_report {
> +       __u8    quality_spec;
> +       __u32   data_len;
> +       __u8    data[];
> +} __packed;
> diff --git a/net/bluetooth/aosp.c b/net/bluetooth/aosp.c
> index 432ae3aac9e3..94faa15b1ea0 100644
> --- a/net/bluetooth/aosp.c
> +++ b/net/bluetooth/aosp.c
> @@ -199,3 +199,53 @@ int aosp_set_quality_report(struct hci_dev *hdev, bool enable)
>         else
>                 return disable_quality_report(hdev);
>  }
> +
> +/* The following LEN = 1-byte Sub-event code + 48-byte Sub-event Parameters */
> +#define BLUETOOTH_QUALITY_REPORT_LEN 49
> +
> +bool aosp_check_quality_report_len(struct sk_buff *skb)
> +{
> +       /* skb->len is allowed to be larger than BLUETOOTH_QUALITY_REPORT_LEN
> +        * to accommodate an additional Vendor Specific Parameter (vsp) field.
> +        */
> +       if (skb->len < BLUETOOTH_QUALITY_REPORT_LEN) {
> +               BT_ERR("AOSP evt data len %d too short (%u expected)",
> +                      skb->len, BLUETOOTH_QUALITY_REPORT_LEN);
> +               return false;
> +       }
> +
> +       return true;
> +}
> +
> +/* AOSP HCI Requirements use 0x54 and up as sub-event codes without
> + * actually defining a vendor prefix. Refer to
> + * https://source.android.com/devices/bluetooth/hci_requirements
> + * Hence, the other vendor event prefixes should not use the same
> + * space to avoid collision.
> + * Since the AOSP does not define a prefix, its prefix is NULL
> + * and prefix_len is 0.
> + * While there are a number of subcodes in AOSP, only interested in
> + * Bluetooth Quality Report (0x58) for now.
> + */
> +#define AOSP_EV_QUALITY_REPORT         0x58
> +
> +static unsigned char AOSP_SUBCODES[] = { AOSP_EV_QUALITY_REPORT };
> +
> +static struct ext_vendor_prefix aosp_ext_prefix = {
> +       .prefix         = NULL,
> +       .prefix_len     = 0,
> +       .subcodes       = AOSP_SUBCODES,
> +       .subcodes_len   = sizeof(AOSP_SUBCODES),
> +};
> +
> +struct ext_vendor_prefix *aosp_get_ext_prefix(struct hci_dev *hdev)
> +{
> +       return &aosp_ext_prefix;
> +}
> +
> +void aosp_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb)
> +{
> +       if (aosp_has_quality_report(hdev) && aosp_check_quality_report_len(skb))
> +               mgmt_quality_report(hdev, skb->data, skb->len,
> +                                   QUALITY_SPEC_AOSP_BQR);
> +}
> diff --git a/net/bluetooth/aosp.h b/net/bluetooth/aosp.h
> index 2fd8886d51b2..8208e01fffed 100644
> --- a/net/bluetooth/aosp.h
> +++ b/net/bluetooth/aosp.h
> @@ -10,6 +10,9 @@ void aosp_do_close(struct hci_dev *hdev);
>
>  bool aosp_has_quality_report(struct hci_dev *hdev);
>  int aosp_set_quality_report(struct hci_dev *hdev, bool enable);
> +bool aosp_check_quality_report_len(struct sk_buff *skb);
> +struct ext_vendor_prefix *aosp_get_ext_prefix(struct hci_dev *hdev);
> +void aosp_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb);
>
>  #else
>
> @@ -26,4 +29,19 @@ static inline int aosp_set_quality_report(struct hci_dev *hdev, bool enable)
>         return -EOPNOTSUPP;
>  }
>
> +static inline bool aosp_check_quality_report_len(struct sk_buff *skb)
> +{
> +       return false;
> +}
> +
> +static inline struct ext_vendor_prefix *
> +aosp_get_ext_prefix(struct hci_dev *hdev)
> +{
> +       return NULL;
> +}
> +
> +static inline void aosp_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb)
> +{
> +}
> +
>  #endif
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 0270e597c285..8398971eddf4 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -37,6 +37,7 @@
>  #include "smp.h"
>  #include "msft.h"
>  #include "eir.h"
> +#include "aosp.h"
>
>  #define ZERO_KEY "\x00\x00\x00\x00\x00\x00\x00\x00" \
>                  "\x00\x00\x00\x00\x00\x00\x00\x00"
> @@ -4259,6 +4260,57 @@ static void hci_num_comp_blocks_evt(struct hci_dev *hdev, void *data,
>         queue_work(hdev->workqueue, &hdev->tx_work);
>  }
>
> +/* Every distinct vendor specification must have a well-defined vendor
> + * event prefix to determine if a vendor event meets the specification.
> + * Some vendor prefixes are fixed values while some other vendor prefixes
> + * are only available at run time.
> + */
> +static struct ext_vendor_event_prefix {
> +       /* Some vendor prefixes are variable length. For convenience,
> +        * the prefix in struct ext_vendor_prefix is in little endian.
> +        */
> +       struct ext_vendor_prefix *
> +               (*get_ext_vendor_prefix)(struct hci_dev *hdev);
> +       void (*vendor_func)(struct hci_dev *hdev, struct sk_buff *skb);
> +} evt_prefixes[] = {
> +       { aosp_get_ext_prefix, aosp_vendor_evt },
> +       { msft_get_ext_prefix, msft_vendor_evt },
> +
> +       /* end with a null entry */
> +       {},
> +};
> +
> +static void hci_vendor_evt(struct hci_dev *hdev, void *data,
> +                          struct sk_buff *skb)
> +{
> +       int i, j;
> +       struct ext_vendor_prefix *vnd;
> +       __u8 subcode;
> +
> +       for (i = 0; evt_prefixes[i].get_ext_vendor_prefix; i++) {
> +               vnd = evt_prefixes[i].get_ext_vendor_prefix(hdev);
> +               if (!vnd)
> +                       continue;
> +
> +               /* Compare the raw prefix data in little endian directly. */
> +               if (memcmp(vnd->prefix, skb->data, vnd->prefix_len))
> +                       continue;
> +
> +               /* Make sure that there are more data after prefix. */
> +               if (skb->len <= vnd->prefix_len)
> +                       continue;
> +
> +               /* The subcode is the single octet following the prefix. */
> +               subcode = skb->data[vnd->prefix_len];
> +               for (j = 0; j < vnd->subcodes_len; j++) {
> +                       if (vnd->subcodes[j] == subcode) {
> +                               evt_prefixes[i].vendor_func(hdev, skb);
> +                               break;
> +                       }
> +               }
> +       }
> +}

I recall saying that having such matching logic applied without the
driver confirming that is the structure it using to be a bad idea
since it could actually cause an event to misinterpret and cause bad
behavior, instead we probably need a callback that gets populated by
the driver e.g.(hdev->vendor_evt) then the driver can either populate
with hci_vendor_evt if it does use prefixes or its own specialized
function or NULL if it doesn't use vendor events, specially for old
controllers Id leave it as NULL.

>  static void hci_mode_change_evt(struct hci_dev *hdev, void *data,
>                                 struct sk_buff *skb)
>  {
> @@ -6879,7 +6931,7 @@ static const struct hci_ev {
>         HCI_EV(HCI_EV_NUM_COMP_BLOCKS, hci_num_comp_blocks_evt,
>                sizeof(struct hci_ev_num_comp_blocks)),
>         /* [0xff = HCI_EV_VENDOR] */
> -       HCI_EV_VL(HCI_EV_VENDOR, msft_vendor_evt, 0, HCI_MAX_EVENT_SIZE),
> +       HCI_EV_VL(HCI_EV_VENDOR, hci_vendor_evt, 0, HCI_MAX_EVENT_SIZE),
>  };
>
>  static void hci_event_func(struct hci_dev *hdev, u8 event, struct sk_buff *skb,
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 1ad84f34097f..9d3666bdd07c 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -4332,6 +4332,25 @@ static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
>                                MGMT_STATUS_NOT_SUPPORTED);
>  }
>
> +int mgmt_quality_report(struct hci_dev *hdev, void *data, u32 data_len,
> +                       u8 quality_spec)
> +{
> +       struct mgmt_ev_quality_report *ev;
> +       struct sk_buff *skb;
> +
> +       skb = mgmt_alloc_skb(hdev, MGMT_EV_QUALITY_REPORT,
> +                            sizeof(*ev) + data_len);
> +       if (!skb)
> +               return -ENOMEM;
> +
> +       ev = skb_put(skb, sizeof(*ev));
> +       ev->quality_spec = quality_spec;
> +       ev->data_len = data_len;
> +       skb_put_data(skb, data, data_len);
> +
> +       return mgmt_event_skb(skb, NULL);
> +}
> +
>  static int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
>                             u16 data_len)
>  {
> diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
> index f43994523b1f..c003e94faccd 100644
> --- a/net/bluetooth/msft.c
> +++ b/net/bluetooth/msft.c
> @@ -116,6 +116,20 @@ bool msft_monitor_supported(struct hci_dev *hdev)
>         return !!(msft_get_features(hdev) & MSFT_FEATURE_MASK_LE_ADV_MONITOR);
>  }
>
> +/* Add the MSFT vendor event subcodes into MSFT_SUBCODES which
> + * msft_vendor_evt() is interested in handling.
> + */
> +static unsigned char MSFT_SUBCODES[] = { MSFT_EV_LE_MONITOR_DEVICE };
> +static struct ext_vendor_prefix msft_ext_prefix = { 0 };
> +
> +static void set_ext_prefix(struct msft_data *msft)
> +{
> +       msft_ext_prefix.prefix = msft->evt_prefix;
> +       msft_ext_prefix.prefix_len = msft->evt_prefix_len;
> +       msft_ext_prefix.subcodes = MSFT_SUBCODES;
> +       msft_ext_prefix.subcodes_len = sizeof(MSFT_SUBCODES);
> +}
> +
>  static bool read_supported_features(struct hci_dev *hdev,
>                                     struct msft_data *msft)
>  {
> @@ -156,6 +170,8 @@ static bool read_supported_features(struct hci_dev *hdev,
>         if (msft->features & MSFT_FEATURE_MASK_CURVE_VALIDITY)
>                 hdev->msft_curve_validity = true;
>
> +       set_ext_prefix(msft);
> +
>         kfree_skb(skb);
>         return true;
>
> @@ -742,7 +758,17 @@ static void msft_monitor_device_evt(struct hci_dev *hdev, struct sk_buff *skb)
>                                  handle_data->mgmt_handle);
>  }
>
> -void msft_vendor_evt(struct hci_dev *hdev, void *data, struct sk_buff *skb)
> +struct ext_vendor_prefix *msft_get_ext_prefix(struct hci_dev *hdev)
> +{
> +       struct msft_data *msft = hdev->msft_data;
> +
> +       if (!msft)
> +               return NULL;
> +
> +       return &msft_ext_prefix;
> +}
> +
> +void msft_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb)
>  {
>         struct msft_data *msft = hdev->msft_data;
>         u8 *evt_prefix;
> diff --git a/net/bluetooth/msft.h b/net/bluetooth/msft.h
> index afcaf7d3b1cb..1515ae06c628 100644
> --- a/net/bluetooth/msft.h
> +++ b/net/bluetooth/msft.h
> @@ -17,7 +17,7 @@ void msft_register(struct hci_dev *hdev);
>  void msft_unregister(struct hci_dev *hdev);
>  void msft_do_open(struct hci_dev *hdev);
>  void msft_do_close(struct hci_dev *hdev);
> -void msft_vendor_evt(struct hci_dev *hdev, void *data, struct sk_buff *skb);
> +void msft_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb);
>  __u64 msft_get_features(struct hci_dev *hdev);
>  int msft_add_monitor_pattern(struct hci_dev *hdev, struct adv_monitor *monitor);
>  int msft_remove_monitor(struct hci_dev *hdev, struct adv_monitor *monitor,
> @@ -27,6 +27,7 @@ int msft_set_filter_enable(struct hci_dev *hdev, bool enable);
>  int msft_suspend_sync(struct hci_dev *hdev);
>  int msft_resume_sync(struct hci_dev *hdev);
>  bool msft_curve_validity(struct hci_dev *hdev);
> +struct ext_vendor_prefix *msft_get_ext_prefix(struct hci_dev *hdev);
>
>  #else
>
> @@ -39,8 +40,7 @@ static inline void msft_register(struct hci_dev *hdev) {}
>  static inline void msft_unregister(struct hci_dev *hdev) {}
>  static inline void msft_do_open(struct hci_dev *hdev) {}
>  static inline void msft_do_close(struct hci_dev *hdev) {}
> -static inline void msft_vendor_evt(struct hci_dev *hdev, void *data,
> -                                  struct sk_buff *skb) {}
> +static inline void msft_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb) {}
>  static inline __u64 msft_get_features(struct hci_dev *hdev) { return 0; }
>  static inline int msft_add_monitor_pattern(struct hci_dev *hdev,
>                                            struct adv_monitor *monitor)
> @@ -77,4 +77,10 @@ static inline bool msft_curve_validity(struct hci_dev *hdev)
>         return false;
>  }
>
> +static inline struct ext_vendor_prefix *
> +msft_get_ext_prefix(struct hci_dev *hdev)
> +{
> +       return NULL;
> +}
> +
>  #endif
> --
> 2.36.1.124.g0e6072fb45-goog
>

Joseph Hwang May 27, 2022, 4:36 a.m. UTC | #2

Hi Luiz:

Thanks for your review! The get_ext_vendor_prefix() in the table
provides a *unique* extended vendor prefix ( = vendor prefix + 1-octet
subcode) that can uniquely identify a vendor event. I am not aware of
any situation that might cause an event to be incorrectly matched with
an extended vendor prefix. Maybe I am missing something?

On the other hand, in your comment, to let a driver confirm whether it
is the vendor event structure it uses might be a bit risky. For
example, assume that we pass a vendor event to
msft.c:msft_vendor_evt() to determine whether it is a MSFT event. The
current implementation of msft_vendor_evt() is to call skb_pull_data()
to pull the event prefix for comparison with the dynamic MSFT event
prefix. No matter whether the event matches or not, the event skb has
been modified already and would cause bad behavior if we pass the
event skb to other vendor drivers/functions. How can we generally make
sure that every such vendor drivers/functions are implemented in a
read-only way that does not modify the skb when comparing the prefix?
In this patch, we propose to use get_ext_vendor_prefix() which is
guaranteed not to modify the skb in any possible way.

Please also note that the mechanism here also takes care of older
controllers that might not support some of the vendor specifications.
For example, if an older controller does not support the MSFT spec,
the msft_get_ext_prefix() would return NULL as its prefix. And hence a
vendor event would not accidentally match the MSFT spec on the older
controller. Similarly, in the following patch “btintel: setup
vendor_get_prefix and vendor_evt”, on an older Intel controller that
does not support Intel telemetry events, the btintel driver would
*not* set up

    hdev->vendor_get_ext_prefix = btintel_get_ext_prefix;

such that an event would not match as an Intel vendor event in any way.

Please let me know if I have any misunderstanding.

Thanks and regards,
Joseph


On Fri, May 27, 2022 at 4:25 AM Luiz Augusto von Dentz
<[e-mail]> wrote:
>
> Hi Joseph,
>
> On Thu, May 26, 2022 at 4:21 AM Joseph Hwang <[e-mail]> wrote:
> >
> > When receiving a HCI vendor event, the kernel checks if it is an
> > AOSP bluetooth quality report. If yes, the event is sent to bluez
> > user space through the mgmt socket.
> >
> > Reported-by: kernel test robot <[e-mail]>
> >
> > Signed-off-by: Joseph Hwang <[e-mail]>
> > Reviewed-by: Archie Pusaka <[e-mail]>
> > ---
> >
> > Changes in v6:
> > - Fixed a sparse check warning about using static for evt_prefixes.
> >
> > Changes in v5:
> > - Define "struct ext_vendor_prefix" to replace "struct vendor_prefix"
> >   so that extended vendor prefix = prefix + 1-octet subcode
> > - Define aosp_ext_prefix to provide AOSP extended prefix which is
> >   returned by aosp_get_ext_prefix().
> > - Redefine struct ext_vendor_event_prefix such that
> >   . it uses get_ext_vendor_prefix to get prefix and subcodes where
> >     the prefix and the prefix length may be variable and are not
> >     unknown until run time;
> >   . it uses vendor_func to handle a vendor event
> >   This table handles vendor events in a generic way.
> > - Rewrite hci_vendor_evt() so that it compares both vendor prefix
> >   and subcode to match a vendor event.
> > - Define set_ext_prefix() to create MSFT extended vendor prefix
> >   which is returned by msft_get_ext_prefix().
> > - Do not EXPORT_SYMBOL(mgmt_quality_report).
> > - Keep msft_get_ext_prefix in msft instead of hci_dev since it is
> >   not used by any drivers.
> >
> > Changes in v3:
> > - Rebase to resolve the code conflict.
> > - Move aosp_quality_report_evt() from hci_event.c to aosp.c.
> > - A new patch (3/3) is added to enable the quality report feature.
> >
> > Changes in v2:
> > - Scrap the two structures defined in aosp.c and use constants for
> >   size check.
> > - Do a basic size check about the quality report event. Do not pull
> >   data from the event in which the kernel has no interest.
> > - Define vendor event prefixes with which vendor events of distinct
> >   vendor specifications can be clearly differentiated.
> > - Use mgmt helpers to add the header and data to a mgmt skb.
> >
> >  include/net/bluetooth/hci_core.h | 12 +++++++
> >  include/net/bluetooth/mgmt.h     |  7 +++++
> >  net/bluetooth/aosp.c             | 50 +++++++++++++++++++++++++++++
> >  net/bluetooth/aosp.h             | 18 +++++++++++
> >  net/bluetooth/hci_event.c        | 54 +++++++++++++++++++++++++++++++-
> >  net/bluetooth/mgmt.c             | 19 +++++++++++
> >  net/bluetooth/msft.c             | 28 ++++++++++++++++-
> >  net/bluetooth/msft.h             | 12 +++++--
> >  8 files changed, 195 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index 64d3a63759a8..f89738c6b973 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -328,6 +328,13 @@ struct amp_assoc {
> >
> >  #define HCI_MAX_PAGES  3
> >
> > +struct ext_vendor_prefix {
> > +       __u8 *prefix;
> > +       __u8 prefix_len;
> > +       __u8 *subcodes;
> > +       __u8 subcodes_len;
> > +};
> > +
> >  struct hci_dev {
> >         struct list_head list;
> >         struct mutex    lock;
> > @@ -1876,6 +1883,8 @@ int mgmt_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status);
> >  int mgmt_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status);
> >  void mgmt_adv_monitor_device_lost(struct hci_dev *hdev, u16 handle,
> >                                   bdaddr_t *bdaddr, u8 addr_type);
> > +int mgmt_quality_report(struct hci_dev *hdev, void *data, u32 data_len,
> > +                       u8 quality_spec);
> >
> >  u8 hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max, u16 latency,
> >                       u16 to_multiplier);
> > @@ -1894,4 +1903,7 @@ void hci_copy_identity_address(struct hci_dev *hdev, bdaddr_t *bdaddr,
> >
> >  #define TRANSPORT_TYPE_MAX     0x04
> >
> > +#define QUALITY_SPEC_AOSP_BQR          0x0
> > +#define QUALITY_SPEC_INTEL_TELEMETRY   0x1
> > +
> >  #endif /* __HCI_CORE_H */
> > diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> > index c1c2fd72d9e3..6ccd0067c295 100644
> > --- a/include/net/bluetooth/mgmt.h
> > +++ b/include/net/bluetooth/mgmt.h
> > @@ -1127,3 +1127,10 @@ struct mgmt_ev_adv_monitor_device_lost {
> >         __le16 monitor_handle;
> >         struct mgmt_addr_info addr;
> >  } __packed;
> > +
> > +#define MGMT_EV_QUALITY_REPORT                 0x0031
> > +struct mgmt_ev_quality_report {
> > +       __u8    quality_spec;
> > +       __u32   data_len;
> > +       __u8    data[];
> > +} __packed;
> > diff --git a/net/bluetooth/aosp.c b/net/bluetooth/aosp.c
> > index 432ae3aac9e3..94faa15b1ea0 100644
> > --- a/net/bluetooth/aosp.c
> > +++ b/net/bluetooth/aosp.c
> > @@ -199,3 +199,53 @@ int aosp_set_quality_report(struct hci_dev *hdev, bool enable)
> >         else
> >                 return disable_quality_report(hdev);
> >  }
> > +
> > +/* The following LEN = 1-byte Sub-event code + 48-byte Sub-event Parameters */
> > +#define BLUETOOTH_QUALITY_REPORT_LEN 49
> > +
> > +bool aosp_check_quality_report_len(struct sk_buff *skb)
> > +{
> > +       /* skb->len is allowed to be larger than BLUETOOTH_QUALITY_REPORT_LEN
> > +        * to accommodate an additional Vendor Specific Parameter (vsp) field.
> > +        */
> > +       if (skb->len < BLUETOOTH_QUALITY_REPORT_LEN) {
> > +               BT_ERR("AOSP evt data len %d too short (%u expected)",
> > +                      skb->len, BLUETOOTH_QUALITY_REPORT_LEN);
> > +               return false;
> > +       }
> > +
> > +       return true;
> > +}
> > +
> > +/* AOSP HCI Requirements use 0x54 and up as sub-event codes without
> > + * actually defining a vendor prefix. Refer to
> > + * https://source.android.com/devices/bluetooth/hci_requirements
> > + * Hence, the other vendor event prefixes should not use the same
> > + * space to avoid collision.
> > + * Since the AOSP does not define a prefix, its prefix is NULL
> > + * and prefix_len is 0.
> > + * While there are a number of subcodes in AOSP, only interested in
> > + * Bluetooth Quality Report (0x58) for now.
> > + */
> > +#define AOSP_EV_QUALITY_REPORT         0x58
> > +
> > +static unsigned char AOSP_SUBCODES[] = { AOSP_EV_QUALITY_REPORT };
> > +
> > +static struct ext_vendor_prefix aosp_ext_prefix = {
> > +       .prefix         = NULL,
> > +       .prefix_len     = 0,
> > +       .subcodes       = AOSP_SUBCODES,
> > +       .subcodes_len   = sizeof(AOSP_SUBCODES),
> > +};
> > +
> > +struct ext_vendor_prefix *aosp_get_ext_prefix(struct hci_dev *hdev)
> > +{
> > +       return &aosp_ext_prefix;
> > +}
> > +
> > +void aosp_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb)
> > +{
> > +       if (aosp_has_quality_report(hdev) && aosp_check_quality_report_len(skb))
> > +               mgmt_quality_report(hdev, skb->data, skb->len,
> > +                                   QUALITY_SPEC_AOSP_BQR);
> > +}
> > diff --git a/net/bluetooth/aosp.h b/net/bluetooth/aosp.h
> > index 2fd8886d51b2..8208e01fffed 100644
> > --- a/net/bluetooth/aosp.h
> > +++ b/net/bluetooth/aosp.h
> > @@ -10,6 +10,9 @@ void aosp_do_close(struct hci_dev *hdev);
> >
> >  bool aosp_has_quality_report(struct hci_dev *hdev);
> >  int aosp_set_quality_report(struct hci_dev *hdev, bool enable);
> > +bool aosp_check_quality_report_len(struct sk_buff *skb);
> > +struct ext_vendor_prefix *aosp_get_ext_prefix(struct hci_dev *hdev);
> > +void aosp_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb);
> >
> >  #else
> >
> > @@ -26,4 +29,19 @@ static inline int aosp_set_quality_report(struct hci_dev *hdev, bool enable)
> >         return -EOPNOTSUPP;
> >  }
> >
> > +static inline bool aosp_check_quality_report_len(struct sk_buff *skb)
> > +{
> > +       return false;
> > +}
> > +
> > +static inline struct ext_vendor_prefix *
> > +aosp_get_ext_prefix(struct hci_dev *hdev)
> > +{
> > +       return NULL;
> > +}
> > +
> > +static inline void aosp_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb)
> > +{
> > +}
> > +
> >  #endif
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index 0270e597c285..8398971eddf4 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -37,6 +37,7 @@
> >  #include "smp.h"
> >  #include "msft.h"
> >  #include "eir.h"
> > +#include "aosp.h"
> >
> >  #define ZERO_KEY "\x00\x00\x00\x00\x00\x00\x00\x00" \
> >                  "\x00\x00\x00\x00\x00\x00\x00\x00"
> > @@ -4259,6 +4260,57 @@ static void hci_num_comp_blocks_evt(struct hci_dev *hdev, void *data,
> >         queue_work(hdev->workqueue, &hdev->tx_work);
> >  }
> >
> > +/* Every distinct vendor specification must have a well-defined vendor
> > + * event prefix to determine if a vendor event meets the specification.
> > + * Some vendor prefixes are fixed values while some other vendor prefixes
> > + * are only available at run time.
> > + */
> > +static struct ext_vendor_event_prefix {
> > +       /* Some vendor prefixes are variable length. For convenience,
> > +        * the prefix in struct ext_vendor_prefix is in little endian.
> > +        */
> > +       struct ext_vendor_prefix *
> > +               (*get_ext_vendor_prefix)(struct hci_dev *hdev);
> > +       void (*vendor_func)(struct hci_dev *hdev, struct sk_buff *skb);
> > +} evt_prefixes[] = {
> > +       { aosp_get_ext_prefix, aosp_vendor_evt },
> > +       { msft_get_ext_prefix, msft_vendor_evt },
> > +
> > +       /* end with a null entry */
> > +       {},
> > +};
> > +
> > +static void hci_vendor_evt(struct hci_dev *hdev, void *data,
> > +                          struct sk_buff *skb)
> > +{
> > +       int i, j;
> > +       struct ext_vendor_prefix *vnd;
> > +       __u8 subcode;
> > +
> > +       for (i = 0; evt_prefixes[i].get_ext_vendor_prefix; i++) {
> > +               vnd = evt_prefixes[i].get_ext_vendor_prefix(hdev);
> > +               if (!vnd)
> > +                       continue;
> > +
> > +               /* Compare the raw prefix data in little endian directly. */
> > +               if (memcmp(vnd->prefix, skb->data, vnd->prefix_len))
> > +                       continue;
> > +
> > +               /* Make sure that there are more data after prefix. */
> > +               if (skb->len <= vnd->prefix_len)
> > +                       continue;
> > +
> > +               /* The subcode is the single octet following the prefix. */
> > +               subcode = skb->data[vnd->prefix_len];
> > +               for (j = 0; j < vnd->subcodes_len; j++) {
> > +                       if (vnd->subcodes[j] == subcode) {
> > +                               evt_prefixes[i].vendor_func(hdev, skb);
> > +                               break;
> > +                       }
> > +               }
> > +       }
> > +}
>
> I recall saying that having such matching logic applied without the
> driver confirming that is the structure it using to be a bad idea
> since it could actually cause an event to misinterpret and cause bad
> behavior, instead we probably need a callback that gets populated by
> the driver e.g.(hdev->vendor_evt) then the driver can either populate
> with hci_vendor_evt if it does use prefixes or its own specialized
> function or NULL if it doesn't use vendor events, specially for old
> controllers Id leave it as NULL.
>
> >  static void hci_mode_change_evt(struct hci_dev *hdev, void *data,
> >                                 struct sk_buff *skb)
> >  {
> > @@ -6879,7 +6931,7 @@ static const struct hci_ev {
> >         HCI_EV(HCI_EV_NUM_COMP_BLOCKS, hci_num_comp_blocks_evt,
> >                sizeof(struct hci_ev_num_comp_blocks)),
> >         /* [0xff = HCI_EV_VENDOR] */
> > -       HCI_EV_VL(HCI_EV_VENDOR, msft_vendor_evt, 0, HCI_MAX_EVENT_SIZE),
> > +       HCI_EV_VL(HCI_EV_VENDOR, hci_vendor_evt, 0, HCI_MAX_EVENT_SIZE),
> >  };
> >
> >  static void hci_event_func(struct hci_dev *hdev, u8 event, struct sk_buff *skb,
> > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> > index 1ad84f34097f..9d3666bdd07c 100644
> > --- a/net/bluetooth/mgmt.c
> > +++ b/net/bluetooth/mgmt.c
> > @@ -4332,6 +4332,25 @@ static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
> >                                MGMT_STATUS_NOT_SUPPORTED);
> >  }
> >
> > +int mgmt_quality_report(struct hci_dev *hdev, void *data, u32 data_len,
> > +                       u8 quality_spec)
> > +{
> > +       struct mgmt_ev_quality_report *ev;
> > +       struct sk_buff *skb;
> > +
> > +       skb = mgmt_alloc_skb(hdev, MGMT_EV_QUALITY_REPORT,
> > +                            sizeof(*ev) + data_len);
> > +       if (!skb)
> > +               return -ENOMEM;
> > +
> > +       ev = skb_put(skb, sizeof(*ev));
> > +       ev->quality_spec = quality_spec;
> > +       ev->data_len = data_len;
> > +       skb_put_data(skb, data, data_len);
> > +
> > +       return mgmt_event_skb(skb, NULL);
> > +}
> > +
> >  static int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
> >                             u16 data_len)
> >  {
> > diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
> > index f43994523b1f..c003e94faccd 100644
> > --- a/net/bluetooth/msft.c
> > +++ b/net/bluetooth/msft.c
> > @@ -116,6 +116,20 @@ bool msft_monitor_supported(struct hci_dev *hdev)
> >         return !!(msft_get_features(hdev) & MSFT_FEATURE_MASK_LE_ADV_MONITOR);
> >  }
> >
> > +/* Add the MSFT vendor event subcodes into MSFT_SUBCODES which
> > + * msft_vendor_evt() is interested in handling.
> > + */
> > +static unsigned char MSFT_SUBCODES[] = { MSFT_EV_LE_MONITOR_DEVICE };
> > +static struct ext_vendor_prefix msft_ext_prefix = { 0 };
> > +
> > +static void set_ext_prefix(struct msft_data *msft)
> > +{
> > +       msft_ext_prefix.prefix = msft->evt_prefix;
> > +       msft_ext_prefix.prefix_len = msft->evt_prefix_len;
> > +       msft_ext_prefix.subcodes = MSFT_SUBCODES;
> > +       msft_ext_prefix.subcodes_len = sizeof(MSFT_SUBCODES);
> > +}
> > +
> >  static bool read_supported_features(struct hci_dev *hdev,
> >                                     struct msft_data *msft)
> >  {
> > @@ -156,6 +170,8 @@ static bool read_supported_features(struct hci_dev *hdev,
> >         if (msft->features & MSFT_FEATURE_MASK_CURVE_VALIDITY)
> >                 hdev->msft_curve_validity = true;
> >
> > +       set_ext_prefix(msft);
> > +
> >         kfree_skb(skb);
> >         return true;
> >
> > @@ -742,7 +758,17 @@ static void msft_monitor_device_evt(struct hci_dev *hdev, struct sk_buff *skb)
> >                                  handle_data->mgmt_handle);
> >  }
> >
> > -void msft_vendor_evt(struct hci_dev *hdev, void *data, struct sk_buff *skb)
> > +struct ext_vendor_prefix *msft_get_ext_prefix(struct hci_dev *hdev)
> > +{
> > +       struct msft_data *msft = hdev->msft_data;
> > +
> > +       if (!msft)
> > +               return NULL;
> > +
> > +       return &msft_ext_prefix;
> > +}
> > +
> > +void msft_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb)
> >  {
> >         struct msft_data *msft = hdev->msft_data;
> >         u8 *evt_prefix;
> > diff --git a/net/bluetooth/msft.h b/net/bluetooth/msft.h
> > index afcaf7d3b1cb..1515ae06c628 100644
> > --- a/net/bluetooth/msft.h
> > +++ b/net/bluetooth/msft.h
> > @@ -17,7 +17,7 @@ void msft_register(struct hci_dev *hdev);
> >  void msft_unregister(struct hci_dev *hdev);
> >  void msft_do_open(struct hci_dev *hdev);
> >  void msft_do_close(struct hci_dev *hdev);
> > -void msft_vendor_evt(struct hci_dev *hdev, void *data, struct sk_buff *skb);
> > +void msft_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb);
> >  __u64 msft_get_features(struct hci_dev *hdev);
> >  int msft_add_monitor_pattern(struct hci_dev *hdev, struct adv_monitor *monitor);
> >  int msft_remove_monitor(struct hci_dev *hdev, struct adv_monitor *monitor,
> > @@ -27,6 +27,7 @@ int msft_set_filter_enable(struct hci_dev *hdev, bool enable);
> >  int msft_suspend_sync(struct hci_dev *hdev);
> >  int msft_resume_sync(struct hci_dev *hdev);
> >  bool msft_curve_validity(struct hci_dev *hdev);
> > +struct ext_vendor_prefix *msft_get_ext_prefix(struct hci_dev *hdev);
> >
> >  #else
> >
> > @@ -39,8 +40,7 @@ static inline void msft_register(struct hci_dev *hdev) {}
> >  static inline void msft_unregister(struct hci_dev *hdev) {}
> >  static inline void msft_do_open(struct hci_dev *hdev) {}
> >  static inline void msft_do_close(struct hci_dev *hdev) {}
> > -static inline void msft_vendor_evt(struct hci_dev *hdev, void *data,
> > -                                  struct sk_buff *skb) {}
> > +static inline void msft_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb) {}
> >  static inline __u64 msft_get_features(struct hci_dev *hdev) { return 0; }
> >  static inline int msft_add_monitor_pattern(struct hci_dev *hdev,
> >                                            struct adv_monitor *monitor)
> > @@ -77,4 +77,10 @@ static inline bool msft_curve_validity(struct hci_dev *hdev)
> >         return false;
> >  }
> >
> > +static inline struct ext_vendor_prefix *
> > +msft_get_ext_prefix(struct hci_dev *hdev)
> > +{
> > +       return NULL;
> > +}
> > +
> >  #endif
> > --
> > 2.36.1.124.g0e6072fb45-goog
> >
>
>
> --
> Luiz Augusto von Dentz

Luiz Augusto von Dentz May 30, 2022, 9 p.m. UTC | #3

Hi Joseph,

On Thu, May 26, 2022 at 9:37 PM Joseph Hwang <[e-mail]> wrote:
>
> Hi Luiz:
>
> Thanks for your review! The get_ext_vendor_prefix() in the table
> provides a *unique* extended vendor prefix ( = vendor prefix + 1-octet
> subcode) that can uniquely identify a vendor event. I am not aware of
> any situation that might cause an event to be incorrectly matched with
> an extended vendor prefix. Maybe I am missing something?
>
> On the other hand, in your comment, to let a driver confirm whether it
> is the vendor event structure it uses might be a bit risky. For
> example, assume that we pass a vendor event to
> msft.c:msft_vendor_evt() to determine whether it is a MSFT event. The
> current implementation of msft_vendor_evt() is to call skb_pull_data()
> to pull the event prefix for comparison with the dynamic MSFT event
> prefix. No matter whether the event matches or not, the event skb has
> been modified already and would cause bad behavior if we pass the
> event skb to other vendor drivers/functions. How can we generally make
> sure that every such vendor drivers/functions are implemented in a
> read-only way that does not modify the skb when comparing the prefix?
> In this patch, we propose to use get_ext_vendor_prefix() which is
> guaranteed not to modify the skb in any possible way.
>
> Please also note that the mechanism here also takes care of older
> controllers that might not support some of the vendor specifications.
> For example, if an older controller does not support the MSFT spec,
> the msft_get_ext_prefix() would return NULL as its prefix. And hence a
> vendor event would not accidentally match the MSFT spec on the older
> controller. Similarly, in the following patch “btintel: setup
> vendor_get_prefix and vendor_evt”, on an older Intel controller that
> does not support Intel telemetry events, the btintel driver would
> *not* set up
>
>     hdev->vendor_get_ext_prefix = btintel_get_ext_prefix;

I see, while this does indeed prevent events to be misinterpreted,
this locks us on only supporting vendor commands which use vendor
prefixes, but perhaps that is fine since I assume there is probably no
better way to create vendor opcodes in the first place.

> such that an event would not match as an Intel vendor event in any way.
>
> Please let me know if I have any misunderstanding.
>
> Thanks and regards,
> Joseph
>
>
> On Fri, May 27, 2022 at 4:25 AM Luiz Augusto von Dentz
> <[e-mail]> wrote:
> >
> > Hi Joseph,
> >
> > On Thu, May 26, 2022 at 4:21 AM Joseph Hwang <[e-mail]> wrote:
> > >
> > > When receiving a HCI vendor event, the kernel checks if it is an
> > > AOSP bluetooth quality report. If yes, the event is sent to bluez
> > > user space through the mgmt socket.
> > >
> > > Reported-by: kernel test robot <[e-mail]>
> > >
> > > Signed-off-by: Joseph Hwang <[e-mail]>
> > > Reviewed-by: Archie Pusaka <[e-mail]>
> > > ---
> > >
> > > Changes in v6:
> > > - Fixed a sparse check warning about using static for evt_prefixes.
> > >
> > > Changes in v5:
> > > - Define "struct ext_vendor_prefix" to replace "struct vendor_prefix"
> > >   so that extended vendor prefix = prefix + 1-octet subcode
> > > - Define aosp_ext_prefix to provide AOSP extended prefix which is
> > >   returned by aosp_get_ext_prefix().
> > > - Redefine struct ext_vendor_event_prefix such that
> > >   . it uses get_ext_vendor_prefix to get prefix and subcodes where
> > >     the prefix and the prefix length may be variable and are not
> > >     unknown until run time;
> > >   . it uses vendor_func to handle a vendor event
> > >   This table handles vendor events in a generic way.
> > > - Rewrite hci_vendor_evt() so that it compares both vendor prefix
> > >   and subcode to match a vendor event.
> > > - Define set_ext_prefix() to create MSFT extended vendor prefix
> > >   which is returned by msft_get_ext_prefix().
> > > - Do not EXPORT_SYMBOL(mgmt_quality_report).
> > > - Keep msft_get_ext_prefix in msft instead of hci_dev since it is
> > >   not used by any drivers.
> > >
> > > Changes in v3:
> > > - Rebase to resolve the code conflict.
> > > - Move aosp_quality_report_evt() from hci_event.c to aosp.c.
> > > - A new patch (3/3) is added to enable the quality report feature.
> > >
> > > Changes in v2:
> > > - Scrap the two structures defined in aosp.c and use constants for
> > >   size check.
> > > - Do a basic size check about the quality report event. Do not pull
> > >   data from the event in which the kernel has no interest.
> > > - Define vendor event prefixes with which vendor events of distinct
> > >   vendor specifications can be clearly differentiated.
> > > - Use mgmt helpers to add the header and data to a mgmt skb.
> > >
> > >  include/net/bluetooth/hci_core.h | 12 +++++++
> > >  include/net/bluetooth/mgmt.h     |  7 +++++
> > >  net/bluetooth/aosp.c             | 50 +++++++++++++++++++++++++++++
> > >  net/bluetooth/aosp.h             | 18 +++++++++++
> > >  net/bluetooth/hci_event.c        | 54 +++++++++++++++++++++++++++++++-
> > >  net/bluetooth/mgmt.c             | 19 +++++++++++
> > >  net/bluetooth/msft.c             | 28 ++++++++++++++++-
> > >  net/bluetooth/msft.h             | 12 +++++--
> > >  8 files changed, 195 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > > index 64d3a63759a8..f89738c6b973 100644
> > > --- a/include/net/bluetooth/hci_core.h
> > > +++ b/include/net/bluetooth/hci_core.h
> > > @@ -328,6 +328,13 @@ struct amp_assoc {
> > >
> > >  #define HCI_MAX_PAGES  3
> > >
> > > +struct ext_vendor_prefix {
> > > +       __u8 *prefix;
> > > +       __u8 prefix_len;
> > > +       __u8 *subcodes;
> > > +       __u8 subcodes_len;
> > > +};
> > > +
> > >  struct hci_dev {
> > >         struct list_head list;
> > >         struct mutex    lock;
> > > @@ -1876,6 +1883,8 @@ int mgmt_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status);
> > >  int mgmt_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status);
> > >  void mgmt_adv_monitor_device_lost(struct hci_dev *hdev, u16 handle,
> > >                                   bdaddr_t *bdaddr, u8 addr_type);
> > > +int mgmt_quality_report(struct hci_dev *hdev, void *data, u32 data_len,
> > > +                       u8 quality_spec);
> > >
> > >  u8 hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max, u16 latency,
> > >                       u16 to_multiplier);
> > > @@ -1894,4 +1903,7 @@ void hci_copy_identity_address(struct hci_dev *hdev, bdaddr_t *bdaddr,
> > >
> > >  #define TRANSPORT_TYPE_MAX     0x04
> > >
> > > +#define QUALITY_SPEC_AOSP_BQR          0x0
> > > +#define QUALITY_SPEC_INTEL_TELEMETRY   0x1
> > > +
> > >  #endif /* __HCI_CORE_H */
> > > diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> > > index c1c2fd72d9e3..6ccd0067c295 100644
> > > --- a/include/net/bluetooth/mgmt.h
> > > +++ b/include/net/bluetooth/mgmt.h
> > > @@ -1127,3 +1127,10 @@ struct mgmt_ev_adv_monitor_device_lost {
> > >         __le16 monitor_handle;
> > >         struct mgmt_addr_info addr;
> > >  } __packed;
> > > +
> > > +#define MGMT_EV_QUALITY_REPORT                 0x0031
> > > +struct mgmt_ev_quality_report {
> > > +       __u8    quality_spec;
> > > +       __u32   data_len;
> > > +       __u8    data[];
> > > +} __packed;
> > > diff --git a/net/bluetooth/aosp.c b/net/bluetooth/aosp.c
> > > index 432ae3aac9e3..94faa15b1ea0 100644
> > > --- a/net/bluetooth/aosp.c
> > > +++ b/net/bluetooth/aosp.c
> > > @@ -199,3 +199,53 @@ int aosp_set_quality_report(struct hci_dev *hdev, bool enable)
> > >         else
> > >                 return disable_quality_report(hdev);
> > >  }
> > > +
> > > +/* The following LEN = 1-byte Sub-event code + 48-byte Sub-event Parameters */
> > > +#define BLUETOOTH_QUALITY_REPORT_LEN 49
> > > +
> > > +bool aosp_check_quality_report_len(struct sk_buff *skb)
> > > +{
> > > +       /* skb->len is allowed to be larger than BLUETOOTH_QUALITY_REPORT_LEN
> > > +        * to accommodate an additional Vendor Specific Parameter (vsp) field.
> > > +        */
> > > +       if (skb->len < BLUETOOTH_QUALITY_REPORT_LEN) {
> > > +               BT_ERR("AOSP evt data len %d too short (%u expected)",
> > > +                      skb->len, BLUETOOTH_QUALITY_REPORT_LEN);
> > > +               return false;
> > > +       }
> > > +
> > > +       return true;
> > > +}
> > > +
> > > +/* AOSP HCI Requirements use 0x54 and up as sub-event codes without
> > > + * actually defining a vendor prefix. Refer to
> > > + * https://source.android.com/devices/bluetooth/hci_requirements
> > > + * Hence, the other vendor event prefixes should not use the same
> > > + * space to avoid collision.
> > > + * Since the AOSP does not define a prefix, its prefix is NULL
> > > + * and prefix_len is 0.
> > > + * While there are a number of subcodes in AOSP, only interested in
> > > + * Bluetooth Quality Report (0x58) for now.
> > > + */
> > > +#define AOSP_EV_QUALITY_REPORT         0x58
> > > +
> > > +static unsigned char AOSP_SUBCODES[] = { AOSP_EV_QUALITY_REPORT };
> > > +
> > > +static struct ext_vendor_prefix aosp_ext_prefix = {
> > > +       .prefix         = NULL,
> > > +       .prefix_len     = 0,
> > > +       .subcodes       = AOSP_SUBCODES,
> > > +       .subcodes_len   = sizeof(AOSP_SUBCODES),
> > > +};
> > > +
> > > +struct ext_vendor_prefix *aosp_get_ext_prefix(struct hci_dev *hdev)
> > > +{
> > > +       return &aosp_ext_prefix;
> > > +}
> > > +
> > > +void aosp_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb)
> > > +{
> > > +       if (aosp_has_quality_report(hdev) && aosp_check_quality_report_len(skb))
> > > +               mgmt_quality_report(hdev, skb->data, skb->len,
> > > +                                   QUALITY_SPEC_AOSP_BQR);
> > > +}
> > > diff --git a/net/bluetooth/aosp.h b/net/bluetooth/aosp.h
> > > index 2fd8886d51b2..8208e01fffed 100644
> > > --- a/net/bluetooth/aosp.h
> > > +++ b/net/bluetooth/aosp.h
> > > @@ -10,6 +10,9 @@ void aosp_do_close(struct hci_dev *hdev);
> > >
> > >  bool aosp_has_quality_report(struct hci_dev *hdev);
> > >  int aosp_set_quality_report(struct hci_dev *hdev, bool enable);
> > > +bool aosp_check_quality_report_len(struct sk_buff *skb);
> > > +struct ext_vendor_prefix *aosp_get_ext_prefix(struct hci_dev *hdev);
> > > +void aosp_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb);
> > >
> > >  #else
> > >
> > > @@ -26,4 +29,19 @@ static inline int aosp_set_quality_report(struct hci_dev *hdev, bool enable)
> > >         return -EOPNOTSUPP;
> > >  }
> > >
> > > +static inline bool aosp_check_quality_report_len(struct sk_buff *skb)
> > > +{
> > > +       return false;
> > > +}
> > > +
> > > +static inline struct ext_vendor_prefix *
> > > +aosp_get_ext_prefix(struct hci_dev *hdev)
> > > +{
> > > +       return NULL;
> > > +}
> > > +
> > > +static inline void aosp_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb)
> > > +{
> > > +}
> > > +
> > >  #endif
> > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > > index 0270e597c285..8398971eddf4 100644
> > > --- a/net/bluetooth/hci_event.c
> > > +++ b/net/bluetooth/hci_event.c
> > > @@ -37,6 +37,7 @@
> > >  #include "smp.h"
> > >  #include "msft.h"
> > >  #include "eir.h"
> > > +#include "aosp.h"
> > >
> > >  #define ZERO_KEY "\x00\x00\x00\x00\x00\x00\x00\x00" \
> > >                  "\x00\x00\x00\x00\x00\x00\x00\x00"
> > > @@ -4259,6 +4260,57 @@ static void hci_num_comp_blocks_evt(struct hci_dev *hdev, void *data,
> > >         queue_work(hdev->workqueue, &hdev->tx_work);
> > >  }
> > >
> > > +/* Every distinct vendor specification must have a well-defined vendor
> > > + * event prefix to determine if a vendor event meets the specification.
> > > + * Some vendor prefixes are fixed values while some other vendor prefixes
> > > + * are only available at run time.
> > > + */
> > > +static struct ext_vendor_event_prefix {
> > > +       /* Some vendor prefixes are variable length. For convenience,
> > > +        * the prefix in struct ext_vendor_prefix is in little endian.
> > > +        */
> > > +       struct ext_vendor_prefix *
> > > +               (*get_ext_vendor_prefix)(struct hci_dev *hdev);
> > > +       void (*vendor_func)(struct hci_dev *hdev, struct sk_buff *skb);
> > > +} evt_prefixes[] = {
> > > +       { aosp_get_ext_prefix, aosp_vendor_evt },
> > > +       { msft_get_ext_prefix, msft_vendor_evt },
> > > +
> > > +       /* end with a null entry */
> > > +       {},
> > > +};
> > > +
> > > +static void hci_vendor_evt(struct hci_dev *hdev, void *data,
> > > +                          struct sk_buff *skb)
> > > +{
> > > +       int i, j;
> > > +       struct ext_vendor_prefix *vnd;
> > > +       __u8 subcode;
> > > +
> > > +       for (i = 0; evt_prefixes[i].get_ext_vendor_prefix; i++) {
> > > +               vnd = evt_prefixes[i].get_ext_vendor_prefix(hdev);
> > > +               if (!vnd)
> > > +                       continue;
> > > +
> > > +               /* Compare the raw prefix data in little endian directly. */
> > > +               if (memcmp(vnd->prefix, skb->data, vnd->prefix_len))
> > > +                       continue;
> > > +
> > > +               /* Make sure that there are more data after prefix. */
> > > +               if (skb->len <= vnd->prefix_len)
> > > +                       continue;
> > > +
> > > +               /* The subcode is the single octet following the prefix. */
> > > +               subcode = skb->data[vnd->prefix_len];
> > > +               for (j = 0; j < vnd->subcodes_len; j++) {
> > > +                       if (vnd->subcodes[j] == subcode) {
> > > +                               evt_prefixes[i].vendor_func(hdev, skb);
> > > +                               break;
> > > +                       }
> > > +               }
> > > +       }
> > > +}
> >
> > I recall saying that having such matching logic applied without the
> > driver confirming that is the structure it using to be a bad idea
> > since it could actually cause an event to misinterpret and cause bad
> > behavior, instead we probably need a callback that gets populated by
> > the driver e.g.(hdev->vendor_evt) then the driver can either populate
> > with hci_vendor_evt if it does use prefixes or its own specialized
> > function or NULL if it doesn't use vendor events, specially for old
> > controllers Id leave it as NULL.
> >
> > >  static void hci_mode_change_evt(struct hci_dev *hdev, void *data,
> > >                                 struct sk_buff *skb)
> > >  {
> > > @@ -6879,7 +6931,7 @@ static const struct hci_ev {
> > >         HCI_EV(HCI_EV_NUM_COMP_BLOCKS, hci_num_comp_blocks_evt,
> > >                sizeof(struct hci_ev_num_comp_blocks)),
> > >         /* [0xff = HCI_EV_VENDOR] */
> > > -       HCI_EV_VL(HCI_EV_VENDOR, msft_vendor_evt, 0, HCI_MAX_EVENT_SIZE),
> > > +       HCI_EV_VL(HCI_EV_VENDOR, hci_vendor_evt, 0, HCI_MAX_EVENT_SIZE),
> > >  };
> > >
> > >  static void hci_event_func(struct hci_dev *hdev, u8 event, struct sk_buff *skb,
> > > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> > > index 1ad84f34097f..9d3666bdd07c 100644
> > > --- a/net/bluetooth/mgmt.c
> > > +++ b/net/bluetooth/mgmt.c
> > > @@ -4332,6 +4332,25 @@ static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
> > >                                MGMT_STATUS_NOT_SUPPORTED);
> > >  }
> > >
> > > +int mgmt_quality_report(struct hci_dev *hdev, void *data, u32 data_len,
> > > +                       u8 quality_spec)
> > > +{
> > > +       struct mgmt_ev_quality_report *ev;
> > > +       struct sk_buff *skb;
> > > +
> > > +       skb = mgmt_alloc_skb(hdev, MGMT_EV_QUALITY_REPORT,
> > > +                            sizeof(*ev) + data_len);
> > > +       if (!skb)
> > > +               return -ENOMEM;
> > > +
> > > +       ev = skb_put(skb, sizeof(*ev));
> > > +       ev->quality_spec = quality_spec;
> > > +       ev->data_len = data_len;
> > > +       skb_put_data(skb, data, data_len);
> > > +
> > > +       return mgmt_event_skb(skb, NULL);
> > > +}
> > > +
> > >  static int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
> > >                             u16 data_len)
> > >  {
> > > diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
> > > index f43994523b1f..c003e94faccd 100644
> > > --- a/net/bluetooth/msft.c
> > > +++ b/net/bluetooth/msft.c
> > > @@ -116,6 +116,20 @@ bool msft_monitor_supported(struct hci_dev *hdev)
> > >         return !!(msft_get_features(hdev) & MSFT_FEATURE_MASK_LE_ADV_MONITOR);
> > >  }
> > >
> > > +/* Add the MSFT vendor event subcodes into MSFT_SUBCODES which
> > > + * msft_vendor_evt() is interested in handling.
> > > + */
> > > +static unsigned char MSFT_SUBCODES[] = { MSFT_EV_LE_MONITOR_DEVICE };
> > > +static struct ext_vendor_prefix msft_ext_prefix = { 0 };
> > > +
> > > +static void set_ext_prefix(struct msft_data *msft)
> > > +{
> > > +       msft_ext_prefix.prefix = msft->evt_prefix;
> > > +       msft_ext_prefix.prefix_len = msft->evt_prefix_len;
> > > +       msft_ext_prefix.subcodes = MSFT_SUBCODES;
> > > +       msft_ext_prefix.subcodes_len = sizeof(MSFT_SUBCODES);
> > > +}
> > > +
> > >  static bool read_supported_features(struct hci_dev *hdev,
> > >                                     struct msft_data *msft)
> > >  {
> > > @@ -156,6 +170,8 @@ static bool read_supported_features(struct hci_dev *hdev,
> > >         if (msft->features & MSFT_FEATURE_MASK_CURVE_VALIDITY)
> > >                 hdev->msft_curve_validity = true;
> > >
> > > +       set_ext_prefix(msft);
> > > +
> > >         kfree_skb(skb);
> > >         return true;
> > >
> > > @@ -742,7 +758,17 @@ static void msft_monitor_device_evt(struct hci_dev *hdev, struct sk_buff *skb)
> > >                                  handle_data->mgmt_handle);
> > >  }
> > >
> > > -void msft_vendor_evt(struct hci_dev *hdev, void *data, struct sk_buff *skb)
> > > +struct ext_vendor_prefix *msft_get_ext_prefix(struct hci_dev *hdev)
> > > +{
> > > +       struct msft_data *msft = hdev->msft_data;
> > > +
> > > +       if (!msft)
> > > +               return NULL;
> > > +
> > > +       return &msft_ext_prefix;
> > > +}
> > > +
> > > +void msft_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb)
> > >  {
> > >         struct msft_data *msft = hdev->msft_data;
> > >         u8 *evt_prefix;
> > > diff --git a/net/bluetooth/msft.h b/net/bluetooth/msft.h
> > > index afcaf7d3b1cb..1515ae06c628 100644
> > > --- a/net/bluetooth/msft.h
> > > +++ b/net/bluetooth/msft.h
> > > @@ -17,7 +17,7 @@ void msft_register(struct hci_dev *hdev);
> > >  void msft_unregister(struct hci_dev *hdev);
> > >  void msft_do_open(struct hci_dev *hdev);
> > >  void msft_do_close(struct hci_dev *hdev);
> > > -void msft_vendor_evt(struct hci_dev *hdev, void *data, struct sk_buff *skb);
> > > +void msft_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb);
> > >  __u64 msft_get_features(struct hci_dev *hdev);
> > >  int msft_add_monitor_pattern(struct hci_dev *hdev, struct adv_monitor *monitor);
> > >  int msft_remove_monitor(struct hci_dev *hdev, struct adv_monitor *monitor,
> > > @@ -27,6 +27,7 @@ int msft_set_filter_enable(struct hci_dev *hdev, bool enable);
> > >  int msft_suspend_sync(struct hci_dev *hdev);
> > >  int msft_resume_sync(struct hci_dev *hdev);
> > >  bool msft_curve_validity(struct hci_dev *hdev);
> > > +struct ext_vendor_prefix *msft_get_ext_prefix(struct hci_dev *hdev);
> > >
> > >  #else
> > >
> > > @@ -39,8 +40,7 @@ static inline void msft_register(struct hci_dev *hdev) {}
> > >  static inline void msft_unregister(struct hci_dev *hdev) {}
> > >  static inline void msft_do_open(struct hci_dev *hdev) {}
> > >  static inline void msft_do_close(struct hci_dev *hdev) {}
> > > -static inline void msft_vendor_evt(struct hci_dev *hdev, void *data,
> > > -                                  struct sk_buff *skb) {}
> > > +static inline void msft_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb) {}
> > >  static inline __u64 msft_get_features(struct hci_dev *hdev) { return 0; }
> > >  static inline int msft_add_monitor_pattern(struct hci_dev *hdev,
> > >                                            struct adv_monitor *monitor)
> > > @@ -77,4 +77,10 @@ static inline bool msft_curve_validity(struct hci_dev *hdev)
> > >         return false;
> > >  }
> > >
> > > +static inline struct ext_vendor_prefix *
> > > +msft_get_ext_prefix(struct hci_dev *hdev)
> > > +{
> > > +       return NULL;
> > > +}
> > > +
> > >  #endif
> > > --
> > > 2.36.1.124.g0e6072fb45-goog
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz
>
>
>
> --
>
> Joseph Shyh-In Hwang
> Email: [e-mail]

Luiz Augusto von Dentz May 31, 2022, 10:30 p.m. UTC | #4

Hi Joseph,

On Mon, May 30, 2022 at 2:00 PM Luiz Augusto von Dentz
<[e-mail]> wrote:
>
> Hi Joseph,
>
> On Thu, May 26, 2022 at 9:37 PM Joseph Hwang <[e-mail]> wrote:
> >
> > Hi Luiz:
> >
> > Thanks for your review! The get_ext_vendor_prefix() in the table
> > provides a *unique* extended vendor prefix ( = vendor prefix + 1-octet
> > subcode) that can uniquely identify a vendor event. I am not aware of
> > any situation that might cause an event to be incorrectly matched with
> > an extended vendor prefix. Maybe I am missing something?
> >
> > On the other hand, in your comment, to let a driver confirm whether it
> > is the vendor event structure it uses might be a bit risky. For
> > example, assume that we pass a vendor event to
> > msft.c:msft_vendor_evt() to determine whether it is a MSFT event. The
> > current implementation of msft_vendor_evt() is to call skb_pull_data()
> > to pull the event prefix for comparison with the dynamic MSFT event
> > prefix. No matter whether the event matches or not, the event skb has
> > been modified already and would cause bad behavior if we pass the
> > event skb to other vendor drivers/functions. How can we generally make
> > sure that every such vendor drivers/functions are implemented in a
> > read-only way that does not modify the skb when comparing the prefix?
> > In this patch, we propose to use get_ext_vendor_prefix() which is
> > guaranteed not to modify the skb in any possible way.
> >
> > Please also note that the mechanism here also takes care of older
> > controllers that might not support some of the vendor specifications.
> > For example, if an older controller does not support the MSFT spec,
> > the msft_get_ext_prefix() would return NULL as its prefix. And hence a
> > vendor event would not accidentally match the MSFT spec on the older
> > controller. Similarly, in the following patch “btintel: setup
> > vendor_get_prefix and vendor_evt”, on an older Intel controller that
> > does not support Intel telemetry events, the btintel driver would
> > *not* set up
> >
> >     hdev->vendor_get_ext_prefix = btintel_get_ext_prefix;
>
> I see, while this does indeed prevent events to be misinterpreted,
> this locks us on only supporting vendor commands which use vendor
> prefixes, but perhaps that is fine since I assume there is probably no
> better way to create vendor opcodes in the first place.
>
> > such that an event would not match as an Intel vendor event in any way.
> >
> > Please let me know if I have any misunderstanding.
> >
> > Thanks and regards,
> > Joseph
> >
> >
> > On Fri, May 27, 2022 at 4:25 AM Luiz Augusto von Dentz
> > <[e-mail]> wrote:
> > >
> > > Hi Joseph,
> > >
> > > On Thu, May 26, 2022 at 4:21 AM Joseph Hwang <[e-mail]> wrote:
> > > >
> > > > When receiving a HCI vendor event, the kernel checks if it is an
> > > > AOSP bluetooth quality report. If yes, the event is sent to bluez
> > > > user space through the mgmt socket.
> > > >
> > > > Reported-by: kernel test robot <[e-mail]>
> > > >
> > > > Signed-off-by: Joseph Hwang <[e-mail]>
> > > > Reviewed-by: Archie Pusaka <[e-mail]>
> > > > ---
> > > >
> > > > Changes in v6:
> > > > - Fixed a sparse check warning about using static for evt_prefixes.
> > > >
> > > > Changes in v5:
> > > > - Define "struct ext_vendor_prefix" to replace "struct vendor_prefix"
> > > >   so that extended vendor prefix = prefix + 1-octet subcode
> > > > - Define aosp_ext_prefix to provide AOSP extended prefix which is
> > > >   returned by aosp_get_ext_prefix().
> > > > - Redefine struct ext_vendor_event_prefix such that
> > > >   . it uses get_ext_vendor_prefix to get prefix and subcodes where
> > > >     the prefix and the prefix length may be variable and are not
> > > >     unknown until run time;
> > > >   . it uses vendor_func to handle a vendor event
> > > >   This table handles vendor events in a generic way.
> > > > - Rewrite hci_vendor_evt() so that it compares both vendor prefix
> > > >   and subcode to match a vendor event.
> > > > - Define set_ext_prefix() to create MSFT extended vendor prefix
> > > >   which is returned by msft_get_ext_prefix().
> > > > - Do not EXPORT_SYMBOL(mgmt_quality_report).
> > > > - Keep msft_get_ext_prefix in msft instead of hci_dev since it is
> > > >   not used by any drivers.
> > > >
> > > > Changes in v3:
> > > > - Rebase to resolve the code conflict.
> > > > - Move aosp_quality_report_evt() from hci_event.c to aosp.c.
> > > > - A new patch (3/3) is added to enable the quality report feature.
> > > >
> > > > Changes in v2:
> > > > - Scrap the two structures defined in aosp.c and use constants for
> > > >   size check.
> > > > - Do a basic size check about the quality report event. Do not pull
> > > >   data from the event in which the kernel has no interest.
> > > > - Define vendor event prefixes with which vendor events of distinct
> > > >   vendor specifications can be clearly differentiated.
> > > > - Use mgmt helpers to add the header and data to a mgmt skb.
> > > >
> > > >  include/net/bluetooth/hci_core.h | 12 +++++++
> > > >  include/net/bluetooth/mgmt.h     |  7 +++++
> > > >  net/bluetooth/aosp.c             | 50 +++++++++++++++++++++++++++++
> > > >  net/bluetooth/aosp.h             | 18 +++++++++++
> > > >  net/bluetooth/hci_event.c        | 54 +++++++++++++++++++++++++++++++-
> > > >  net/bluetooth/mgmt.c             | 19 +++++++++++
> > > >  net/bluetooth/msft.c             | 28 ++++++++++++++++-
> > > >  net/bluetooth/msft.h             | 12 +++++--
> > > >  8 files changed, 195 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > > > index 64d3a63759a8..f89738c6b973 100644
> > > > --- a/include/net/bluetooth/hci_core.h
> > > > +++ b/include/net/bluetooth/hci_core.h
> > > > @@ -328,6 +328,13 @@ struct amp_assoc {
> > > >
> > > >  #define HCI_MAX_PAGES  3
> > > >
> > > > +struct ext_vendor_prefix {
> > > > +       __u8 *prefix;
> > > > +       __u8 prefix_len;
> > > > +       __u8 *subcodes;
> > > > +       __u8 subcodes_len;
> > > > +};
> > > > +
> > > >  struct hci_dev {
> > > >         struct list_head list;
> > > >         struct mutex    lock;
> > > > @@ -1876,6 +1883,8 @@ int mgmt_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status);
> > > >  int mgmt_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status);
> > > >  void mgmt_adv_monitor_device_lost(struct hci_dev *hdev, u16 handle,
> > > >                                   bdaddr_t *bdaddr, u8 addr_type);
> > > > +int mgmt_quality_report(struct hci_dev *hdev, void *data, u32 data_len,
> > > > +                       u8 quality_spec);
> > > >
> > > >  u8 hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max, u16 latency,
> > > >                       u16 to_multiplier);
> > > > @@ -1894,4 +1903,7 @@ void hci_copy_identity_address(struct hci_dev *hdev, bdaddr_t *bdaddr,
> > > >
> > > >  #define TRANSPORT_TYPE_MAX     0x04
> > > >
> > > > +#define QUALITY_SPEC_AOSP_BQR          0x0
> > > > +#define QUALITY_SPEC_INTEL_TELEMETRY   0x1
> > > > +
> > > >  #endif /* __HCI_CORE_H */
> > > > diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> > > > index c1c2fd72d9e3..6ccd0067c295 100644
> > > > --- a/include/net/bluetooth/mgmt.h
> > > > +++ b/include/net/bluetooth/mgmt.h
> > > > @@ -1127,3 +1127,10 @@ struct mgmt_ev_adv_monitor_device_lost {
> > > >         __le16 monitor_handle;
> > > >         struct mgmt_addr_info addr;
> > > >  } __packed;
> > > > +
> > > > +#define MGMT_EV_QUALITY_REPORT                 0x0031
> > > > +struct mgmt_ev_quality_report {
> > > > +       __u8    quality_spec;
> > > > +       __u32   data_len;
> > > > +       __u8    data[];
> > > > +} __packed;
> > > > diff --git a/net/bluetooth/aosp.c b/net/bluetooth/aosp.c
> > > > index 432ae3aac9e3..94faa15b1ea0 100644
> > > > --- a/net/bluetooth/aosp.c
> > > > +++ b/net/bluetooth/aosp.c
> > > > @@ -199,3 +199,53 @@ int aosp_set_quality_report(struct hci_dev *hdev, bool enable)
> > > >         else
> > > >                 return disable_quality_report(hdev);
> > > >  }
> > > > +
> > > > +/* The following LEN = 1-byte Sub-event code + 48-byte Sub-event Parameters */
> > > > +#define BLUETOOTH_QUALITY_REPORT_LEN 49
> > > > +
> > > > +bool aosp_check_quality_report_len(struct sk_buff *skb)
> > > > +{
> > > > +       /* skb->len is allowed to be larger than BLUETOOTH_QUALITY_REPORT_LEN
> > > > +        * to accommodate an additional Vendor Specific Parameter (vsp) field.
> > > > +        */
> > > > +       if (skb->len < BLUETOOTH_QUALITY_REPORT_LEN) {
> > > > +               BT_ERR("AOSP evt data len %d too short (%u expected)",
> > > > +                      skb->len, BLUETOOTH_QUALITY_REPORT_LEN);
> > > > +               return false;
> > > > +       }
> > > > +
> > > > +       return true;
> > > > +}
> > > > +
> > > > +/* AOSP HCI Requirements use 0x54 and up as sub-event codes without
> > > > + * actually defining a vendor prefix. Refer to
> > > > + * https://source.android.com/devices/bluetooth/hci_requirements
> > > > + * Hence, the other vendor event prefixes should not use the same
> > > > + * space to avoid collision.
> > > > + * Since the AOSP does not define a prefix, its prefix is NULL
> > > > + * and prefix_len is 0.
> > > > + * While there are a number of subcodes in AOSP, only interested in
> > > > + * Bluetooth Quality Report (0x58) for now.
> > > > + */
> > > > +#define AOSP_EV_QUALITY_REPORT         0x58
> > > > +
> > > > +static unsigned char AOSP_SUBCODES[] = { AOSP_EV_QUALITY_REPORT };
> > > > +
> > > > +static struct ext_vendor_prefix aosp_ext_prefix = {
> > > > +       .prefix         = NULL,
> > > > +       .prefix_len     = 0,
> > > > +       .subcodes       = AOSP_SUBCODES,
> > > > +       .subcodes_len   = sizeof(AOSP_SUBCODES),
> > > > +};
> > > > +
> > > > +struct ext_vendor_prefix *aosp_get_ext_prefix(struct hci_dev *hdev)
> > > > +{
> > > > +       return &aosp_ext_prefix;
> > > > +}
> > > > +
> > > > +void aosp_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb)
> > > > +{
> > > > +       if (aosp_has_quality_report(hdev) && aosp_check_quality_report_len(skb))
> > > > +               mgmt_quality_report(hdev, skb->data, skb->len,
> > > > +                                   QUALITY_SPEC_AOSP_BQR);
> > > > +}
> > > > diff --git a/net/bluetooth/aosp.h b/net/bluetooth/aosp.h
> > > > index 2fd8886d51b2..8208e01fffed 100644
> > > > --- a/net/bluetooth/aosp.h
> > > > +++ b/net/bluetooth/aosp.h
> > > > @@ -10,6 +10,9 @@ void aosp_do_close(struct hci_dev *hdev);
> > > >
> > > >  bool aosp_has_quality_report(struct hci_dev *hdev);
> > > >  int aosp_set_quality_report(struct hci_dev *hdev, bool enable);
> > > > +bool aosp_check_quality_report_len(struct sk_buff *skb);
> > > > +struct ext_vendor_prefix *aosp_get_ext_prefix(struct hci_dev *hdev);
> > > > +void aosp_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb);
> > > >
> > > >  #else
> > > >
> > > > @@ -26,4 +29,19 @@ static inline int aosp_set_quality_report(struct hci_dev *hdev, bool enable)
> > > >         return -EOPNOTSUPP;
> > > >  }
> > > >
> > > > +static inline bool aosp_check_quality_report_len(struct sk_buff *skb)
> > > > +{
> > > > +       return false;
> > > > +}
> > > > +
> > > > +static inline struct ext_vendor_prefix *
> > > > +aosp_get_ext_prefix(struct hci_dev *hdev)
> > > > +{
> > > > +       return NULL;
> > > > +}
> > > > +
> > > > +static inline void aosp_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb)
> > > > +{
> > > > +}
> > > > +
> > > >  #endif
> > > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > > > index 0270e597c285..8398971eddf4 100644
> > > > --- a/net/bluetooth/hci_event.c
> > > > +++ b/net/bluetooth/hci_event.c
> > > > @@ -37,6 +37,7 @@
> > > >  #include "smp.h"
> > > >  #include "msft.h"
> > > >  #include "eir.h"
> > > > +#include "aosp.h"
> > > >
> > > >  #define ZERO_KEY "\x00\x00\x00\x00\x00\x00\x00\x00" \
> > > >                  "\x00\x00\x00\x00\x00\x00\x00\x00"
> > > > @@ -4259,6 +4260,57 @@ static void hci_num_comp_blocks_evt(struct hci_dev *hdev, void *data,
> > > >         queue_work(hdev->workqueue, &hdev->tx_work);
> > > >  }
> > > >
> > > > +/* Every distinct vendor specification must have a well-defined vendor
> > > > + * event prefix to determine if a vendor event meets the specification.
> > > > + * Some vendor prefixes are fixed values while some other vendor prefixes
> > > > + * are only available at run time.
> > > > + */
> > > > +static struct ext_vendor_event_prefix {
> > > > +       /* Some vendor prefixes are variable length. For convenience,
> > > > +        * the prefix in struct ext_vendor_prefix is in little endian.
> > > > +        */
> > > > +       struct ext_vendor_prefix *
> > > > +               (*get_ext_vendor_prefix)(struct hci_dev *hdev);
> > > > +       void (*vendor_func)(struct hci_dev *hdev, struct sk_buff *skb);
> > > > +} evt_prefixes[] = {
> > > > +       { aosp_get_ext_prefix, aosp_vendor_evt },
> > > > +       { msft_get_ext_prefix, msft_vendor_evt },
> > > > +
> > > > +       /* end with a null entry */
> > > > +       {},
> > > > +};
> > > > +
> > > > +static void hci_vendor_evt(struct hci_dev *hdev, void *data,
> > > > +                          struct sk_buff *skb)
> > > > +{
> > > > +       int i, j;
> > > > +       struct ext_vendor_prefix *vnd;
> > > > +       __u8 subcode;
> > > > +
> > > > +       for (i = 0; evt_prefixes[i].get_ext_vendor_prefix; i++) {
> > > > +               vnd = evt_prefixes[i].get_ext_vendor_prefix(hdev);
> > > > +               if (!vnd)
> > > > +                       continue;

We need to check like if (skb->len < vnd->prefix_len), btw are the
prefixes supposed to be changed at runtime? If not probably a const
table would work better. In fact I would suggest changing the
vendor_func to start parsing _after_ the prefix so you can use
skb_pull_data, or create a helper that does that conditionally e.g
skb_pull_data_cmp(skb, data, data_len).

> > > > +               /* Compare the raw prefix data in little endian directly. */
> > > > +               if (memcmp(vnd->prefix, skb->data, vnd->prefix_len))
> > > > +                       continue;
> > > > +
> > > > +               /* Make sure that there are more data after prefix. */
> > > > +               if (skb->len <= vnd->prefix_len)
> > > > +                       continue;
> > > > +
> > > > +               /* The subcode is the single octet following the prefix. */
> > > > +               subcode = skb->data[vnd->prefix_len];

This part I don't really get, so the core is doing subcode matching,
what if the vendor decides that the subcode is part of the prefix?
Prefix matching is one thing but prefix + subcode is another pattern,
or is this to allow 0 prefix_len so it essentially becomes subcode
matching? Anyway I was thinking this could all be work out in a single
const table e.g:

#define data(args...) ((const unsigned char[]) { args })

#define HCI_VENDOR_EVT(_func, _prefix_len, _prefix...)
{ \
    .func = _func,
    .prefix_len = sizeof(data(_prefix)),
    .prefix = data(_prefix),
}

const static struct hci_vendor_evt {
    (*func)...
    u16 prefix_len;
    u8 prefix[]; /* Prefix + subcode */
} = vendor_evt_table[] = {
    HCI_VENDOR_EVT(msft_monitor_device_evt, sizeof(msft_device_evt),
msft_prefix, MSFT_EV_LE_MONITOR_DEVICE),
};

So it becomes pretty similar to the existing const tables we have in
hci_event.c with the only difference is that we use prefix matching
where the length is not constant among each element, the only drawback
that I can think of is that all vendor events will need to be added to
the table (msftp, aosp, etc.) but perhaps that makes it more visible
on exactly what events are supported instead of every subcode being
handled automatically which is not always the case, the table(s) would
sit in the driver code and would be registered to hdev all at once
instead of setting each prefix separately at runtime this all happens
at build time so the compiler can do its job while compiling it.

> > > > +               for (j = 0; j < vnd->subcodes_len; j++) {
> > > > +                       if (vnd->subcodes[j] == subcode) {
> > > > +                               evt_prefixes[i].vendor_func(hdev, skb);
> > > > +                               break;
> > > > +                       }
> > > > +               }
> > > > +       }
> > > > +}
> > >
> > > I recall saying that having such matching logic applied without the
> > > driver confirming that is the structure it using to be a bad idea
> > > since it could actually cause an event to misinterpret and cause bad
> > > behavior, instead we probably need a callback that gets populated by
> > > the driver e.g.(hdev->vendor_evt) then the driver can either populate
> > > with hci_vendor_evt if it does use prefixes or its own specialized
> > > function or NULL if it doesn't use vendor events, specially for old
> > > controllers Id leave it as NULL.
> > >
> > > >  static void hci_mode_change_evt(struct hci_dev *hdev, void *data,
> > > >                                 struct sk_buff *skb)
> > > >  {
> > > > @@ -6879,7 +6931,7 @@ static const struct hci_ev {
> > > >         HCI_EV(HCI_EV_NUM_COMP_BLOCKS, hci_num_comp_blocks_evt,
> > > >                sizeof(struct hci_ev_num_comp_blocks)),
> > > >         /* [0xff = HCI_EV_VENDOR] */
> > > > -       HCI_EV_VL(HCI_EV_VENDOR, msft_vendor_evt, 0, HCI_MAX_EVENT_SIZE),
> > > > +       HCI_EV_VL(HCI_EV_VENDOR, hci_vendor_evt, 0, HCI_MAX_EVENT_SIZE),
> > > >  };
> > > >
> > > >  static void hci_event_func(struct hci_dev *hdev, u8 event, struct sk_buff *skb,
> > > > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> > > > index 1ad84f34097f..9d3666bdd07c 100644
> > > > --- a/net/bluetooth/mgmt.c
> > > > +++ b/net/bluetooth/mgmt.c
> > > > @@ -4332,6 +4332,25 @@ static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
> > > >                                MGMT_STATUS_NOT_SUPPORTED);
> > > >  }
> > > >
> > > > +int mgmt_quality_report(struct hci_dev *hdev, void *data, u32 data_len,
> > > > +                       u8 quality_spec)
> > > > +{
> > > > +       struct mgmt_ev_quality_report *ev;
> > > > +       struct sk_buff *skb;
> > > > +
> > > > +       skb = mgmt_alloc_skb(hdev, MGMT_EV_QUALITY_REPORT,
> > > > +                            sizeof(*ev) + data_len);
> > > > +       if (!skb)
> > > > +               return -ENOMEM;
> > > > +
> > > > +       ev = skb_put(skb, sizeof(*ev));
> > > > +       ev->quality_spec = quality_spec;
> > > > +       ev->data_len = data_len;
> > > > +       skb_put_data(skb, data, data_len);
> > > > +
> > > > +       return mgmt_event_skb(skb, NULL);
> > > > +}
> > > > +
> > > >  static int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
> > > >                             u16 data_len)
> > > >  {
> > > > diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
> > > > index f43994523b1f..c003e94faccd 100644
> > > > --- a/net/bluetooth/msft.c
> > > > +++ b/net/bluetooth/msft.c
> > > > @@ -116,6 +116,20 @@ bool msft_monitor_supported(struct hci_dev *hdev)
> > > >         return !!(msft_get_features(hdev) & MSFT_FEATURE_MASK_LE_ADV_MONITOR);
> > > >  }
> > > >
> > > > +/* Add the MSFT vendor event subcodes into MSFT_SUBCODES which
> > > > + * msft_vendor_evt() is interested in handling.
> > > > + */
> > > > +static unsigned char MSFT_SUBCODES[] = { MSFT_EV_LE_MONITOR_DEVICE };
> > > > +static struct ext_vendor_prefix msft_ext_prefix = { 0 };
> > > > +
> > > > +static void set_ext_prefix(struct msft_data *msft)
> > > > +{
> > > > +       msft_ext_prefix.prefix = msft->evt_prefix;
> > > > +       msft_ext_prefix.prefix_len = msft->evt_prefix_len;
> > > > +       msft_ext_prefix.subcodes = MSFT_SUBCODES;
> > > > +       msft_ext_prefix.subcodes_len = sizeof(MSFT_SUBCODES);
> > > > +}
> > > > +
> > > >  static bool read_supported_features(struct hci_dev *hdev,
> > > >                                     struct msft_data *msft)
> > > >  {
> > > > @@ -156,6 +170,8 @@ static bool read_supported_features(struct hci_dev *hdev,
> > > >         if (msft->features & MSFT_FEATURE_MASK_CURVE_VALIDITY)
> > > >                 hdev->msft_curve_validity = true;
> > > >
> > > > +       set_ext_prefix(msft);
> > > > +
> > > >         kfree_skb(skb);
> > > >         return true;
> > > >
> > > > @@ -742,7 +758,17 @@ static void msft_monitor_device_evt(struct hci_dev *hdev, struct sk_buff *skb)
> > > >                                  handle_data->mgmt_handle);
> > > >  }
> > > >
> > > > -void msft_vendor_evt(struct hci_dev *hdev, void *data, struct sk_buff *skb)
> > > > +struct ext_vendor_prefix *msft_get_ext_prefix(struct hci_dev *hdev)
> > > > +{
> > > > +       struct msft_data *msft = hdev->msft_data;
> > > > +
> > > > +       if (!msft)
> > > > +               return NULL;
> > > > +
> > > > +       return &msft_ext_prefix;
> > > > +}
> > > > +
> > > > +void msft_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb)
> > > >  {
> > > >         struct msft_data *msft = hdev->msft_data;
> > > >         u8 *evt_prefix;
> > > > diff --git a/net/bluetooth/msft.h b/net/bluetooth/msft.h
> > > > index afcaf7d3b1cb..1515ae06c628 100644
> > > > --- a/net/bluetooth/msft.h
> > > > +++ b/net/bluetooth/msft.h
> > > > @@ -17,7 +17,7 @@ void msft_register(struct hci_dev *hdev);
> > > >  void msft_unregister(struct hci_dev *hdev);
> > > >  void msft_do_open(struct hci_dev *hdev);
> > > >  void msft_do_close(struct hci_dev *hdev);
> > > > -void msft_vendor_evt(struct hci_dev *hdev, void *data, struct sk_buff *skb);
> > > > +void msft_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb);
> > > >  __u64 msft_get_features(struct hci_dev *hdev);
> > > >  int msft_add_monitor_pattern(struct hci_dev *hdev, struct adv_monitor *monitor);
> > > >  int msft_remove_monitor(struct hci_dev *hdev, struct adv_monitor *monitor,
> > > > @@ -27,6 +27,7 @@ int msft_set_filter_enable(struct hci_dev *hdev, bool enable);
> > > >  int msft_suspend_sync(struct hci_dev *hdev);
> > > >  int msft_resume_sync(struct hci_dev *hdev);
> > > >  bool msft_curve_validity(struct hci_dev *hdev);
> > > > +struct ext_vendor_prefix *msft_get_ext_prefix(struct hci_dev *hdev);
> > > >
> > > >  #else
> > > >
> > > > @@ -39,8 +40,7 @@ static inline void msft_register(struct hci_dev *hdev) {}
> > > >  static inline void msft_unregister(struct hci_dev *hdev) {}
> > > >  static inline void msft_do_open(struct hci_dev *hdev) {}
> > > >  static inline void msft_do_close(struct hci_dev *hdev) {}
> > > > -static inline void msft_vendor_evt(struct hci_dev *hdev, void *data,
> > > > -                                  struct sk_buff *skb) {}
> > > > +static inline void msft_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb) {}
> > > >  static inline __u64 msft_get_features(struct hci_dev *hdev) { return 0; }
> > > >  static inline int msft_add_monitor_pattern(struct hci_dev *hdev,
> > > >                                            struct adv_monitor *monitor)
> > > > @@ -77,4 +77,10 @@ static inline bool msft_curve_validity(struct hci_dev *hdev)
> > > >         return false;
> > > >  }
> > > >
> > > > +static inline struct ext_vendor_prefix *
> > > > +msft_get_ext_prefix(struct hci_dev *hdev)
> > > > +{
> > > > +       return NULL;
> > > > +}
> > > > +
> > > >  #endif
> > > > --
> > > > 2.36.1.124.g0e6072fb45-goog
> > > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
> >
> >
> >
> > --
> >
> > Joseph Shyh-In Hwang
> > Email: [e-mail]
>
>
>
> --
> Luiz Augusto von Dentz

Joseph Hwang June 1, 2022, 2:46 a.m. UTC | #5

Hi Luiz:

  Thank you for your comments. Please read my replies in the lines
below. Thanks!

On Wed, Jun 1, 2022 at 6:31 AM Luiz Augusto von Dentz
<[e-mail]> wrote:
>
> Hi Joseph,
>
> On Mon, May 30, 2022 at 2:00 PM Luiz Augusto von Dentz
> <[e-mail]> wrote:
> >
> > Hi Joseph,
> >
> > On Thu, May 26, 2022 at 9:37 PM Joseph Hwang <[e-mail]> wrote:
> > >
> > > Hi Luiz:
> > >
> > > Thanks for your review! The get_ext_vendor_prefix() in the table
> > > provides a *unique* extended vendor prefix ( = vendor prefix + 1-octet
> > > subcode) that can uniquely identify a vendor event. I am not aware of
> > > any situation that might cause an event to be incorrectly matched with
> > > an extended vendor prefix. Maybe I am missing something?
> > >
> > > On the other hand, in your comment, to let a driver confirm whether it
> > > is the vendor event structure it uses might be a bit risky. For
> > > example, assume that we pass a vendor event to
> > > msft.c:msft_vendor_evt() to determine whether it is a MSFT event. The
> > > current implementation of msft_vendor_evt() is to call skb_pull_data()
> > > to pull the event prefix for comparison with the dynamic MSFT event
> > > prefix. No matter whether the event matches or not, the event skb has
> > > been modified already and would cause bad behavior if we pass the
> > > event skb to other vendor drivers/functions. How can we generally make
> > > sure that every such vendor drivers/functions are implemented in a
> > > read-only way that does not modify the skb when comparing the prefix?
> > > In this patch, we propose to use get_ext_vendor_prefix() which is
> > > guaranteed not to modify the skb in any possible way.
> > >
> > > Please also note that the mechanism here also takes care of older
> > > controllers that might not support some of the vendor specifications.
> > > For example, if an older controller does not support the MSFT spec,
> > > the msft_get_ext_prefix() would return NULL as its prefix. And hence a
> > > vendor event would not accidentally match the MSFT spec on the older
> > > controller. Similarly, in the following patch “btintel: setup
> > > vendor_get_prefix and vendor_evt”, on an older Intel controller that
> > > does not support Intel telemetry events, the btintel driver would
> > > *not* set up
> > >
> > >     hdev->vendor_get_ext_prefix = btintel_get_ext_prefix;
> >
> > I see, while this does indeed prevent events to be misinterpreted,
> > this locks us on only supporting vendor commands which use vendor
> > prefixes, but perhaps that is fine since I assume there is probably no
> > better way to create vendor opcodes in the first place.
> >
> > > such that an event would not match as an Intel vendor event in any way.
> > >
> > > Please let me know if I have any misunderstanding.
> > >
> > > Thanks and regards,
> > > Joseph
> > >
> > >
> > > On Fri, May 27, 2022 at 4:25 AM Luiz Augusto von Dentz
> > > <[e-mail]> wrote:
> > > >
> > > > Hi Joseph,
> > > >
> > > > On Thu, May 26, 2022 at 4:21 AM Joseph Hwang <[e-mail]> wrote:
> > > > >
> > > > > When receiving a HCI vendor event, the kernel checks if it is an
> > > > > AOSP bluetooth quality report. If yes, the event is sent to bluez
> > > > > user space through the mgmt socket.
> > > > >
> > > > > Reported-by: kernel test robot <[e-mail]>
> > > > >
> > > > > Signed-off-by: Joseph Hwang <[e-mail]>
> > > > > Reviewed-by: Archie Pusaka <[e-mail]>
> > > > > ---
> > > > >
> > > > > Changes in v6:
> > > > > - Fixed a sparse check warning about using static for evt_prefixes.
> > > > >
> > > > > Changes in v5:
> > > > > - Define "struct ext_vendor_prefix" to replace "struct vendor_prefix"
> > > > >   so that extended vendor prefix = prefix + 1-octet subcode
> > > > > - Define aosp_ext_prefix to provide AOSP extended prefix which is
> > > > >   returned by aosp_get_ext_prefix().
> > > > > - Redefine struct ext_vendor_event_prefix such that
> > > > >   . it uses get_ext_vendor_prefix to get prefix and subcodes where
> > > > >     the prefix and the prefix length may be variable and are not
> > > > >     unknown until run time;
> > > > >   . it uses vendor_func to handle a vendor event
> > > > >   This table handles vendor events in a generic way.
> > > > > - Rewrite hci_vendor_evt() so that it compares both vendor prefix
> > > > >   and subcode to match a vendor event.
> > > > > - Define set_ext_prefix() to create MSFT extended vendor prefix
> > > > >   which is returned by msft_get_ext_prefix().
> > > > > - Do not EXPORT_SYMBOL(mgmt_quality_report).
> > > > > - Keep msft_get_ext_prefix in msft instead of hci_dev since it is
> > > > >   not used by any drivers.
> > > > >
> > > > > Changes in v3:
> > > > > - Rebase to resolve the code conflict.
> > > > > - Move aosp_quality_report_evt() from hci_event.c to aosp.c.
> > > > > - A new patch (3/3) is added to enable the quality report feature.
> > > > >
> > > > > Changes in v2:
> > > > > - Scrap the two structures defined in aosp.c and use constants for
> > > > >   size check.
> > > > > - Do a basic size check about the quality report event. Do not pull
> > > > >   data from the event in which the kernel has no interest.
> > > > > - Define vendor event prefixes with which vendor events of distinct
> > > > >   vendor specifications can be clearly differentiated.
> > > > > - Use mgmt helpers to add the header and data to a mgmt skb.
> > > > >
> > > > >  include/net/bluetooth/hci_core.h | 12 +++++++
> > > > >  include/net/bluetooth/mgmt.h     |  7 +++++
> > > > >  net/bluetooth/aosp.c             | 50 +++++++++++++++++++++++++++++
> > > > >  net/bluetooth/aosp.h             | 18 +++++++++++
> > > > >  net/bluetooth/hci_event.c        | 54 +++++++++++++++++++++++++++++++-
> > > > >  net/bluetooth/mgmt.c             | 19 +++++++++++
> > > > >  net/bluetooth/msft.c             | 28 ++++++++++++++++-
> > > > >  net/bluetooth/msft.h             | 12 +++++--
> > > > >  8 files changed, 195 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > > > > index 64d3a63759a8..f89738c6b973 100644
> > > > > --- a/include/net/bluetooth/hci_core.h
> > > > > +++ b/include/net/bluetooth/hci_core.h
> > > > > @@ -328,6 +328,13 @@ struct amp_assoc {
> > > > >
> > > > >  #define HCI_MAX_PAGES  3
> > > > >
> > > > > +struct ext_vendor_prefix {
> > > > > +       __u8 *prefix;
> > > > > +       __u8 prefix_len;
> > > > > +       __u8 *subcodes;
> > > > > +       __u8 subcodes_len;
> > > > > +};
> > > > > +
> > > > >  struct hci_dev {
> > > > >         struct list_head list;
> > > > >         struct mutex    lock;
> > > > > @@ -1876,6 +1883,8 @@ int mgmt_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status);
> > > > >  int mgmt_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status);
> > > > >  void mgmt_adv_monitor_device_lost(struct hci_dev *hdev, u16 handle,
> > > > >                                   bdaddr_t *bdaddr, u8 addr_type);
> > > > > +int mgmt_quality_report(struct hci_dev *hdev, void *data, u32 data_len,
> > > > > +                       u8 quality_spec);
> > > > >
> > > > >  u8 hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max, u16 latency,
> > > > >                       u16 to_multiplier);
> > > > > @@ -1894,4 +1903,7 @@ void hci_copy_identity_address(struct hci_dev *hdev, bdaddr_t *bdaddr,
> > > > >
> > > > >  #define TRANSPORT_TYPE_MAX     0x04
> > > > >
> > > > > +#define QUALITY_SPEC_AOSP_BQR          0x0
> > > > > +#define QUALITY_SPEC_INTEL_TELEMETRY   0x1
> > > > > +
> > > > >  #endif /* __HCI_CORE_H */
> > > > > diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> > > > > index c1c2fd72d9e3..6ccd0067c295 100644
> > > > > --- a/include/net/bluetooth/mgmt.h
> > > > > +++ b/include/net/bluetooth/mgmt.h
> > > > > @@ -1127,3 +1127,10 @@ struct mgmt_ev_adv_monitor_device_lost {
> > > > >         __le16 monitor_handle;
> > > > >         struct mgmt_addr_info addr;
> > > > >  } __packed;
> > > > > +
> > > > > +#define MGMT_EV_QUALITY_REPORT                 0x0031
> > > > > +struct mgmt_ev_quality_report {
> > > > > +       __u8    quality_spec;
> > > > > +       __u32   data_len;
> > > > > +       __u8    data[];
> > > > > +} __packed;
> > > > > diff --git a/net/bluetooth/aosp.c b/net/bluetooth/aosp.c
> > > > > index 432ae3aac9e3..94faa15b1ea0 100644
> > > > > --- a/net/bluetooth/aosp.c
> > > > > +++ b/net/bluetooth/aosp.c
> > > > > @@ -199,3 +199,53 @@ int aosp_set_quality_report(struct hci_dev *hdev, bool enable)
> > > > >         else
> > > > >                 return disable_quality_report(hdev);
> > > > >  }
> > > > > +
> > > > > +/* The following LEN = 1-byte Sub-event code + 48-byte Sub-event Parameters */
> > > > > +#define BLUETOOTH_QUALITY_REPORT_LEN 49
> > > > > +
> > > > > +bool aosp_check_quality_report_len(struct sk_buff *skb)
> > > > > +{
> > > > > +       /* skb->len is allowed to be larger than BLUETOOTH_QUALITY_REPORT_LEN
> > > > > +        * to accommodate an additional Vendor Specific Parameter (vsp) field.
> > > > > +        */
> > > > > +       if (skb->len < BLUETOOTH_QUALITY_REPORT_LEN) {
> > > > > +               BT_ERR("AOSP evt data len %d too short (%u expected)",
> > > > > +                      skb->len, BLUETOOTH_QUALITY_REPORT_LEN);
> > > > > +               return false;
> > > > > +       }
> > > > > +
> > > > > +       return true;
> > > > > +}
> > > > > +
> > > > > +/* AOSP HCI Requirements use 0x54 and up as sub-event codes without
> > > > > + * actually defining a vendor prefix. Refer to
> > > > > + * https://source.android.com/devices/bluetooth/hci_requirements
> > > > > + * Hence, the other vendor event prefixes should not use the same
> > > > > + * space to avoid collision.
> > > > > + * Since the AOSP does not define a prefix, its prefix is NULL
> > > > > + * and prefix_len is 0.
> > > > > + * While there are a number of subcodes in AOSP, only interested in
> > > > > + * Bluetooth Quality Report (0x58) for now.
> > > > > + */
> > > > > +#define AOSP_EV_QUALITY_REPORT         0x58
> > > > > +
> > > > > +static unsigned char AOSP_SUBCODES[] = { AOSP_EV_QUALITY_REPORT };
> > > > > +
> > > > > +static struct ext_vendor_prefix aosp_ext_prefix = {
> > > > > +       .prefix         = NULL,
> > > > > +       .prefix_len     = 0,
> > > > > +       .subcodes       = AOSP_SUBCODES,
> > > > > +       .subcodes_len   = sizeof(AOSP_SUBCODES),
> > > > > +};
> > > > > +
> > > > > +struct ext_vendor_prefix *aosp_get_ext_prefix(struct hci_dev *hdev)
> > > > > +{
> > > > > +       return &aosp_ext_prefix;
> > > > > +}
> > > > > +
> > > > > +void aosp_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb)
> > > > > +{
> > > > > +       if (aosp_has_quality_report(hdev) && aosp_check_quality_report_len(skb))
> > > > > +               mgmt_quality_report(hdev, skb->data, skb->len,
> > > > > +                                   QUALITY_SPEC_AOSP_BQR);
> > > > > +}
> > > > > diff --git a/net/bluetooth/aosp.h b/net/bluetooth/aosp.h
> > > > > index 2fd8886d51b2..8208e01fffed 100644
> > > > > --- a/net/bluetooth/aosp.h
> > > > > +++ b/net/bluetooth/aosp.h
> > > > > @@ -10,6 +10,9 @@ void aosp_do_close(struct hci_dev *hdev);
> > > > >
> > > > >  bool aosp_has_quality_report(struct hci_dev *hdev);
> > > > >  int aosp_set_quality_report(struct hci_dev *hdev, bool enable);
> > > > > +bool aosp_check_quality_report_len(struct sk_buff *skb);
> > > > > +struct ext_vendor_prefix *aosp_get_ext_prefix(struct hci_dev *hdev);
> > > > > +void aosp_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb);
> > > > >
> > > > >  #else
> > > > >
> > > > > @@ -26,4 +29,19 @@ static inline int aosp_set_quality_report(struct hci_dev *hdev, bool enable)
> > > > >         return -EOPNOTSUPP;
> > > > >  }
> > > > >
> > > > > +static inline bool aosp_check_quality_report_len(struct sk_buff *skb)
> > > > > +{
> > > > > +       return false;
> > > > > +}
> > > > > +
> > > > > +static inline struct ext_vendor_prefix *
> > > > > +aosp_get_ext_prefix(struct hci_dev *hdev)
> > > > > +{
> > > > > +       return NULL;
> > > > > +}
> > > > > +
> > > > > +static inline void aosp_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb)
> > > > > +{
> > > > > +}
> > > > > +
> > > > >  #endif
> > > > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > > > > index 0270e597c285..8398971eddf4 100644
> > > > > --- a/net/bluetooth/hci_event.c
> > > > > +++ b/net/bluetooth/hci_event.c
> > > > > @@ -37,6 +37,7 @@
> > > > >  #include "smp.h"
> > > > >  #include "msft.h"
> > > > >  #include "eir.h"
> > > > > +#include "aosp.h"
> > > > >
> > > > >  #define ZERO_KEY "\x00\x00\x00\x00\x00\x00\x00\x00" \
> > > > >                  "\x00\x00\x00\x00\x00\x00\x00\x00"
> > > > > @@ -4259,6 +4260,57 @@ static void hci_num_comp_blocks_evt(struct hci_dev *hdev, void *data,
> > > > >         queue_work(hdev->workqueue, &hdev->tx_work);
> > > > >  }
> > > > >
> > > > > +/* Every distinct vendor specification must have a well-defined vendor
> > > > > + * event prefix to determine if a vendor event meets the specification.
> > > > > + * Some vendor prefixes are fixed values while some other vendor prefixes
> > > > > + * are only available at run time.
> > > > > + */
> > > > > +static struct ext_vendor_event_prefix {
> > > > > +       /* Some vendor prefixes are variable length. For convenience,
> > > > > +        * the prefix in struct ext_vendor_prefix is in little endian.
> > > > > +        */
> > > > > +       struct ext_vendor_prefix *
> > > > > +               (*get_ext_vendor_prefix)(struct hci_dev *hdev);
> > > > > +       void (*vendor_func)(struct hci_dev *hdev, struct sk_buff *skb);
> > > > > +} evt_prefixes[] = {
> > > > > +       { aosp_get_ext_prefix, aosp_vendor_evt },
> > > > > +       { msft_get_ext_prefix, msft_vendor_evt },
> > > > > +
> > > > > +       /* end with a null entry */
> > > > > +       {},
> > > > > +};
> > > > > +
> > > > > +static void hci_vendor_evt(struct hci_dev *hdev, void *data,
> > > > > +                          struct sk_buff *skb)
> > > > > +{
> > > > > +       int i, j;
> > > > > +       struct ext_vendor_prefix *vnd;
> > > > > +       __u8 subcode;
> > > > > +
> > > > > +       for (i = 0; evt_prefixes[i].get_ext_vendor_prefix; i++) {
> > > > > +               vnd = evt_prefixes[i].get_ext_vendor_prefix(hdev);
> > > > > +               if (!vnd)
> > > > > +                       continue;
>
> We need to check like if (skb->len < vnd->prefix_len), btw are the

This condition has been checked below.

> prefixes supposed to be changed at runtime? If not probably a const

The answer: yes, the MSFT prefixes change at runtime. Hence, a const
table would not work. Please refer to the Extended Vendor Prefix
section in the shared doc
(https://docs.google.com/document/d/1QAs_Z36O41OFoIoWx2DzWve4rS4xm6RC7TipI3odPqY/edit?resourcekey=0-PQgtBiWGStMfnNzX6J4arg#heading=h.4fmxxrhdjdi0)
about what current vendor prefixes actually look like in various
vendor specifications.

> table would work better. In fact I would suggest changing the
> vendor_func to start parsing _after_ the prefix so you can use
> skb_pull_data, or create a helper that does that conditionally e.g
> skb_pull_data_cmp(skb, data, data_len).

I do not understand the meaning clearly. Do you mean
get_ext_vendor_prefix instead of vendor_func here? The
get_ext_vendor_prefix does provide more data, which is the subode,
_after_ the prefix for comparison. On the other hand, we do not
concatenate the subcode with the prefix directly for the reason of
flexibility to accommodate multiple subcodes in a vendor
specification.

>
> > > > > +               /* Compare the raw prefix data in little endian directly. */
> > > > > +               if (memcmp(vnd->prefix, skb->data, vnd->prefix_len))
> > > > > +                       continue;
> > > > > +
> > > > > +               /* Make sure that there are more data after prefix. */
> > > > > +               if (skb->len <= vnd->prefix_len)
> > > > > +                       continue;
> > > > > +
> > > > > +               /* The subcode is the single octet following the prefix. */
> > > > > +               subcode = skb->data[vnd->prefix_len];
>
> This part I don't really get, so the core is doing subcode matching,
> what if the vendor decides that the subcode is part of the prefix?

AFAIK, the vendor specs, including MSFT, AOSP, and Intel telemetry,
define subcodes explicitly without mixing them with the prefixes.
However, prefixes may not be defined as in the case of the AOSP spec.

> Prefix matching is one thing but prefix + subcode is another pattern,
> or is this to allow 0 prefix_len so it essentially becomes subcode

Yes, it allows prefix_len to be 0. For example, the AOSP spec does not
define a prefix, and hence its length is 0. Yes, it essentially
becomes subcode matching in this case.

> matching? Anyway I was thinking this could all be work out in a single
> const table e.g:

The const table may not be possible for one reason. It does not
accommodate the MSFT dynamic prefixes that are only known at run time.

In addition, the concept of using “u8 prefix[]; /* Prefix + subcode
*/” is less extensible. For example, MSFT vendor events define two
subcodes, 0x01 and 0x02, so far. More subcodes are on the road. Using
the concept that you proposed above, we would need two entries for the
two subcodes in the table. More entries are needed soon as various
specifications are planning more subcodes. I think it is a bit cleaner
to dispatch the MSFT vendor events to the MSFT vendor function and let
the vendor function to handle their subcodes inside their functions.
It seems that the core table does not need to know the details about
the internal things of the vendor specifications.

>
> #define data(args...) ((const unsigned char[]) { args })
>
> #define HCI_VENDOR_EVT(_func, _prefix_len, _prefix...)
> { \
>     .func = _func,
>     .prefix_len = sizeof(data(_prefix)),
>     .prefix = data(_prefix),
> }
>
> const static struct hci_vendor_evt {
>     (*func)...
>     u16 prefix_len;
>     u8 prefix[]; /* Prefix + subcode */
> } = vendor_evt_table[] = {
>     HCI_VENDOR_EVT(msft_monitor_device_evt, sizeof(msft_device_evt),
> msft_prefix, MSFT_EV_LE_MONITOR_DEVICE),
> };
>
> So it becomes pretty similar to the existing const tables we have in

As explained above, MSFT prefixes should be queried at run time per
the specification. Hence, the table would not be similar to the
existing const tables.

> hci_event.c with the only difference is that we use prefix matching
> where the length is not constant among each element, the only drawback
> that I can think of is that all vendor events will need to be added to
> the table (msftp, aosp, etc.) but perhaps that makes it more visible

The visibility does look to me as an advantage such that there is a
single table describing what specs are being supported.

> on exactly what events are supported instead of every subcode being
> handled automatically which is not always the case, the table(s) would
> sit in the driver code and would be registered to hdev all at once
> instead of setting each prefix separately at runtime this all happens
> at build time so the compiler can do its job while compiling it.

As explained above, MSFT prefixes should be queried at run time per
the specification. Hence, what you like to do cannot all happen at
build time.

One of my previous series-version uses a mixed table. For Intel and
AOSP, their table entries are set statistically, while the MSFT table
entry is queried at run time. The table looked a bit cluttered in that
way though.

>
> > > > > +               for (j = 0; j < vnd->subcodes_len; j++) {
> > > > > +                       if (vnd->subcodes[j] == subcode) {
> > > > > +                               evt_prefixes[i].vendor_func(hdev, skb);
> > > > > +                               break;
> > > > > +                       }
> > > > > +               }
> > > > > +       }
> > > > > +}
> > > >
> > > > I recall saying that having such matching logic applied without the
> > > > driver confirming that is the structure it using to be a bad idea
> > > > since it could actually cause an event to misinterpret and cause bad
> > > > behavior, instead we probably need a callback that gets populated by
> > > > the driver e.g.(hdev->vendor_evt) then the driver can either populate
> > > > with hci_vendor_evt if it does use prefixes or its own specialized
> > > > function or NULL if it doesn't use vendor events, specially for old
> > > > controllers Id leave it as NULL.
> > > >
> > > > >  static void hci_mode_change_evt(struct hci_dev *hdev, void *data,
> > > > >                                 struct sk_buff *skb)
> > > > >  {
> > > > > @@ -6879,7 +6931,7 @@ static const struct hci_ev {
> > > > >         HCI_EV(HCI_EV_NUM_COMP_BLOCKS, hci_num_comp_blocks_evt,
> > > > >                sizeof(struct hci_ev_num_comp_blocks)),
> > > > >         /* [0xff = HCI_EV_VENDOR] */
> > > > > -       HCI_EV_VL(HCI_EV_VENDOR, msft_vendor_evt, 0, HCI_MAX_EVENT_SIZE),
> > > > > +       HCI_EV_VL(HCI_EV_VENDOR, hci_vendor_evt, 0, HCI_MAX_EVENT_SIZE),
> > > > >  };
> > > > >
> > > > >  static void hci_event_func(struct hci_dev *hdev, u8 event, struct sk_buff *skb,
> > > > > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> > > > > index 1ad84f34097f..9d3666bdd07c 100644
> > > > > --- a/net/bluetooth/mgmt.c
> > > > > +++ b/net/bluetooth/mgmt.c
> > > > > @@ -4332,6 +4332,25 @@ static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
> > > > >                                MGMT_STATUS_NOT_SUPPORTED);
> > > > >  }
> > > > >
> > > > > +int mgmt_quality_report(struct hci_dev *hdev, void *data, u32 data_len,
> > > > > +                       u8 quality_spec)
> > > > > +{
> > > > > +       struct mgmt_ev_quality_report *ev;
> > > > > +       struct sk_buff *skb;
> > > > > +
> > > > > +       skb = mgmt_alloc_skb(hdev, MGMT_EV_QUALITY_REPORT,
> > > > > +                            sizeof(*ev) + data_len);
> > > > > +       if (!skb)
> > > > > +               return -ENOMEM;
> > > > > +
> > > > > +       ev = skb_put(skb, sizeof(*ev));
> > > > > +       ev->quality_spec = quality_spec;
> > > > > +       ev->data_len = data_len;
> > > > > +       skb_put_data(skb, data, data_len);
> > > > > +
> > > > > +       return mgmt_event_skb(skb, NULL);
> > > > > +}
> > > > > +
> > > > >  static int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
> > > > >                             u16 data_len)
> > > > >  {
> > > > > diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
> > > > > index f43994523b1f..c003e94faccd 100644
> > > > > --- a/net/bluetooth/msft.c
> > > > > +++ b/net/bluetooth/msft.c
> > > > > @@ -116,6 +116,20 @@ bool msft_monitor_supported(struct hci_dev *hdev)
> > > > >         return !!(msft_get_features(hdev) & MSFT_FEATURE_MASK_LE_ADV_MONITOR);
> > > > >  }
> > > > >
> > > > > +/* Add the MSFT vendor event subcodes into MSFT_SUBCODES which
> > > > > + * msft_vendor_evt() is interested in handling.
> > > > > + */
> > > > > +static unsigned char MSFT_SUBCODES[] = { MSFT_EV_LE_MONITOR_DEVICE };
> > > > > +static struct ext_vendor_prefix msft_ext_prefix = { 0 };
> > > > > +
> > > > > +static void set_ext_prefix(struct msft_data *msft)
> > > > > +{
> > > > > +       msft_ext_prefix.prefix = msft->evt_prefix;
> > > > > +       msft_ext_prefix.prefix_len = msft->evt_prefix_len;
> > > > > +       msft_ext_prefix.subcodes = MSFT_SUBCODES;
> > > > > +       msft_ext_prefix.subcodes_len = sizeof(MSFT_SUBCODES);
> > > > > +}
> > > > > +
> > > > >  static bool read_supported_features(struct hci_dev *hdev,
> > > > >                                     struct msft_data *msft)
> > > > >  {
> > > > > @@ -156,6 +170,8 @@ static bool read_supported_features(struct hci_dev *hdev,
> > > > >         if (msft->features & MSFT_FEATURE_MASK_CURVE_VALIDITY)
> > > > >                 hdev->msft_curve_validity = true;
> > > > >
> > > > > +       set_ext_prefix(msft);
> > > > > +
> > > > >         kfree_skb(skb);
> > > > >         return true;
> > > > >
> > > > > @@ -742,7 +758,17 @@ static void msft_monitor_device_evt(struct hci_dev *hdev, struct sk_buff *skb)
> > > > >                                  handle_data->mgmt_handle);
> > > > >  }
> > > > >
> > > > > -void msft_vendor_evt(struct hci_dev *hdev, void *data, struct sk_buff *skb)
> > > > > +struct ext_vendor_prefix *msft_get_ext_prefix(struct hci_dev *hdev)
> > > > > +{
> > > > > +       struct msft_data *msft = hdev->msft_data;
> > > > > +
> > > > > +       if (!msft)
> > > > > +               return NULL;
> > > > > +
> > > > > +       return &msft_ext_prefix;
> > > > > +}
> > > > > +
> > > > > +void msft_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb)
> > > > >  {
> > > > >         struct msft_data *msft = hdev->msft_data;
> > > > >         u8 *evt_prefix;
> > > > > diff --git a/net/bluetooth/msft.h b/net/bluetooth/msft.h
> > > > > index afcaf7d3b1cb..1515ae06c628 100644
> > > > > --- a/net/bluetooth/msft.h
> > > > > +++ b/net/bluetooth/msft.h
> > > > > @@ -17,7 +17,7 @@ void msft_register(struct hci_dev *hdev);
> > > > >  void msft_unregister(struct hci_dev *hdev);
> > > > >  void msft_do_open(struct hci_dev *hdev);
> > > > >  void msft_do_close(struct hci_dev *hdev);
> > > > > -void msft_vendor_evt(struct hci_dev *hdev, void *data, struct sk_buff *skb);
> > > > > +void msft_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb);
> > > > >  __u64 msft_get_features(struct hci_dev *hdev);
> > > > >  int msft_add_monitor_pattern(struct hci_dev *hdev, struct adv_monitor *monitor);
> > > > >  int msft_remove_monitor(struct hci_dev *hdev, struct adv_monitor *monitor,
> > > > > @@ -27,6 +27,7 @@ int msft_set_filter_enable(struct hci_dev *hdev, bool enable);
> > > > >  int msft_suspend_sync(struct hci_dev *hdev);
> > > > >  int msft_resume_sync(struct hci_dev *hdev);
> > > > >  bool msft_curve_validity(struct hci_dev *hdev);
> > > > > +struct ext_vendor_prefix *msft_get_ext_prefix(struct hci_dev *hdev);
> > > > >
> > > > >  #else
> > > > >
> > > > > @@ -39,8 +40,7 @@ static inline void msft_register(struct hci_dev *hdev) {}
> > > > >  static inline void msft_unregister(struct hci_dev *hdev) {}
> > > > >  static inline void msft_do_open(struct hci_dev *hdev) {}
> > > > >  static inline void msft_do_close(struct hci_dev *hdev) {}
> > > > > -static inline void msft_vendor_evt(struct hci_dev *hdev, void *data,
> > > > > -                                  struct sk_buff *skb) {}
> > > > > +static inline void msft_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb) {}
> > > > >  static inline __u64 msft_get_features(struct hci_dev *hdev) { return 0; }
> > > > >  static inline int msft_add_monitor_pattern(struct hci_dev *hdev,
> > > > >                                            struct adv_monitor *monitor)
> > > > > @@ -77,4 +77,10 @@ static inline bool msft_curve_validity(struct hci_dev *hdev)
> > > > >         return false;
> > > > >  }
> > > > >
> > > > > +static inline struct ext_vendor_prefix *
> > > > > +msft_get_ext_prefix(struct hci_dev *hdev)
> > > > > +{
> > > > > +       return NULL;
> > > > > +}
> > > > > +
> > > > >  #endif
> > > > > --
> > > > > 2.36.1.124.g0e6072fb45-goog
> > > > >
> > > >
> > > >
> > > > --
> > > > Luiz Augusto von Dentz
> > >
> > >
> > >
> > > --
> > >
> > > Joseph Shyh-In Hwang
> > > Email: [e-mail]
> >
> >
> >
> > --
> > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz

Marcel Holtmann June 2, 2022, 4:47 p.m. UTC | #6

Hi Joseph,

> When receiving a HCI vendor event, the kernel checks if it is an
> AOSP bluetooth quality report. If yes, the event is sent to bluez
> user space through the mgmt socket.
> 
> Reported-by: kernel test robot <[e-mail]>
> 
> Signed-off-by: Joseph Hwang <[e-mail]>
> Reviewed-by: Archie Pusaka <[e-mail]>
> ---
> 
> Changes in v6:
> - Fixed a sparse check warning about using static for evt_prefixes.
> 
> Changes in v5:
> - Define "struct ext_vendor_prefix" to replace "struct vendor_prefix"
>  so that extended vendor prefix = prefix + 1-octet subcode
> - Define aosp_ext_prefix to provide AOSP extended prefix which is
>  returned by aosp_get_ext_prefix().
> - Redefine struct ext_vendor_event_prefix such that
>  . it uses get_ext_vendor_prefix to get prefix and subcodes where
>    the prefix and the prefix length may be variable and are not
>    unknown until run time;
>  . it uses vendor_func to handle a vendor event
>  This table handles vendor events in a generic way.
> - Rewrite hci_vendor_evt() so that it compares both vendor prefix
>  and subcode to match a vendor event.
> - Define set_ext_prefix() to create MSFT extended vendor prefix
>  which is returned by msft_get_ext_prefix().
> - Do not EXPORT_SYMBOL(mgmt_quality_report).
> - Keep msft_get_ext_prefix in msft instead of hci_dev since it is
>  not used by any drivers.
> 
> Changes in v3:
> - Rebase to resolve the code conflict.
> - Move aosp_quality_report_evt() from hci_event.c to aosp.c.
> - A new patch (3/3) is added to enable the quality report feature.
> 
> Changes in v2:
> - Scrap the two structures defined in aosp.c and use constants for
>  size check.
> - Do a basic size check about the quality report event. Do not pull
>  data from the event in which the kernel has no interest.
> - Define vendor event prefixes with which vendor events of distinct
>  vendor specifications can be clearly differentiated.
> - Use mgmt helpers to add the header and data to a mgmt skb.
> 
> include/net/bluetooth/hci_core.h | 12 +++++++
> include/net/bluetooth/mgmt.h     |  7 +++++
> net/bluetooth/aosp.c             | 50 +++++++++++++++++++++++++++++
> net/bluetooth/aosp.h             | 18 +++++++++++
> net/bluetooth/hci_event.c        | 54 +++++++++++++++++++++++++++++++-
> net/bluetooth/mgmt.c             | 19 +++++++++++
> net/bluetooth/msft.c             | 28 ++++++++++++++++-
> net/bluetooth/msft.h             | 12 +++++--
> 8 files changed, 195 insertions(+), 5 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 64d3a63759a8..f89738c6b973 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -328,6 +328,13 @@ struct amp_assoc {
> 
> #define HCI_MAX_PAGES	3
> 
> +struct ext_vendor_prefix {
> +	__u8 *prefix;
> +	__u8 prefix_len;
> +	__u8 *subcodes;
> +	__u8 subcodes_len;
> +};
> +
> struct hci_dev {
> 	struct list_head list;
> 	struct mutex	lock;
> @@ -1876,6 +1883,8 @@ int mgmt_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status);
> int mgmt_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status);
> void mgmt_adv_monitor_device_lost(struct hci_dev *hdev, u16 handle,
> 				  bdaddr_t *bdaddr, u8 addr_type);
> +int mgmt_quality_report(struct hci_dev *hdev, void *data, u32 data_len,
> +			u8 quality_spec);
> 
> u8 hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max, u16 latency,
> 		      u16 to_multiplier);
> @@ -1894,4 +1903,7 @@ void hci_copy_identity_address(struct hci_dev *hdev, bdaddr_t *bdaddr,
> 
> #define TRANSPORT_TYPE_MAX	0x04
> 
> +#define QUALITY_SPEC_AOSP_BQR		0x0
> +#define QUALITY_SPEC_INTEL_TELEMETRY	0x1
> +
> #endif /* __HCI_CORE_H */
> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> index c1c2fd72d9e3..6ccd0067c295 100644
> --- a/include/net/bluetooth/mgmt.h
> +++ b/include/net/bluetooth/mgmt.h
> @@ -1127,3 +1127,10 @@ struct mgmt_ev_adv_monitor_device_lost {
> 	__le16 monitor_handle;
> 	struct mgmt_addr_info addr;
> } __packed;
> +
> +#define MGMT_EV_QUALITY_REPORT			0x0031
> +struct mgmt_ev_quality_report {
> +	__u8	quality_spec;
> +	__u32	data_len;
> +	__u8	data[];
> +} __packed;

I would really prefer you separate the addition of ext_prefix handling and the addition of the AOSP report and mgmt report event. It is currently a bit of a mix and match.

> diff --git a/net/bluetooth/aosp.c b/net/bluetooth/aosp.c
> index 432ae3aac9e3..94faa15b1ea0 100644
> --- a/net/bluetooth/aosp.c
> +++ b/net/bluetooth/aosp.c
> @@ -199,3 +199,53 @@ int aosp_set_quality_report(struct hci_dev *hdev, bool enable)
> 	else
> 		return disable_quality_report(hdev);
> }
> +
> +/* The following LEN = 1-byte Sub-event code + 48-byte Sub-event Parameters */
> +#define BLUETOOTH_QUALITY_REPORT_LEN 49
> +
> +bool aosp_check_quality_report_len(struct sk_buff *skb)
> +{
> +	/* skb->len is allowed to be larger than BLUETOOTH_QUALITY_REPORT_LEN
> +	 * to accommodate an additional Vendor Specific Parameter (vsp) field.
> +	 */
> +	if (skb->len < BLUETOOTH_QUALITY_REPORT_LEN) {
> +		BT_ERR("AOSP evt data len %d too short (%u expected)",
> +		       skb->len, BLUETOOTH_QUALITY_REPORT_LEN);
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +/* AOSP HCI Requirements use 0x54 and up as sub-event codes without
> + * actually defining a vendor prefix. Refer to
> + * https://source.android.com/devices/bluetooth/hci_requirements
> + * Hence, the other vendor event prefixes should not use the same
> + * space to avoid collision.
> + * Since the AOSP does not define a prefix, its prefix is NULL
> + * and prefix_len is 0.
> + * While there are a number of subcodes in AOSP, only interested in
> + * Bluetooth Quality Report (0x58) for now.
> + */
> +#define AOSP_EV_QUALITY_REPORT		0x58
> +
> +static unsigned char AOSP_SUBCODES[] = { AOSP_EV_QUALITY_REPORT };
> +
> +static struct ext_vendor_prefix aosp_ext_prefix = {
> +	.prefix		= NULL,
> +	.prefix_len	= 0,
> +	.subcodes	= AOSP_SUBCODES,
> +	.subcodes_len	= sizeof(AOSP_SUBCODES),
> +};
> +

So I really like to have both as const. Any reason not to make array and struct const.

The all upper case AOSP_SUBCODES is weird to me, but we could let this slide since it is limited to these few lines.

> +struct ext_vendor_prefix *aosp_get_ext_prefix(struct hci_dev *hdev)
> +{
> +	return &aosp_ext_prefix;
> +}
> +
> +void aosp_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb)
> +{
> +	if (aosp_has_quality_report(hdev) && aosp_check_quality_report_len(skb))
> +		mgmt_quality_report(hdev, skb->data, skb->len,
> +				    QUALITY_SPEC_AOSP_BQR);
> +}
> diff --git a/net/bluetooth/aosp.h b/net/bluetooth/aosp.h
> index 2fd8886d51b2..8208e01fffed 100644
> --- a/net/bluetooth/aosp.h
> +++ b/net/bluetooth/aosp.h
> @@ -10,6 +10,9 @@ void aosp_do_close(struct hci_dev *hdev);
> 
> bool aosp_has_quality_report(struct hci_dev *hdev);
> int aosp_set_quality_report(struct hci_dev *hdev, bool enable);
> +bool aosp_check_quality_report_len(struct sk_buff *skb);
> +struct ext_vendor_prefix *aosp_get_ext_prefix(struct hci_dev *hdev);
> +void aosp_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb);
> 
> #else
> 
> @@ -26,4 +29,19 @@ static inline int aosp_set_quality_report(struct hci_dev *hdev, bool enable)
> 	return -EOPNOTSUPP;
> }
> 
> +static inline bool aosp_check_quality_report_len(struct sk_buff *skb)
> +{
> +	return false;
> +}
> +
> +static inline struct ext_vendor_prefix *
> +aosp_get_ext_prefix(struct hci_dev *hdev)
> +{
> +	return NULL;
> +}
> +
> +static inline void aosp_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb)
> +{
> +}
> +
> #endif
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 0270e597c285..8398971eddf4 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -37,6 +37,7 @@
> #include "smp.h"
> #include "msft.h"
> #include "eir.h"
> +#include "aosp.h"
> 
> #define ZERO_KEY "\x00\x00\x00\x00\x00\x00\x00\x00" \
> 		 "\x00\x00\x00\x00\x00\x00\x00\x00"
> @@ -4259,6 +4260,57 @@ static void hci_num_comp_blocks_evt(struct hci_dev *hdev, void *data,
> 	queue_work(hdev->workqueue, &hdev->tx_work);
> }
> 
> +/* Every distinct vendor specification must have a well-defined vendor
> + * event prefix to determine if a vendor event meets the specification.
> + * Some vendor prefixes are fixed values while some other vendor prefixes
> + * are only available at run time.
> + */
> +static struct ext_vendor_event_prefix {
> +	/* Some vendor prefixes are variable length. For convenience,
> +	 * the prefix in struct ext_vendor_prefix is in little endian.
> +	 */
> +	struct ext_vendor_prefix *
> +		(*get_ext_vendor_prefix)(struct hci_dev *hdev);

You can just shorten this to get and func here.

> +	void (*vendor_func)(struct hci_dev *hdev, struct sk_buff *skb);
> +} evt_prefixes[] = {
> +	{ aosp_get_ext_prefix, aosp_vendor_evt },
> +	{ msft_get_ext_prefix, msft_vendor_evt },
> +
> +	/* end with a null entry */
> +	{},
> +};
> +

Again here, why is this not const?

> +static void hci_vendor_evt(struct hci_dev *hdev, void *data,
> +			   struct sk_buff *skb)
> +{
> +	int i, j;
> +	struct ext_vendor_prefix *vnd;
> +	__u8 subcode;
> +
> +	for (i = 0; evt_prefixes[i].get_ext_vendor_prefix; i++) {
> +		vnd = evt_prefixes[i].get_ext_vendor_prefix(hdev);
> +		if (!vnd)
> +			continue;
> +
> +		/* Compare the raw prefix data in little endian directly. */

Little endian has nothing to do with this. You are comparing a byte-stream. That has no endianness.

> +		if (memcmp(vnd->prefix, skb->data, vnd->prefix_len))
> +			continue;
> +
> +		/* Make sure that there are more data after prefix. */
> +		if (skb->len <= vnd->prefix_len)
> +			continue;
> +
> +		/* The subcode is the single octet following the prefix. */
> +		subcode = skb->data[vnd->prefix_len];
> +		for (j = 0; j < vnd->subcodes_len; j++) {
> +			if (vnd->subcodes[j] == subcode) {
> +				evt_prefixes[i].vendor_func(hdev, skb);
> +				break;
> +			}
> +		}
> +	}
> +}
> +
> static void hci_mode_change_evt(struct hci_dev *hdev, void *data,
> 				struct sk_buff *skb)
> {
> @@ -6879,7 +6931,7 @@ static const struct hci_ev {
> 	HCI_EV(HCI_EV_NUM_COMP_BLOCKS, hci_num_comp_blocks_evt,
> 	       sizeof(struct hci_ev_num_comp_blocks)),
> 	/* [0xff = HCI_EV_VENDOR] */
> -	HCI_EV_VL(HCI_EV_VENDOR, msft_vendor_evt, 0, HCI_MAX_EVENT_SIZE),
> +	HCI_EV_VL(HCI_EV_VENDOR, hci_vendor_evt, 0, HCI_MAX_EVENT_SIZE),
> };
> 
> static void hci_event_func(struct hci_dev *hdev, u8 event, struct sk_buff *skb,
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 1ad84f34097f..9d3666bdd07c 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -4332,6 +4332,25 @@ static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
> 			       MGMT_STATUS_NOT_SUPPORTED);
> }
> 
> +int mgmt_quality_report(struct hci_dev *hdev, void *data, u32 data_len,
> +			u8 quality_spec)
> +{
> +	struct mgmt_ev_quality_report *ev;
> +	struct sk_buff *skb;
> +
> +	skb = mgmt_alloc_skb(hdev, MGMT_EV_QUALITY_REPORT,
> +			     sizeof(*ev) + data_len);
> +	if (!skb)
> +		return -ENOMEM;
> +
> +	ev = skb_put(skb, sizeof(*ev));
> +	ev->quality_spec = quality_spec;
> +	ev->data_len = data_len;
> +	skb_put_data(skb, data, data_len);
> +
> +	return mgmt_event_skb(skb, NULL);
> +}
> +
> static int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
> 			    u16 data_len)
> {
> diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
> index f43994523b1f..c003e94faccd 100644
> --- a/net/bluetooth/msft.c
> +++ b/net/bluetooth/msft.c
> @@ -116,6 +116,20 @@ bool msft_monitor_supported(struct hci_dev *hdev)
> 	return !!(msft_get_features(hdev) & MSFT_FEATURE_MASK_LE_ADV_MONITOR);
> }
> 
> +/* Add the MSFT vendor event subcodes into MSFT_SUBCODES which
> + * msft_vendor_evt() is interested in handling.
> + */
> +static unsigned char MSFT_SUBCODES[] = { MSFT_EV_LE_MONITOR_DEVICE };
> +static struct ext_vendor_prefix msft_ext_prefix = { 0 };
> +
> +static void set_ext_prefix(struct msft_data *msft)
> +{
> +	msft_ext_prefix.prefix = msft->evt_prefix;
> +	msft_ext_prefix.prefix_len = msft->evt_prefix_len;
> +	msft_ext_prefix.subcodes = MSFT_SUBCODES;
> +	msft_ext_prefix.subcodes_len = sizeof(MSFT_SUBCODES);
> +}
> +
> static bool read_supported_features(struct hci_dev *hdev,
> 				    struct msft_data *msft)
> {
> @@ -156,6 +170,8 @@ static bool read_supported_features(struct hci_dev *hdev,
> 	if (msft->features & MSFT_FEATURE_MASK_CURVE_VALIDITY)
> 		hdev->msft_curve_validity = true;
> 
> +	set_ext_prefix(msft);
> +

This is broken!

If you have one Intel controller and one Realtek controller in your system, you now created a big mess. You are overwriting msft_ext_prefix with each msft_do_open() now.

For AOSP this works because it is the same struct no matter what the manufacturer is. For MSFT extensions it depends on the controller.

> 	kfree_skb(skb);
> 	return true;
> 
> @@ -742,7 +758,17 @@ static void msft_monitor_device_evt(struct hci_dev *hdev, struct sk_buff *skb)
> 				 handle_data->mgmt_handle);
> }
> 
> -void msft_vendor_evt(struct hci_dev *hdev, void *data, struct sk_buff *skb)
> +struct ext_vendor_prefix *msft_get_ext_prefix(struct hci_dev *hdev)
> +{
> +	struct msft_data *msft = hdev->msft_data;
> +
> +	if (!msft)
> +		return NULL;
> +
> +	return &msft_ext_prefix;
> +}
> +
> +void msft_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb)
> {
> 	struct msft_data *msft = hdev->msft_data;
> 	u8 *evt_prefix;
> diff --git a/net/bluetooth/msft.h b/net/bluetooth/msft.h
> index afcaf7d3b1cb..1515ae06c628 100644
> --- a/net/bluetooth/msft.h
> +++ b/net/bluetooth/msft.h
> @@ -17,7 +17,7 @@ void msft_register(struct hci_dev *hdev);
> void msft_unregister(struct hci_dev *hdev);
> void msft_do_open(struct hci_dev *hdev);
> void msft_do_close(struct hci_dev *hdev);
> -void msft_vendor_evt(struct hci_dev *hdev, void *data, struct sk_buff *skb);
> +void msft_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb);
> __u64 msft_get_features(struct hci_dev *hdev);
> int msft_add_monitor_pattern(struct hci_dev *hdev, struct adv_monitor *monitor);
> int msft_remove_monitor(struct hci_dev *hdev, struct adv_monitor *monitor,
> @@ -27,6 +27,7 @@ int msft_set_filter_enable(struct hci_dev *hdev, bool enable);
> int msft_suspend_sync(struct hci_dev *hdev);
> int msft_resume_sync(struct hci_dev *hdev);
> bool msft_curve_validity(struct hci_dev *hdev);
> +struct ext_vendor_prefix *msft_get_ext_prefix(struct hci_dev *hdev);
> 
> #else
> 
> @@ -39,8 +40,7 @@ static inline void msft_register(struct hci_dev *hdev) {}
> static inline void msft_unregister(struct hci_dev *hdev) {}
> static inline void msft_do_open(struct hci_dev *hdev) {}
> static inline void msft_do_close(struct hci_dev *hdev) {}
> -static inline void msft_vendor_evt(struct hci_dev *hdev, void *data,
> -				   struct sk_buff *skb) {}
> +static inline void msft_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb) {}
> static inline __u64 msft_get_features(struct hci_dev *hdev) { return 0; }
> static inline int msft_add_monitor_pattern(struct hci_dev *hdev,
> 					   struct adv_monitor *monitor)
> @@ -77,4 +77,10 @@ static inline bool msft_curve_validity(struct hci_dev *hdev)
> 	return false;
> }
> 
> +static inline struct ext_vendor_prefix *
> +msft_get_ext_prefix(struct hci_dev *hdev)
> +{
> +	return NULL;
> +}
> +
> #endif

Regards

Marcel

Patch

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 64d3a63759a8..f89738c6b973 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -328,6 +328,13 @@  struct amp_assoc {
 
 #define HCI_MAX_PAGES	3
 
+struct ext_vendor_prefix {
+	__u8 *prefix;
+	__u8 prefix_len;
+	__u8 *subcodes;
+	__u8 subcodes_len;
+};
+
 struct hci_dev {
 	struct list_head list;
 	struct mutex	lock;
@@ -1876,6 +1883,8 @@  int mgmt_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status);
 int mgmt_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status);
 void mgmt_adv_monitor_device_lost(struct hci_dev *hdev, u16 handle,
 				  bdaddr_t *bdaddr, u8 addr_type);
+int mgmt_quality_report(struct hci_dev *hdev, void *data, u32 data_len,
+			u8 quality_spec);
 
 u8 hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max, u16 latency,
 		      u16 to_multiplier);
@@ -1894,4 +1903,7 @@  void hci_copy_identity_address(struct hci_dev *hdev, bdaddr_t *bdaddr,
 
 #define TRANSPORT_TYPE_MAX	0x04
 
+#define QUALITY_SPEC_AOSP_BQR		0x0
+#define QUALITY_SPEC_INTEL_TELEMETRY	0x1
+
 #endif /* __HCI_CORE_H */
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index c1c2fd72d9e3..6ccd0067c295 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -1127,3 +1127,10 @@  struct mgmt_ev_adv_monitor_device_lost {
 	__le16 monitor_handle;
 	struct mgmt_addr_info addr;
 } __packed;
+
+#define MGMT_EV_QUALITY_REPORT			0x0031
+struct mgmt_ev_quality_report {
+	__u8	quality_spec;
+	__u32	data_len;
+	__u8	data[];
+} __packed;
diff --git a/net/bluetooth/aosp.c b/net/bluetooth/aosp.c
index 432ae3aac9e3..94faa15b1ea0 100644
--- a/net/bluetooth/aosp.c
+++ b/net/bluetooth/aosp.c
@@ -199,3 +199,53 @@  int aosp_set_quality_report(struct hci_dev *hdev, bool enable)
 	else
 		return disable_quality_report(hdev);
 }
+
+/* The following LEN = 1-byte Sub-event code + 48-byte Sub-event Parameters */
+#define BLUETOOTH_QUALITY_REPORT_LEN 49
+
+bool aosp_check_quality_report_len(struct sk_buff *skb)
+{
+	/* skb->len is allowed to be larger than BLUETOOTH_QUALITY_REPORT_LEN
+	 * to accommodate an additional Vendor Specific Parameter (vsp) field.
+	 */
+	if (skb->len < BLUETOOTH_QUALITY_REPORT_LEN) {
+		BT_ERR("AOSP evt data len %d too short (%u expected)",
+		       skb->len, BLUETOOTH_QUALITY_REPORT_LEN);
+		return false;
+	}
+
+	return true;
+}
+
+/* AOSP HCI Requirements use 0x54 and up as sub-event codes without
+ * actually defining a vendor prefix. Refer to
+ * https://source.android.com/devices/bluetooth/hci_requirements
+ * Hence, the other vendor event prefixes should not use the same
+ * space to avoid collision.
+ * Since the AOSP does not define a prefix, its prefix is NULL
+ * and prefix_len is 0.
+ * While there are a number of subcodes in AOSP, only interested in
+ * Bluetooth Quality Report (0x58) for now.
+ */
+#define AOSP_EV_QUALITY_REPORT		0x58
+
+static unsigned char AOSP_SUBCODES[] = { AOSP_EV_QUALITY_REPORT };
+
+static struct ext_vendor_prefix aosp_ext_prefix = {
+	.prefix		= NULL,
+	.prefix_len	= 0,
+	.subcodes	= AOSP_SUBCODES,
+	.subcodes_len	= sizeof(AOSP_SUBCODES),
+};
+
+struct ext_vendor_prefix *aosp_get_ext_prefix(struct hci_dev *hdev)
+{
+	return &aosp_ext_prefix;
+}
+
+void aosp_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	if (aosp_has_quality_report(hdev) && aosp_check_quality_report_len(skb))
+		mgmt_quality_report(hdev, skb->data, skb->len,
+				    QUALITY_SPEC_AOSP_BQR);
+}
diff --git a/net/bluetooth/aosp.h b/net/bluetooth/aosp.h
index 2fd8886d51b2..8208e01fffed 100644
--- a/net/bluetooth/aosp.h
+++ b/net/bluetooth/aosp.h
@@ -10,6 +10,9 @@  void aosp_do_close(struct hci_dev *hdev);
 
 bool aosp_has_quality_report(struct hci_dev *hdev);
 int aosp_set_quality_report(struct hci_dev *hdev, bool enable);
+bool aosp_check_quality_report_len(struct sk_buff *skb);
+struct ext_vendor_prefix *aosp_get_ext_prefix(struct hci_dev *hdev);
+void aosp_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb);
 
 #else
 
@@ -26,4 +29,19 @@  static inline int aosp_set_quality_report(struct hci_dev *hdev, bool enable)
 	return -EOPNOTSUPP;
 }
 
+static inline bool aosp_check_quality_report_len(struct sk_buff *skb)
+{
+	return false;
+}
+
+static inline struct ext_vendor_prefix *
+aosp_get_ext_prefix(struct hci_dev *hdev)
+{
+	return NULL;
+}
+
+static inline void aosp_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb)
+{
+}
+
 #endif
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 0270e597c285..8398971eddf4 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -37,6 +37,7 @@ 
 #include "smp.h"
 #include "msft.h"
 #include "eir.h"
+#include "aosp.h"
 
 #define ZERO_KEY "\x00\x00\x00\x00\x00\x00\x00\x00" \
 		 "\x00\x00\x00\x00\x00\x00\x00\x00"
@@ -4259,6 +4260,57 @@  static void hci_num_comp_blocks_evt(struct hci_dev *hdev, void *data,
 	queue_work(hdev->workqueue, &hdev->tx_work);
 }
 
+/* Every distinct vendor specification must have a well-defined vendor
+ * event prefix to determine if a vendor event meets the specification.
+ * Some vendor prefixes are fixed values while some other vendor prefixes
+ * are only available at run time.
+ */
+static struct ext_vendor_event_prefix {
+	/* Some vendor prefixes are variable length. For convenience,
+	 * the prefix in struct ext_vendor_prefix is in little endian.
+	 */
+	struct ext_vendor_prefix *
+		(*get_ext_vendor_prefix)(struct hci_dev *hdev);
+	void (*vendor_func)(struct hci_dev *hdev, struct sk_buff *skb);
+} evt_prefixes[] = {
+	{ aosp_get_ext_prefix, aosp_vendor_evt },
+	{ msft_get_ext_prefix, msft_vendor_evt },
+
+	/* end with a null entry */
+	{},
+};
+
+static void hci_vendor_evt(struct hci_dev *hdev, void *data,
+			   struct sk_buff *skb)
+{
+	int i, j;
+	struct ext_vendor_prefix *vnd;
+	__u8 subcode;
+
+	for (i = 0; evt_prefixes[i].get_ext_vendor_prefix; i++) {
+		vnd = evt_prefixes[i].get_ext_vendor_prefix(hdev);
+		if (!vnd)
+			continue;
+
+		/* Compare the raw prefix data in little endian directly. */
+		if (memcmp(vnd->prefix, skb->data, vnd->prefix_len))
+			continue;
+
+		/* Make sure that there are more data after prefix. */
+		if (skb->len <= vnd->prefix_len)
+			continue;
+
+		/* The subcode is the single octet following the prefix. */
+		subcode = skb->data[vnd->prefix_len];
+		for (j = 0; j < vnd->subcodes_len; j++) {
+			if (vnd->subcodes[j] == subcode) {
+				evt_prefixes[i].vendor_func(hdev, skb);
+				break;
+			}
+		}
+	}
+}
+
 static void hci_mode_change_evt(struct hci_dev *hdev, void *data,
 				struct sk_buff *skb)
 {
@@ -6879,7 +6931,7 @@  static const struct hci_ev {
 	HCI_EV(HCI_EV_NUM_COMP_BLOCKS, hci_num_comp_blocks_evt,
 	       sizeof(struct hci_ev_num_comp_blocks)),
 	/* [0xff = HCI_EV_VENDOR] */
-	HCI_EV_VL(HCI_EV_VENDOR, msft_vendor_evt, 0, HCI_MAX_EVENT_SIZE),
+	HCI_EV_VL(HCI_EV_VENDOR, hci_vendor_evt, 0, HCI_MAX_EVENT_SIZE),
 };
 
 static void hci_event_func(struct hci_dev *hdev, u8 event, struct sk_buff *skb,
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 1ad84f34097f..9d3666bdd07c 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -4332,6 +4332,25 @@  static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
 			       MGMT_STATUS_NOT_SUPPORTED);
 }
 
+int mgmt_quality_report(struct hci_dev *hdev, void *data, u32 data_len,
+			u8 quality_spec)
+{
+	struct mgmt_ev_quality_report *ev;
+	struct sk_buff *skb;
+
+	skb = mgmt_alloc_skb(hdev, MGMT_EV_QUALITY_REPORT,
+			     sizeof(*ev) + data_len);
+	if (!skb)
+		return -ENOMEM;
+
+	ev = skb_put(skb, sizeof(*ev));
+	ev->quality_spec = quality_spec;
+	ev->data_len = data_len;
+	skb_put_data(skb, data, data_len);
+
+	return mgmt_event_skb(skb, NULL);
+}
+
 static int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
 			    u16 data_len)
 {
diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
index f43994523b1f..c003e94faccd 100644
--- a/net/bluetooth/msft.c
+++ b/net/bluetooth/msft.c
@@ -116,6 +116,20 @@  bool msft_monitor_supported(struct hci_dev *hdev)
 	return !!(msft_get_features(hdev) & MSFT_FEATURE_MASK_LE_ADV_MONITOR);
 }
 
+/* Add the MSFT vendor event subcodes into MSFT_SUBCODES which
+ * msft_vendor_evt() is interested in handling.
+ */
+static unsigned char MSFT_SUBCODES[] = { MSFT_EV_LE_MONITOR_DEVICE };
+static struct ext_vendor_prefix msft_ext_prefix = { 0 };
+
+static void set_ext_prefix(struct msft_data *msft)
+{
+	msft_ext_prefix.prefix = msft->evt_prefix;
+	msft_ext_prefix.prefix_len = msft->evt_prefix_len;
+	msft_ext_prefix.subcodes = MSFT_SUBCODES;
+	msft_ext_prefix.subcodes_len = sizeof(MSFT_SUBCODES);
+}
+
 static bool read_supported_features(struct hci_dev *hdev,
 				    struct msft_data *msft)
 {
@@ -156,6 +170,8 @@  static bool read_supported_features(struct hci_dev *hdev,
 	if (msft->features & MSFT_FEATURE_MASK_CURVE_VALIDITY)
 		hdev->msft_curve_validity = true;
 
+	set_ext_prefix(msft);
+
 	kfree_skb(skb);
 	return true;
 
@@ -742,7 +758,17 @@  static void msft_monitor_device_evt(struct hci_dev *hdev, struct sk_buff *skb)
 				 handle_data->mgmt_handle);
 }
 
-void msft_vendor_evt(struct hci_dev *hdev, void *data, struct sk_buff *skb)
+struct ext_vendor_prefix *msft_get_ext_prefix(struct hci_dev *hdev)
+{
+	struct msft_data *msft = hdev->msft_data;
+
+	if (!msft)
+		return NULL;
+
+	return &msft_ext_prefix;
+}
+
+void msft_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	struct msft_data *msft = hdev->msft_data;
 	u8 *evt_prefix;
diff --git a/net/bluetooth/msft.h b/net/bluetooth/msft.h
index afcaf7d3b1cb..1515ae06c628 100644
--- a/net/bluetooth/msft.h
+++ b/net/bluetooth/msft.h
@@ -17,7 +17,7 @@  void msft_register(struct hci_dev *hdev);
 void msft_unregister(struct hci_dev *hdev);
 void msft_do_open(struct hci_dev *hdev);
 void msft_do_close(struct hci_dev *hdev);
-void msft_vendor_evt(struct hci_dev *hdev, void *data, struct sk_buff *skb);
+void msft_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb);
 __u64 msft_get_features(struct hci_dev *hdev);
 int msft_add_monitor_pattern(struct hci_dev *hdev, struct adv_monitor *monitor);
 int msft_remove_monitor(struct hci_dev *hdev, struct adv_monitor *monitor,
@@ -27,6 +27,7 @@  int msft_set_filter_enable(struct hci_dev *hdev, bool enable);
 int msft_suspend_sync(struct hci_dev *hdev);
 int msft_resume_sync(struct hci_dev *hdev);
 bool msft_curve_validity(struct hci_dev *hdev);
+struct ext_vendor_prefix *msft_get_ext_prefix(struct hci_dev *hdev);
 
 #else
 
@@ -39,8 +40,7 @@  static inline void msft_register(struct hci_dev *hdev) {}
 static inline void msft_unregister(struct hci_dev *hdev) {}
 static inline void msft_do_open(struct hci_dev *hdev) {}
 static inline void msft_do_close(struct hci_dev *hdev) {}
-static inline void msft_vendor_evt(struct hci_dev *hdev, void *data,
-				   struct sk_buff *skb) {}
+static inline void msft_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb) {}
 static inline __u64 msft_get_features(struct hci_dev *hdev) { return 0; }
 static inline int msft_add_monitor_pattern(struct hci_dev *hdev,
 					   struct adv_monitor *monitor)
@@ -77,4 +77,10 @@  static inline bool msft_curve_validity(struct hci_dev *hdev)
 	return false;
 }
 
+static inline struct ext_vendor_prefix *
+msft_get_ext_prefix(struct hci_dev *hdev)
+{
+	return NULL;
+}
+
 #endif