diff mbox series

Bluetooth: hci_sync: Check the correct flag before starting a scan

Message ID 20240102180810.54515-1-verdre@v0yd.nl
State New
Headers
Series Bluetooth: hci_sync: Check the correct flag before starting a scan |

Commit Message

Jonas Dreßler Jan. 2, 2024, 6:08 p.m. UTC 
  There's a very confusing mistake in the code starting a HCI inquiry: We're
calling hci_dev_test_flag() to test for HCI_INQUIRY, but hci_dev_test_flag()
checks hdev->dev_flags instead of hdev->flags. HCI_INQUIRY is a bit that's
set on hdev->flags, not on hdev->dev_flags though.

HCI_INQUIRY equals the integer 7, and in hdev->dev_flags, 7 means
HCI_BONDABLE, so we were actually checking for HCI_BONDABLE here.

The mistake is only present in the synchronous code for starting an inquiry,
not in the async one. Also devices are typically bondable while doing an
inquiry, so that might be the reason why nobody noticed it so far.

Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
---
 net/bluetooth/hci_sync.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Simon Horman Jan. 4, 2024, 5:41 p.m. UTC | #1 
On Tue, Jan 02, 2024 at 07:08:08PM +0100, Jonas Dreßler wrote:
> There's a very confusing mistake in the code starting a HCI inquiry: We're
> calling hci_dev_test_flag() to test for HCI_INQUIRY, but hci_dev_test_flag()
> checks hdev->dev_flags instead of hdev->flags. HCI_INQUIRY is a bit that's
> set on hdev->flags, not on hdev->dev_flags though.
> 
> HCI_INQUIRY equals the integer 7, and in hdev->dev_flags, 7 means
> HCI_BONDABLE, so we were actually checking for HCI_BONDABLE here.
> 
> The mistake is only present in the synchronous code for starting an inquiry,
> not in the async one. Also devices are typically bondable while doing an
> inquiry, so that might be the reason why nobody noticed it so far.
> 
> Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>

FWIIW, I agree with this analysis and the proposed fix looks
correct to me.

Reviewed-by: Simon Horman <horms@kernel.org>

I do wonder if it is appropriate to treat this as a bug fix -
is there a use-visible problem? If so, the following seems appropriate to
me.

Fixes: abfeea476c68 ("Bluetooth: hci_sync: Convert MGMT_OP_START_DISCOVERY")

...
patchwork-bot+bluetooth@kernel.org Jan. 4, 2024, 10:20 p.m. UTC | #2 
Hello:

This patch was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Tue,  2 Jan 2024 19:08:08 +0100 you wrote:
> There's a very confusing mistake in the code starting a HCI inquiry: We're
> calling hci_dev_test_flag() to test for HCI_INQUIRY, but hci_dev_test_flag()
> checks hdev->dev_flags instead of hdev->flags. HCI_INQUIRY is a bit that's
> set on hdev->flags, not on hdev->dev_flags though.
> 
> HCI_INQUIRY equals the integer 7, and in hdev->dev_flags, 7 means
> HCI_BONDABLE, so we were actually checking for HCI_BONDABLE here.
> 
> [...]

Here is the summary with links:
  - Bluetooth: hci_sync: Check the correct flag before starting a scan
    https://git.kernel.org/bluetooth/bluetooth-next/c/626cef40faf0

You are awesome, thank you!
diff mbox series

Patch

diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index c920de0a2..4a5949a0e 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -5554,7 +5554,7 @@  static int hci_inquiry_sync(struct hci_dev *hdev, u8 length)
 
 	bt_dev_dbg(hdev, "");
 
-	if (hci_dev_test_flag(hdev, HCI_INQUIRY))
+	if (test_bit(HCI_INQUIRY, &hdev->flags))
 		return 0;
 
 	hci_dev_lock(hdev);