Re: [PATCH 1/7] Bluetooth: add a quirk for controllers that cannot enable Secure Connections

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Bluetooth: unmasked lmp_sc_capable() with HCI_QUIRK_BROKEN_WRITE_SC_SUPPORT
- [Low] Bluetooth: inaccurate comment for HCI_QUIRK_BROKEN_WRITE_SC_SUPPORT in hci.h
--

commit 300bedb0d4e07bfd158958e39e058f268b697a0b
Author: Christian Stefan Kövér-Draxl <ckoever@outlook.de>

Bluetooth: add a quirk for controllers that cannot enable Secure Connections

This commit introduces HCI_QUIRK_BROKEN_WRITE_SC_SUPPORT to skip the
HCI_OP_WRITE_SC_SUPPORT command on controllers that advertise BR/EDR Secure
Connections in their extended features but reject enabling host support. It
also sets this quirk in the Amlogic Bluetooth driver (hci_aml).

> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 1641d879dbda9..bcbe76c84d1b5 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -375,6 +375,20 @@ enum {
>  	 */
>  	HCI_QUIRK_BROKEN_READ_PAGE_SCAN_TYPE,
>  
> +	/*
> +	 * When this quirk is set, the HCI_OP_WRITE_SC_SUPPORT command is
> +	 * not supported.
> +	 *
> +	 * This quirk is intended for controllers that list BR/EDR Secure
> +	 * Connections among their extended features and then reject the
> +	 * command that turns host support on. Without it the failure takes
> +	 * the whole of hci_dev_open_sync() with it and the controller can
> +	 * never be powered on.
> +	 *
> +	 * This quirk must be set before hci_register_dev is called.
> +	 */
> +	HCI_QUIRK_BROKEN_WRITE_SC_SUPPORT,

[Severity: Low]
Is the comment stating that this quirk must be set before hci_register_dev
is called accurate?

In drivers/bluetooth/hci_aml.c, aml_serdev_probe() registers the HCI device
first via:

  aml_serdev_probe()
    -> hci_uart_register_device()
      -> hci_uart_register_device_priv()
        -> hci_register_dev()

and aml_setup() sets HCI_QUIRK_BROKEN_WRITE_SC_SUPPORT later inside the
hdev->setup callback during hci_dev_open_sync() -> hci_dev_init_sync() ->
hci_dev_setup_sync().

Should this comment note that the quirk can also be set in the hdev->setup
vendor callback?

> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index a92a81846e9df..f1dfef81ca961 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -3419,7 +3419,8 @@ int hci_write_sc_support_sync(struct hci_dev *hdev, u8 val)
>  {
>  	int err;
>  
> -	if (!bredr_sc_enabled(hdev) || lmp_host_sc_capable(hdev))
> +	if (!bredr_sc_enabled(hdev) || lmp_host_sc_capable(hdev) ||
> +	    hci_test_quirk(hdev, HCI_QUIRK_BROKEN_WRITE_SC_SUPPORT))
>  		return 0;
>  
>  	err = __hci_cmd_sync_status(hdev, HCI_OP_WRITE_SC_SUPPORT,
> @@ -5040,7 +5041,8 @@ static int hci_write_sc_support_1_sync(struct hci_dev *hdev)
>  	u8 support = 0x01;
>  
>  	if (!hci_dev_test_flag(hdev, HCI_SSP_ENABLED) ||
> -	    !bredr_sc_enabled(hdev))
> +	    !bredr_sc_enabled(hdev) ||
> +	    hci_test_quirk(hdev, HCI_QUIRK_BROKEN_WRITE_SC_SUPPORT))
>  		return 0;

[Severity: High]
When HCI_QUIRK_BROKEN_WRITE_SC_SUPPORT is set, skipping
HCI_OP_WRITE_SC_SUPPORT in hci_write_sc_support_sync() and
hci_write_sc_support_1_sync() leaves LMP_HOST_SC unset on the controller,
while lmp_sc_capable(hdev) and bredr_sc_enabled(hdev) in
include/net/bluetooth/hci_core.h still evaluate to true:

  #define lmp_sc_capable(dev)     ((dev)->features[2][1] & LMP_SC)
  ...
  #define bredr_sc_enabled(dev)   (lmp_sc_capable(dev) && \
                                   hci_dev_test_flag(dev, HCI_SC_ENABLED))

When userspace (such as bluetoothd) issues MGMT_OP_SET_SECURE_CONN,
set_secure_conn() or set_secure_conn_sync() in net/bluetooth/mgmt.c sets
HCI_SC_ENABLED:

  set_secure_conn_sync()
    hci_dev_set_flag(hdev, HCI_SC_ENABLED);
    return hci_write_sc_support_sync(hdev, val);

Because hci_write_sc_support_sync() returns 0 under the quirk,
set_secure_conn_complete() keeps HCI_SC_ENABLED (and HCI_SC_ONLY if
requested) set, leaving bredr_sc_enabled(hdev) true even though BR/EDR
Secure Connections was never enabled on the controller.

Won't this cause callers of lmp_sc_capable() and bredr_sc_enabled() to
misbehave on BR/EDR links? For example:

- In net/bluetooth/mgmt.c, read_local_oob_data_sync(),
  read_local_oob_data_complete(), and read_local_oob_ext_data_complete()
  check bredr_sc_enabled(hdev) and issue HCI_OP_READ_LOCAL_OOB_EXT_DATA
  instead of HCI_OP_READ_LOCAL_OOB_DATA.

- In net/bluetooth/hci_event.c, bredr_oob_data_present() and
  hci_remote_oob_data_request_evt() check bredr_sc_enabled(hdev) and report
  P-256 OOB data or send HCI_OP_REMOTE_OOB_EXT_DATA_REPLY instead of
  HCI_OP_REMOTE_OOB_DATA_REPLY.

- In net/bluetooth/smp.c and net/bluetooth/l2cap_core.c, smp_register() and
  l2cap_conn_add() check bredr_sc_enabled(hdev) and advertise
  L2CAP_FC_SMP_BREDR to remote BR/EDR peers. When a peer initiates SMP
  pairing / Cross-Transport Key Derivation over BR/EDR,
  smp_cmd_pairing_req() rejects it with SMP_CROSS_TRANSP_NOT_ALLOWED
  because HCI_CONN_AES_CCM is not set on the link, and smp_recv_cb()
  disconnects the ACL connection with HCI_ERROR_AUTH_FAILURE.

- In net/bluetooth/mgmt.c and net/bluetooth/hci_debugfs.c,
  get_supported_settings(), set_secure_conn(), and
  hci_debugfs_create_bredr() check lmp_sc_capable(hdev) and treat BR/EDR
  Secure Connections as supported.

Should lmp_sc_capable(dev) in include/net/bluetooth/hci_core.h instead mask
out the feature bit when the quirk is set (similar to how le_coded_capable()
checks HCI_QUIRK_BROKEN_LE_CODED), so that both lmp_sc_capable() and
bredr_sc_enabled() evaluate to false across the stack while still allowing
HCI_SC_ENABLED for LE Secure Connections?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261025192338.209574-1-christian.koever-draxl@student.uibk.ac.at?part=1