[v2] Bluetooth: avoid memcmp() out of bounds warning

Commit Message

Arnd Bergmann Oct. 9, 2023, 8:31 p.m. UTC

From: Arnd Bergmann <[e-mail]>

bacmp() is a wrapper around memcpy(), which contain compile-time
checks for buffer overflow. Since the hci_conn_request_evt() also calls
bt_dev_dbg() with an implicit NULL pointer check, the compiler is now
aware of a case where 'hdev' is NULL and treats this as meaning that
zero bytes are available:

In file included from net/bluetooth/hci_event.c:32:
In function 'bacmp',
    inlined from 'hci_conn_request_evt' at net/bluetooth/hci_event.c:3276:7:
include/net/bluetooth/bluetooth.h:364:16: error: 'memcmp' specified bound 6 exceeds source size 0 [-Werror=stringop-overread]
  364 |         return memcmp(ba1, ba2, sizeof(bdaddr_t));
      |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Add another NULL pointer check before the bacmp() to ensure the compiler
understands the code flow enough to not warn about it.  Since the patch
that introduced the warning is marked for stable backports, this one
should also go that way to avoid introducing build regressions.

Fixes: d70e44fef8621 ("Bluetooth: Reject connection with the device which has same BD_ADDR")
Cc: Kees Cook <[e-mail]>
Cc: "Lee, Chun-Yi" <[e-mail]>
Cc: Luiz Augusto von Dentz <[e-mail]>
Cc: Marcel Holtmann <[e-mail]>
Cc: [e-mail]
Signed-off-by: Arnd Bergmann <[e-mail]>
---
v2: rewrite completely
---
 net/bluetooth/hci_event.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Kees Cook Oct. 9, 2023, 8:43 p.m. UTC | #1

On Mon, Oct 09, 2023 at 10:31:31PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <[e-mail]>
> 
> bacmp() is a wrapper around memcpy(), which contain compile-time
> checks for buffer overflow. Since the hci_conn_request_evt() also calls
> bt_dev_dbg() with an implicit NULL pointer check, the compiler is now
> aware of a case where 'hdev' is NULL and treats this as meaning that
> zero bytes are available:
> 
> In file included from net/bluetooth/hci_event.c:32:
> In function 'bacmp',
>     inlined from 'hci_conn_request_evt' at net/bluetooth/hci_event.c:3276:7:
> include/net/bluetooth/bluetooth.h:364:16: error: 'memcmp' specified bound 6 exceeds source size 0 [-Werror=stringop-overread]
>   364 |         return memcmp(ba1, ba2, sizeof(bdaddr_t));
>       |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Add another NULL pointer check before the bacmp() to ensure the compiler
> understands the code flow enough to not warn about it.  Since the patch
> that introduced the warning is marked for stable backports, this one
> should also go that way to avoid introducing build regressions.
> 
> Fixes: d70e44fef8621 ("Bluetooth: Reject connection with the device which has same BD_ADDR")
> Cc: Kees Cook <[e-mail]>
> Cc: "Lee, Chun-Yi" <[e-mail]>
> Cc: Luiz Augusto von Dentz <[e-mail]>
> Cc: Marcel Holtmann <[e-mail]>
> Cc: [e-mail]
> Signed-off-by: Arnd Bergmann <[e-mail]>

A weird side-effect of the NULL check, but not unreasonable. :)

Reviewed-by: Kees Cook <[e-mail]>

[e-mail] Oct. 9, 2023, 9:06 p.m. UTC | #2

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=791522

---Test result---

Test Summary:
CheckPatch                    FAIL      1.02 seconds
GitLint                       FAIL      0.61 seconds
SubjectPrefix                 PASS      0.06 seconds
BuildKernel                   PASS      41.00 seconds
CheckAllWarning               PASS      45.04 seconds
CheckSparse                   WARNING   51.19 seconds
CheckSmatch                   WARNING   136.82 seconds
BuildKernel32                 PASS      39.85 seconds
TestRunnerSetup               PASS      607.10 seconds
TestRunner_l2cap-tester       PASS      35.76 seconds
TestRunner_iso-tester         FAIL      82.33 seconds
TestRunner_bnep-tester        PASS      12.70 seconds
TestRunner_mgmt-tester        FAIL      253.22 seconds
TestRunner_rfcomm-tester      PASS      19.38 seconds
TestRunner_sco-tester         PASS      22.86 seconds
TestRunner_ioctl-tester       PASS      22.32 seconds
TestRunner_mesh-tester        PASS      16.06 seconds
TestRunner_smp-tester         PASS      16.98 seconds
TestRunner_userchan-tester    PASS      13.48 seconds
IncrementalBuild              PASS      37.88 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[v2] Bluetooth: avoid memcmp() out of bounds warning
WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit description?)
#68: 
    inlined from 'hci_conn_request_evt' at net/bluetooth/hci_event.c:3276:7:

WARNING: Please use correct Fixes: style 'Fixes: <12 chars of sha1> ("<title line>")' - ie: 'Fixes:  ("Bluetooth: Reject connection with the device which has same BD_ADDR")'
#78: 
Fixes: d70e44fef8621 ("Bluetooth: Reject connection with the device which has same BD_ADDR")

total: 0 errors, 2 warnings, 0 checks, 8 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13414405.patch has style problems, please review.

NOTE: Ignored message types: UNKNOWN_COMMIT_ID

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


Use of uninitialized value $cid in concatenation (.) or string at /github/workspace/src/src/scripts/checkpatch.pl line 3228.
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[v2] Bluetooth: avoid memcmp() out of bounds warning

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
14: B1 Line exceeds max length (125>80): "include/net/bluetooth/bluetooth.h:364:16: error: 'memcmp' specified bound 6 exceeds source size 0 [-Werror=stringop-overread]"
##############################
Test: CheckSparse - WARNING
Desc: Run sparse tool with linux kernel
Output:
net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):
##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):
##############################
Test: TestRunner_iso-tester - FAIL
Desc: Run iso-tester with test-runner
Output:
Total: 99, Passed: 98 (99.0%), Failed: 1, Not Run: 0

Failed Test Cases
ISO Connect Suspend - Success                        Failed       6.548 seconds
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 497, Passed: 495 (99.6%), Failed: 2, Not Run: 0

Failed Test Cases
Pairing Acceptor - SMP over BR/EDR 2                 Timed out    2.663 seconds
LL Privacy - Start Discovery 2 (Disable RL)          Failed       0.527 seconds


---
Regards,
Linux Bluetooth

[e-mail] Oct. 10, 2023, 6:40 p.m. UTC | #3

Hello:

This patch was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <[e-mail]>:

On Mon,  9 Oct 2023 22:31:31 +0200 you wrote:
> From: Arnd Bergmann <[e-mail]>
> 
> bacmp() is a wrapper around memcpy(), which contain compile-time
> checks for buffer overflow. Since the hci_conn_request_evt() also calls
> bt_dev_dbg() with an implicit NULL pointer check, the compiler is now
> aware of a case where 'hdev' is NULL and treats this as meaning that
> zero bytes are available:
> 
> [...]

Here is the summary with links:
  - [v2] Bluetooth: avoid memcmp() out of bounds warning
    https://git.kernel.org/bluetooth/bluetooth-next/c/b8ba8e65e84b

You are awesome, thank you!

Patch

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 6f4409b4c3648..9b34c9f8ee02c 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -3273,7 +3273,7 @@  static void hci_conn_request_evt(struct hci_dev *hdev, void *data,
 	/* Reject incoming connection from device with same BD ADDR against
 	 * CVE-2020-26555
 	 */
-	if (!bacmp(&hdev->bdaddr, &ev->bdaddr)) {
+	if (hdev && !bacmp(&hdev->bdaddr, &ev->bdaddr)) {
 		bt_dev_dbg(hdev, "Reject connection with same BD_ADDR %pMR\n",
 			   &ev->bdaddr);
 		hci_reject_conn(hdev, &ev->bdaddr);