diff mbox series

Bluetooth: mgmt: Pessimize compile-time bounds-check

Message ID 20210818043912.1466447-1-keescook@chromium.org (mailing list archive)
State New, archived
Headers show
Series Bluetooth: mgmt: Pessimize compile-time bounds-check | expand

Commit Message

Kees Cook Aug. 18, 2021, 4:39 a.m. UTC
After gaining __alloc_size hints, GCC thinks it can reach a memcpy()
with eir_len == 0 (since it can't see into the rewrite of status).
Instead, check eir_len == 0, avoiding this future warning:

In function 'eir_append_data',
    inlined from 'read_local_oob_ext_data_complete' at net/bluetooth/mgmt.c:7210:12:
./include/linux/fortify-string.h:54:29: warning: '__builtin_memcpy' offset 5 is out of the bounds [0, 3] [-Warray-bounds]
...
net/bluetooth/hci_request.h:133:2: note: in expansion of macro 'memcpy'
  133 |  memcpy(&eir[eir_len], data, data_len);
      |  ^~~~~~

Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: Johan Hedberg <johan.hedberg@gmail.com>
Cc: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: linux-bluetooth@vger.kernel.org
Cc: netdev@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 net/bluetooth/mgmt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

bluez.test.bot@gmail.com Aug. 18, 2021, 5:16 a.m. UTC | #1
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=533133

---Test result---

Test Summary:
CheckPatch                    FAIL      0.46 seconds
GitLint                       FAIL      0.11 seconds
BuildKernel                   PASS      536.10 seconds
TestRunner: Setup             PASS      347.74 seconds
TestRunner: l2cap-tester      PASS      2.56 seconds
TestRunner: bnep-tester       PASS      1.90 seconds
TestRunner: mgmt-tester       PASS      30.68 seconds
TestRunner: rfcomm-tester     PASS      2.08 seconds
TestRunner: sco-tester        PASS      2.02 seconds
TestRunner: smp-tester        FAIL      2.06 seconds
TestRunner: userchan-tester   PASS      1.98 seconds

Details
##############################
Test: CheckPatch - FAIL - 0.46 seconds
Run checkpatch.pl script with rule in .checkpatch.conf
Bluetooth: mgmt: Pessimize compile-time bounds-check
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#11: 
    inlined from 'read_local_oob_ext_data_complete' at net/bluetooth/mgmt.c:7210:12:

total: 0 errors, 1 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.

"[PATCH] Bluetooth: mgmt: Pessimize compile-time bounds-check" has style problems, please review.

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


##############################
Test: GitLint - FAIL - 0.11 seconds
Run gitlint with rule in .gitlint
Bluetooth: mgmt: Pessimize compile-time bounds-check
8: B1 Line exceeds max length (84>80): "    inlined from 'read_local_oob_ext_data_complete' at net/bluetooth/mgmt.c:7210:12:"
9: B1 Line exceeds max length (121>80): "./include/linux/fortify-string.h:54:29: warning: '__builtin_memcpy' offset 5 is out of the bounds [0, 3] [-Warray-bounds]"


##############################
Test: BuildKernel - PASS - 536.10 seconds
Build Kernel with minimal configuration supports Bluetooth


##############################
Test: TestRunner: Setup - PASS - 347.74 seconds
Setup environment for running Test Runner


##############################
Test: TestRunner: l2cap-tester - PASS - 2.56 seconds
Run test-runner with l2cap-tester
Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: bnep-tester - PASS - 1.90 seconds
Run test-runner with bnep-tester
Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: mgmt-tester - PASS - 30.68 seconds
Run test-runner with mgmt-tester
Total: 448, Passed: 445 (99.3%), Failed: 0, Not Run: 3

##############################
Test: TestRunner: rfcomm-tester - PASS - 2.08 seconds
Run test-runner with rfcomm-tester
Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: sco-tester - PASS - 2.02 seconds
Run test-runner with sco-tester
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

##############################
Test: TestRunner: smp-tester - FAIL - 2.06 seconds
Run test-runner with smp-tester
Total: 8, Passed: 7 (87.5%), Failed: 1, Not Run: 0

Failed Test Cases
SMP Client - SC Request 2                            Failed       0.021 seconds

##############################
Test: TestRunner: userchan-tester - PASS - 1.98 seconds
Run test-runner with userchan-tester
Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0



---
Regards,
Linux Bluetooth
Kees Cook Aug. 18, 2021, 6:20 a.m. UTC | #2
On Tue, Aug 17, 2021 at 10:16:53PM -0700, bluez.test.bot@gmail.com wrote:
> This is automated email and please do not reply to this email!

... I think I will though. :)

> 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=533133
> 
> ---Test result---
> 
> Test Summary:
> CheckPatch                    FAIL      0.46 seconds
> GitLint                       FAIL      0.11 seconds
> BuildKernel                   PASS      536.10 seconds
> TestRunner: Setup             PASS      347.74 seconds
> TestRunner: l2cap-tester      PASS      2.56 seconds
> TestRunner: bnep-tester       PASS      1.90 seconds
> TestRunner: mgmt-tester       PASS      30.68 seconds
> TestRunner: rfcomm-tester     PASS      2.08 seconds
> TestRunner: sco-tester        PASS      2.02 seconds
> TestRunner: smp-tester        FAIL      2.06 seconds
> TestRunner: userchan-tester   PASS      1.98 seconds
> 
> Details
> ##############################
> Test: CheckPatch - FAIL - 0.46 seconds
> Run checkpatch.pl script with rule in .checkpatch.conf
> Bluetooth: mgmt: Pessimize compile-time bounds-check
> WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
> #11: 
>     inlined from 'read_local_oob_ext_data_complete' at net/bluetooth/mgmt.c:7210:12:

This is a literal gcc warning output, so wrapping shouldn't happen.

> 
> total: 0 errors, 1 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.
> 
> "[PATCH] Bluetooth: mgmt: Pessimize compile-time bounds-check" has style problems, please review.
> 
> NOTE: If any of the errors are false positives, please report
>       them to the maintainer, see CHECKPATCH in MAINTAINERS.
> 
> 
> ##############################
> Test: GitLint - FAIL - 0.11 seconds
> Run gitlint with rule in .gitlint
> Bluetooth: mgmt: Pessimize compile-time bounds-check
> 8: B1 Line exceeds max length (84>80): "    inlined from 'read_local_oob_ext_data_complete' at net/bluetooth/mgmt.c:7210:12:"
> 9: B1 Line exceeds max length (121>80): "./include/linux/fortify-string.h:54:29: warning: '__builtin_memcpy' offset 5 is out of the bounds [0, 3] [-Warray-bounds]"

Same.

> 
> 
> ##############################
> Test: BuildKernel - PASS - 536.10 seconds
> Build Kernel with minimal configuration supports Bluetooth
> 
> 
> ##############################
> Test: TestRunner: Setup - PASS - 347.74 seconds
> Setup environment for running Test Runner
> 
> 
> ##############################
> Test: TestRunner: l2cap-tester - PASS - 2.56 seconds
> Run test-runner with l2cap-tester
> Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0
> 
> ##############################
> Test: TestRunner: bnep-tester - PASS - 1.90 seconds
> Run test-runner with bnep-tester
> Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0
> 
> ##############################
> Test: TestRunner: mgmt-tester - PASS - 30.68 seconds
> Run test-runner with mgmt-tester
> Total: 448, Passed: 445 (99.3%), Failed: 0, Not Run: 3
> 
> ##############################
> Test: TestRunner: rfcomm-tester - PASS - 2.08 seconds
> Run test-runner with rfcomm-tester
> Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0
> 
> ##############################
> Test: TestRunner: sco-tester - PASS - 2.02 seconds
> Run test-runner with sco-tester
> Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0
> 
> ##############################
> Test: TestRunner: smp-tester - FAIL - 2.06 seconds
> Run test-runner with smp-tester
> Total: 8, Passed: 7 (87.5%), Failed: 1, Not Run: 0
> 
> Failed Test Cases
> SMP Client - SC Request 2                            Failed       0.021 seconds

? Any details on this?

> 
> ##############################
> Test: TestRunner: userchan-tester - PASS - 1.98 seconds
> Run test-runner with userchan-tester
> Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0
> 
> 
> 
> ---
> Regards,
> Linux Bluetooth
>
Marcel Holtmann Aug. 19, 2021, 2:52 p.m. UTC | #3
Hi Kees,

> After gaining __alloc_size hints, GCC thinks it can reach a memcpy()
> with eir_len == 0 (since it can't see into the rewrite of status).
> Instead, check eir_len == 0, avoiding this future warning:
> 
> In function 'eir_append_data',
>    inlined from 'read_local_oob_ext_data_complete' at net/bluetooth/mgmt.c:7210:12:
> ./include/linux/fortify-string.h:54:29: warning: '__builtin_memcpy' offset 5 is out of the bounds [0, 3] [-Warray-bounds]
> ...
> net/bluetooth/hci_request.h:133:2: note: in expansion of macro 'memcpy'
>  133 |  memcpy(&eir[eir_len], data, data_len);
>      |  ^~~~~~
> 
> Cc: Marcel Holtmann <marcel@holtmann.org>
> Cc: Johan Hedberg <johan.hedberg@gmail.com>
> Cc: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: linux-bluetooth@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> net/bluetooth/mgmt.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel
diff mbox series

Patch

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 1e21e014efd2..cea01e275f1e 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -7204,7 +7204,7 @@  static void read_local_oob_ext_data_complete(struct hci_dev *hdev, u8 status,
 	if (!mgmt_rp)
 		goto done;
 
-	if (status)
+	if (eir_len == 0)
 		goto send_rsp;
 
 	eir_len = eir_append_data(mgmt_rp->eir, 0, EIR_CLASS_OF_DEV,