Bluetooth: hci_sock: fix slab oob read in create_monitor_event

Commit Message

Edward AD Oct. 10, 2023, 5:36 a.m. UTC

When accessing hdev->name, the actual string length should prevail

Reported-by: [e-mail]
Fixes: dcda165706b9 ("Bluetooth: hci_core: Fix build warnings")
Signed-off-by: Edward AD <[e-mail]>
---
 net/bluetooth/hci_sock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

[e-mail] Oct. 10, 2023, 6:43 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=791644

---Test result---

Test Summary:
CheckPatch                    FAIL      1.00 seconds
GitLint                       PASS      0.28 seconds
SubjectPrefix                 PASS      0.08 seconds
BuildKernel                   PASS      39.81 seconds
CheckAllWarning               PASS      43.46 seconds
CheckSparse                   PASS      49.12 seconds
CheckSmatch                   PASS      132.50 seconds
BuildKernel32                 PASS      38.56 seconds
TestRunnerSetup               PASS      589.07 seconds
TestRunner_l2cap-tester       PASS      35.77 seconds
TestRunner_iso-tester         PASS      79.65 seconds
TestRunner_bnep-tester        PASS      12.49 seconds
TestRunner_mgmt-tester        FAIL      256.84 seconds
TestRunner_rfcomm-tester      PASS      19.08 seconds
TestRunner_sco-tester         PASS      22.05 seconds
TestRunner_ioctl-tester       PASS      21.62 seconds
TestRunner_mesh-tester        PASS      16.19 seconds
TestRunner_smp-tester         PASS      16.90 seconds
TestRunner_userchan-tester    PASS      13.17 seconds
IncrementalBuild              PASS      36.19 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
Bluetooth: hci_sock: fix slab oob read in create_monitor_event
WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report
#89: 
Reported-by: [e-mail]
Fixes: dcda165706b9 ("Bluetooth: hci_core: Fix build warnings")

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.

/github/workspace/src/src/13414808.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.


##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 497, Passed: 496 (99.8%), Failed: 1, Not Run: 0

Failed Test Cases
LL Privacy - Add Device 7 (AL is full)               Failed       0.512 seconds


---
Regards,
Linux Bluetooth

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

Hello:

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

On Tue, 10 Oct 2023 13:36:57 +0800 you wrote:
> When accessing hdev->name, the actual string length should prevail
> 
> Reported-by: [e-mail]
> Fixes: dcda165706b9 ("Bluetooth: hci_core: Fix build warnings")
> Signed-off-by: Edward AD <[e-mail]>
> ---
>  net/bluetooth/hci_sock.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Here is the summary with links:
  - Bluetooth: hci_sock: fix slab oob read in create_monitor_event
    https://git.kernel.org/bluetooth/bluetooth-next/c/78480de55a96

You are awesome, thank you!

Kees Cook Oct. 11, 2023, 4:20 p.m. UTC | #3

On Tue, Oct 10, 2023 at 01:36:57PM +0800, Edward AD wrote:
> When accessing hdev->name, the actual string length should prevail
> 
> Reported-by: [e-mail]
> Fixes: dcda165706b9 ("Bluetooth: hci_core: Fix build warnings")
> Signed-off-by: Edward AD <[e-mail]>
> ---
>  net/bluetooth/hci_sock.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
> index 5e4f718073b7..72abe54c45dd 100644
> --- a/net/bluetooth/hci_sock.c
> +++ b/net/bluetooth/hci_sock.c
> @@ -488,7 +488,7 @@ static struct sk_buff *create_monitor_event(struct hci_dev *hdev, int event)
>  		ni->type = hdev->dev_type;
>  		ni->bus = hdev->bus;
>  		bacpy(&ni->bdaddr, &hdev->bdaddr);
> -		memcpy(ni->name, hdev->name, 8);
> +		memcpy(ni->name, hdev->name, strlen(hdev->name));

Uh, what's going on here?

hdev is:

struct hci_dev {
	...
        const char      *name;

ni is:

struct hci_mon_new_index {
        char            name[8];

You can't use "strlen" here in the case that "hdev->name" is larger than
8 bytes.

Also, why memcpy() and not strscpy()? Is this supposed to be padded out
with %NUL bytes? It appears to be sent over the network, so "yes" seems
to be the safe answer.

Should ni->name be always %NUL terminated? That I can't tell for sure,
but I assume "no", because the solution was to explicitly copy all the
bytes _except_ the %NUL byte (using strlen).

struct hci_mon_new_index's "name" should be marked __nonstring, and
instead strtomem_pad() should be used instead of memcpy.

-Kees

>  
>  		opcode = cpu_to_le16(HCI_MON_NEW_INDEX);
>  		break;
> -- 
> 2.25.1
>

Patch

diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index 5e4f718073b7..72abe54c45dd 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -488,7 +488,7 @@  static struct sk_buff *create_monitor_event(struct hci_dev *hdev, int event)
 		ni->type = hdev->dev_type;
 		ni->bus = hdev->bus;
 		bacpy(&ni->bdaddr, &hdev->bdaddr);
-		memcpy(ni->name, hdev->name, 8);
+		memcpy(ni->name, hdev->name, strlen(hdev->name));
 
 		opcode = cpu_to_le16(HCI_MON_NEW_INDEX);
 		break;