Skip to content

Commit

Permalink
pw_bluetooth_sapphire: Hold PairingToken during BR/EDR legacy pairing
Browse files Browse the repository at this point in the history
Bug: 388607971
Change-Id: I03f0a4f61050b62be0b86ffd272158a6cc165266
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/259414
Reviewed-by: Faraaz Sareshwala <[email protected]>
Docs-Not-Needed: Ben Lawson <[email protected]>
Reviewed-by: Jason Graffius <[email protected]>
Lint: Lint 🤖 <[email protected]>
Commit-Queue: Ben Lawson <[email protected]>
  • Loading branch information
BenjaminLawson authored and CQ Bot Account committed Jan 15, 2025
1 parent 5264396 commit 664fbc4
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 13 deletions.
29 changes: 20 additions & 9 deletions pw_bluetooth_sapphire/host/gap/legacy_pairing_state.cc
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,8 @@ std::optional<hci_spec::LinkKey> LegacyPairingState::OnLinkKeyRequest() {
if (state_ == State::kIdle) {
if (link_key.has_value()) {
PW_CHECK(!is_pairing());
current_pairing_ = Pairing::MakeResponderForBonded();
current_pairing_ =
Pairing::MakeResponderForBonded(peer_->MutBrEdr().RegisterPairing());
state_ = State::kWaitEncryption;
return link_key->key();
}
Expand Down Expand Up @@ -292,7 +293,8 @@ void LegacyPairingState::OnPinCodeRequest(UserPinCodeCallback cb) {

if (state_ == State::kIdle) {
PW_CHECK(!is_pairing());
current_pairing_ = Pairing::MakeResponder(outgoing_connection_);
current_pairing_ = Pairing::MakeResponder(
outgoing_connection_, peer_->MutBrEdr().RegisterPairing());
}

PW_CHECK(pairing_delegate_.is_alive());
Expand Down Expand Up @@ -538,9 +540,12 @@ void LegacyPairingState::OnEncryptionChange(hci::Result<bool> result) {

std::unique_ptr<LegacyPairingState::Pairing>
LegacyPairingState::Pairing::MakeInitiator(
BrEdrSecurityRequirements security_requirements, bool outgoing_connection) {
BrEdrSecurityRequirements security_requirements,
bool outgoing_connection,
Peer::PairingToken&& token) {
// Private constructor is inaccessible to std::make_unique
std::unique_ptr<Pairing> pairing(new Pairing(outgoing_connection));
std::unique_ptr<Pairing> pairing(
new Pairing(outgoing_connection, std::move(token)));
pairing->initiator = true;
pairing->preferred_security = security_requirements;
return pairing;
Expand All @@ -549,9 +554,11 @@ LegacyPairingState::Pairing::MakeInitiator(
std::unique_ptr<LegacyPairingState::Pairing>
LegacyPairingState::Pairing::MakeResponder(
bool outgoing_connection,
Peer::PairingToken&& token,
std::optional<pw::bluetooth::emboss::IoCapability> peer_iocap) {
// Private constructor is inaccessible to std::make_unique
std::unique_ptr<Pairing> pairing(new Pairing(outgoing_connection));
std::unique_ptr<Pairing> pairing(
new Pairing(outgoing_connection, std::move(token)));
pairing->initiator = false;
if (peer_iocap.has_value()) {
pairing->peer_iocap = peer_iocap.value();
Expand All @@ -562,8 +569,10 @@ LegacyPairingState::Pairing::MakeResponder(
}

std::unique_ptr<LegacyPairingState::Pairing>
LegacyPairingState::Pairing::MakeResponderForBonded() {
std::unique_ptr<Pairing> pairing(new Pairing(/*outgoing_connection=*/false));
LegacyPairingState::Pairing::MakeResponderForBonded(
Peer::PairingToken&& token) {
std::unique_ptr<Pairing> pairing(
new Pairing(/*outgoing_connection=*/false, std::move(token)));
pairing->initiator = false;
// Do not try to upgrade security as responder
pairing->preferred_security = kNoSecurityRequirements;
Expand Down Expand Up @@ -645,8 +654,10 @@ void LegacyPairingState::InitiateNextPairingRequest() {

PairingRequest& request = request_queue_.front();

current_pairing_ = Pairing::MakeInitiator(request.security_requirements,
outgoing_connection_);
current_pairing_ =
Pairing::MakeInitiator(request.security_requirements,
outgoing_connection_,
peer_->MutBrEdr().RegisterPairing());
bt_log(DEBUG,
"gap-bredr",
"Initiating queued pairing on link %#.4x for peer id %s",
Expand Down
9 changes: 9 additions & 0 deletions pw_bluetooth_sapphire/host/gap/legacy_pairing_state_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ TEST_F(LegacyPairingStateTest, BuildEstablishedLink) {
EXPECT_FALSE(connection()->ltk().has_value());
EXPECT_TRUE(pairing_state.link_key().has_value());
EXPECT_EQ(kTestLinkKey, pairing_state.link_key().value());
EXPECT_TRUE(peer()->MutBrEdr().is_pairing());

// Authentication is done and connection gets made by BrEdrConnectionManager.
// For testing, we manually set the link info using |connection()|
Expand All @@ -231,6 +232,7 @@ TEST_F(LegacyPairingStateTest, BuildEstablishedLink) {
EXPECT_TRUE(connection()->ltk().has_value());
EXPECT_EQ(kTestLinkKeyValue, connection()->ltk()->value());
EXPECT_EQ(kTestLinkKeyValue, pairing_state.link_ltk()->value());
EXPECT_TRUE(peer()->MutBrEdr().is_pairing());
}

TEST_F(LegacyPairingStateTest, PairingStateStartsAsResponder) {
Expand Down Expand Up @@ -259,6 +261,7 @@ TEST_F(LegacyPairingStateTest,
// |auth_cb| is only called if initiation was successful
EXPECT_EQ(0, auth_request_count());
EXPECT_FALSE(pairing_state.initiator());
EXPECT_FALSE(peer()->MutBrEdr().is_pairing());
}

TEST_F(LegacyPairingStateTest, NeverInitiateLegacyPairingWhenPeerSupportsSSP) {
Expand Down Expand Up @@ -311,6 +314,7 @@ TEST_F(LegacyPairingStateTest,
EXPECT_FALSE(pairing_state.initiator());
ASSERT_EQ(1, initiator_status_handler.call_count());
EXPECT_EQ(fit::ok(), *initiator_status_handler.status());
EXPECT_FALSE(peer()->MutBrEdr().is_pairing());
}

TEST_F(
Expand All @@ -336,6 +340,7 @@ TEST_F(
EXPECT_FALSE(connection()->ltk().has_value());
EXPECT_TRUE(pairing_state.link_key().has_value());
EXPECT_EQ(kTestLinkKey, pairing_state.link_key().value());
EXPECT_TRUE(peer()->MutBrEdr().is_pairing());
}

TEST_F(
Expand Down Expand Up @@ -493,6 +498,7 @@ TEST_F(LegacyPairingStateTest,
EXPECT_EQ(kTestHandle, *status_handler.handle());
ASSERT_TRUE(status_handler.status());
EXPECT_EQ(ToResult(HostError::kFailed), *status_handler.status());
EXPECT_FALSE(peer()->MutBrEdr().is_pairing());
}

TEST_F(LegacyPairingStateTest, PairingInitiatorWithNoInputGeneratesRandomPin) {
Expand Down Expand Up @@ -803,6 +809,7 @@ TEST_F(
ASSERT_EQ(1, status_handler.call_count());
EXPECT_EQ(ToResult(pw::bluetooth::emboss::StatusCode::AUTHENTICATION_FAILURE),
status_handler.status().value());
EXPECT_FALSE(peer()->MutBrEdr().is_pairing());
}

TEST_F(LegacyPairingStateTest,
Expand Down Expand Up @@ -1014,6 +1021,7 @@ TEST_F(LegacyPairingStateTest,

// Advance state machine as pairing responder.
pairing_state.OnPinCodeRequest(NoOpUserPinCodeCallback);
EXPECT_TRUE(peer()->MutBrEdr().is_pairing());

// Try to initiate pairing while pairing is in progress.
TestStatusHandler status_handler;
Expand All @@ -1037,6 +1045,7 @@ TEST_F(LegacyPairingStateTest,
EXPECT_EQ(kTestHandle, *status_handler.handle());
ASSERT_TRUE(status_handler.status());
EXPECT_EQ(fit::ok(), *status_handler.status());
EXPECT_FALSE(peer()->MutBrEdr().is_pairing());

// Errors for a new pairing shouldn't invoke the attempted initiator's
// callback.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -201,13 +201,16 @@ class LegacyPairingState final {
public:
static std::unique_ptr<Pairing> MakeInitiator(
BrEdrSecurityRequirements security_requirements,
bool outgoing_connection);
bool outgoing_connection,
Peer::PairingToken&& token);
static std::unique_ptr<Pairing> MakeResponder(
bool outgoing_connection,
Peer::PairingToken&& token,
std::optional<pw::bluetooth::emboss::IoCapability> peer_iocap =
std::nullopt);
// Make a responder for a peer that has initiated pairing.
static std::unique_ptr<Pairing> MakeResponderForBonded();
static std::unique_ptr<Pairing> MakeResponderForBonded(
Peer::PairingToken&& token);

// Used to prevent PairingDelegate callbacks from using captured stale
// pointers.
Expand Down Expand Up @@ -244,9 +247,13 @@ class LegacyPairingState final {
// properties).
BrEdrSecurityRequirements preferred_security;

Peer::PairingToken pairing_token;

private:
explicit Pairing(bool automatic)
: allow_automatic(automatic), weak_self_(this) {}
explicit Pairing(bool automatic, Peer::PairingToken&& token)
: allow_automatic(automatic),
pairing_token(std::move(token)),
weak_self_(this) {}

WeakSelf<Pairing> weak_self_;
};
Expand Down

0 comments on commit 664fbc4

Please sign in to comment.