[PW_SID:1157499] [v2] Bluetooth: RFCOMM: defer security confirmation to krfcommd - #702
[PW_SID:1157499] [v2] Bluetooth: RFCOMM: defer security confirmation to krfcommd#702BluezTestBot wants to merge 6 commits into
Conversation
This patch adds workflow files for ci: [sync.yml] - The workflow file for scheduled work - Sync the repo with upstream repo and rebase the workflow branch - Review the patches in the patchwork and creates the PR if needed [ci.yml] - The workflow file for CI tasks - Run CI tests when PR is created Signed-off-by: Tedd Ho-Jeong An <tedd.an@intel.com>
This replaces the bzcafe action with bluez/action-ci so we can maintain everything in the github bluez organization Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
This attempts to sync every 5 minutes instead of 30. Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
bluez/action-ci uses master as default branch for workflow which is incorrect for kernel Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
The CI action now creates individual GitHub Check Runs per test, which requires 'checks: write' permission on the GITHUB_TOKEN. Also make the pull_request trigger types explicit to include 'reopened', allowing CI to be retriggered by closing and reopening a PR.
An RFCOMM connect() issued while a BR/EDR link is being authenticated
makes lockdep report a circular dependency, and the reported cycle is a
real AB/BA between rfcomm_mutex and hdev->lock.
rfcomm_security_cfm() is called from the HCI event path, which already
holds hdev->lock:
hci_rx_work()
hci_event_packet()
hci_cc_read_enc_key_size() [hdev->lock]
hci_encrypt_cfm() [hci_cb_list_lock]
rfcomm_security_cfm() [rfcomm_mutex]
while an RFCOMM connect() from userspace takes the same two locks the
other way round:
rfcomm_sock_connect()
rfcomm_dlc_open() [rfcomm_mutex]
__rfcomm_dlc_open()
rfcomm_session_create()
kernel_connect()
l2cap_sock_connect()
l2cap_chan_connect() [hdev->lock]
WARNING: possible circular locking dependency detected
kworker/u131:1/1128 is trying to acquire lock:
rfcomm_mutex, at: rfcomm_security_cfm+0x31/0x3e0 [rfcomm]
but task is already holding lock:
hci_cb_list_lock, at: hci_cc_read_enc_key_size+0x1d2/0xcc0
Chain exists of:
rfcomm_mutex --> &hdev->lock --> hci_cb_list_lock
hci_auth_complete_evt() and hci_encrypt_change_evt() reach the callback
the same way.
Both orders have to be seen in the same boot, which is why a BR/EDR
connection alone is not enough to show it: a session set up by the
remote side is created by rfcomm_accept_connection() in krfcommd, which
calls kernel_accept() and never takes hdev->lock under rfcomm_mutex.
Connecting a device that authenticates and encrypts the link and then
calling connect() on an RFCOMM socket towards any address - the connect
does not have to succeed, the order is recorded before the page timeout
- reports it every time.
The callback does not have to run in the HCI event context at all: it
only updates DLC flags and timers that krfcommd consumes in
rfcomm_process_dlcs(), and it already ends with rfcomm_schedule(). So
queue the confirmation instead of taking rfcomm_mutex from the HCI
event path, and let krfcommd apply it under rfcomm_mutex on its next
pass, ahead of session processing.
The queued entry pins both the connection and the controller, and
krfcommd takes hdev->lock while applying it, so the lookup and
hci_conn_check_secure() run in the same context as before. A session
that was torn down and set up again while the confirmation was queued
runs over a different hci_conn and is skipped. A confirmation that
cannot be allocated is dropped and the DLC closes on its auth timeout.
Fixes: 759c185d0bbd ("Bluetooth: RFCOMM: serialize security confirmation handling")
Reported-by: Pauli Virtanen <pav@iki.fi>
Closes: https://lore.kernel.org/linux-bluetooth/5e76a95e934e451e7006db28827c2d64af5a88be.camel@iki.fi/
Reported-by: syzbot+74071deb72339c215b2e@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/linux-bluetooth/6a92fadc.08e933ee.dbf97.008f.GAE@google.com/
Cc: stable@vger.kernel.org
Signed-off-by: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
|
CheckPatch |
|
VerifyFixes |
|
VerifySignedoff |
|
GitLint |
|
SubjectPrefix |
|
BuildKernel |
|
CheckAllWarning |
|
CheckSparse |
|
BuildKernel32 |
|
CheckKernelLLVM |
|
TestRunnerSetup |
|
TestRunner_rfcomm-tester |
|
IncrementalBuild |
8f53a45 to
d690686
Compare
An RFCOMM connect() issued while a BR/EDR link is being authenticated
makes lockdep report a circular dependency, and the reported cycle is a
real AB/BA between rfcomm_mutex and hdev->lock.
rfcomm_security_cfm() is called from the HCI event path, which already
holds hdev->lock:
hci_rx_work()
hci_event_packet()
hci_cc_read_enc_key_size() [hdev->lock]
hci_encrypt_cfm() [hci_cb_list_lock]
rfcomm_security_cfm() [rfcomm_mutex]
while an RFCOMM connect() from userspace takes the same two locks the
other way round:
rfcomm_sock_connect()
rfcomm_dlc_open() [rfcomm_mutex]
__rfcomm_dlc_open()
rfcomm_session_create()
kernel_connect()
l2cap_sock_connect()
l2cap_chan_connect() [hdev->lock]
WARNING: possible circular locking dependency detected
kworker/u131:1/1128 is trying to acquire lock:
rfcomm_mutex, at: rfcomm_security_cfm+0x31/0x3e0 [rfcomm]
but task is already holding lock:
hci_cb_list_lock, at: hci_cc_read_enc_key_size+0x1d2/0xcc0
Chain exists of:
rfcomm_mutex --> &hdev->lock --> hci_cb_list_lock
hci_auth_complete_evt() and hci_encrypt_change_evt() reach the callback
the same way.
Both orders have to be seen in the same boot, which is why a BR/EDR
connection alone is not enough to show it: a session set up by the
remote side is created by rfcomm_accept_connection() in krfcommd, which
calls kernel_accept() and never takes hdev->lock under rfcomm_mutex.
Connecting a device that authenticates and encrypts the link and then
calling connect() on an RFCOMM socket towards any address - the connect
does not have to succeed, the order is recorded before the page timeout
The callback does not have to run in the HCI event context at all: it
only updates DLC flags and timers that krfcommd consumes in
rfcomm_process_dlcs(), and it already ends with rfcomm_schedule(). So
queue the confirmation instead of taking rfcomm_mutex from the HCI
event path, and let krfcommd apply it under rfcomm_mutex on its next
pass, ahead of session processing.
The queued entry pins both the connection and the controller, and
krfcommd takes hdev->lock while applying it, so the lookup and
hci_conn_check_secure() run in the same context as before. A session
that was torn down and set up again while the confirmation was queued
runs over a different hci_conn and is skipped. A confirmation that
cannot be allocated is dropped and the DLC closes on its auth timeout.
Fixes: 759c185d0bbd ("Bluetooth: RFCOMM: serialize security confirmation handling")
Reported-by: Pauli Virtanen pav@iki.fi
Closes: https://lore.kernel.org/linux-bluetooth/5e76a95e934e451e7006db28827c2d64af5a88be.camel@iki.fi/
Reported-by: syzbot+74071deb72339c215b2e@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/linux-bluetooth/6a92fadc.08e933ee.dbf97.008f.GAE@google.com/
Cc: stable@vger.kernel.org
Signed-off-by: Mikhail Gavrilov mikhail.v.gavrilov@gmail.com
The commit this fixes is in v7.3-rc1 and is marked for stable, so this
probably wants the bluetooth fixes tree rather than -next.
v1: https://lore.kernel.org/linux-bluetooth/20260902235132.453044-1-mikhail.v.gavrilov@gmail.com/
v2:
hci_unregister_cb(), instead of at the end of rfcomm_run(); the init
error path stops the thread before unregistering the callback, so
the old placement leaked there
applied, so conn->sec_level is read in the same context as before
confirmation was reported for
__rfcomm_security_cfm(); not verified with clang, done by inspection
Tested on 7.3.0-rc1 with an MT7922 controller (btusb). Without the
patch the steps above report the inversion on every run. With v2
applied the reproducer leaves the validator armed and silent
(debug_locks: 1), and a 2.5 hour session with three BR/EDR headsets
(soundcore Liberty 5, FIIO UTWS17, JBL Tour Pro 3), HFP/SCO audio and
AVRCP produced no lockdep report.
The connect() side used for testing, so that it does not depend on which
end sets up the HFP session:
#include <stdint.h>
#include <string.h>
#include <unistd.h>
#include <sys/socket.h>
#define BTPROTO_RFCOMM 3
struct sockaddr_rc {
unsigned short rc_family;
uint8_t rc_bdaddr[6]; /* little endian */
uint8_t rc_channel;
};
int main(void)
{
struct sockaddr_rc addr = { .rc_family = AF_BLUETOOTH,
.rc_channel = 1 };
int fd = socket(AF_BLUETOOTH, SOCK_STREAM, BTPROTO_RFCOMM);
}
net/bluetooth/rfcomm/core.c | 178 ++++++++++++++++++++++++++++--------
1 file changed, 140 insertions(+), 38 deletions(-)