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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Joseph,

On Thu, May 26, 2022 at 9:37 PM Joseph Hwang <josephsih@xxxxxxxxxx> 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
> <luiz.dentz@xxxxxxxxx> wrote:
> >
> > Hi Joseph,
> >
> > On Thu, May 26, 2022 at 4:21 AM Joseph Hwang <josephsih@xxxxxxxxxxxx> 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 <lkp@xxxxxxxxx>
> > >
> > > Signed-off-by: Joseph Hwang <josephsih@xxxxxxxxxxxx>
> > > Reviewed-by: Archie Pusaka <apusaka@xxxxxxxxxxxx>
> > > ---
> > >
> > > 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: josephsih@xxxxxxxxxx



-- 
Luiz Augusto von Dentz




[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux