Bug 280039 - bluetooth socket security filter incomplete initialization
Summary: bluetooth socket security filter incomplete initialization
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: Ryan Libby
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-06-28 17:04 UTC by Ryan Libby
Modified: 2024-07-01 15:39 UTC (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ryan Libby freebsd_committer freebsd_triage 2024-06-28 17:04:49 UTC
During buildkernel with gcc13 on amd64, gcc issues this warning:

In file included from /usr/src/freebsd/sys/netgraph/bluetooth/socket/ng_btsocket_hci_raw.c:36:
/usr/src/freebsd/sys/netgraph/bluetooth/socket/ng_btsocket_hci_raw.c: In function 'ng_btsocket_hci_raw_init':
/usr/src/freebsd/sys/sys/systm.h:282:33: warning: 'memset' used with length equal to number of elements without multiplication by element size [-Wmemset-elt-size]
  282 | #define memset(buf, c, len)     __builtin_memset((buf), (c), (len))
      |                                 ^~~~~~~~~~~~~~~~
/usr/src/freebsd/sys/netgraph/bluetooth/socket/ng_btsocket_hci_raw.c:810:9: note: in expansion of macro 'memset'
  810 |         memset(&ng_btsocket_hci_raw_sec_filter->events, 0xff,
      |         ^~~~~~

This appears to be a genuine bug in initialization of the bluetooth
socket events security filter.

https://cgit.freebsd.org/src/tree/sys/netgraph/bluetooth/socket/ng_btsocket_hci_raw.c?id=7f7b4926a779845116913c85ecbb10527daeab02#n801
	/*
	 * XXX How paranoid can we get? 
	 *
	 * Initialize security filter. If bit is set in the mask then
	 * unprivileged socket is allowed to send (receive) this command
	 * (event).
	 */

	/* Enable all events */
	memset(&ng_btsocket_hci_raw_sec_filter->events, 0xff,
		sizeof(ng_btsocket_hci_raw_sec_filter->events)/
			sizeof(ng_btsocket_hci_raw_sec_filter->events[0]));

	/* Disable some critical events */
	f = ng_btsocket_hci_raw_sec_filter->events;
	bit_clear(f, NG_HCI_EVENT_RETURN_LINK_KEYS - 1);
	bit_clear(f, NG_HCI_EVENT_LINK_KEY_NOTIFICATION - 1);
	bit_clear(f, NG_HCI_EVENT_VENDOR - 1);

The effect of the obvious fix to actually set all bits to 1 would be to
permit additional events on unprivileged sockets.  It's unclear if this
is the desired policy.  Someone knowledgeable should take a look.

Additionally perhaps this initialization ought to be reversed to use
default deny with explicit allows.

I posted a phabricator review that naively follows the apparent intent
of the code:
https://reviews.freebsd.org/D45707
Comment 1 commit-hook freebsd_committer freebsd_triage 2024-07-01 15:25:11 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=f8a46de2dd481da2bf69747551db30ea453490d5

commit f8a46de2dd481da2bf69747551db30ea453490d5
Author:     Ryan Libby <rlibby@FreeBSD.org>
AuthorDate: 2024-07-01 15:22:31 +0000
Commit:     Ryan Libby <rlibby@FreeBSD.org>
CommitDate: 2024-07-01 15:22:31 +0000

    bluetooth socket sysinit: correct memset initialization

    gcc -Wmemset-elt-size diagnosed this.  The code was only initializing
    the first 1/sizeof(long) bytes.  On 64-bit systems, this would mean only
    events up to 0x20 were initialized.

    This effectively reverses the security policy for some events with
    higher ids, now permitting them on unprivileged sockets.  Two that are
    defined are NG_HCI_EVENT_LE (0x3e) and NG_HCI_EVENT_BT_LOGO (0xfe).

    PR:             280039
    Reviewed by:    imp
    Differential Revision:  https://reviews.freebsd.org/D45707

 sys/netgraph/bluetooth/socket/ng_btsocket_hci_raw.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)