Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit fd0af8fd authored by Zach Johnson's avatar Zach Johnson
Browse files

Prevent race condition in setting up L2CAP 

Since outgoing and incoming were accessing the same underlying data
structure, if both were in progress one could overwrite the data
present and the first to succeed would cause the following one to crash.

Instead pass the data directly in the closure, to remove unnecessary
fields and ensure the data is present.

Test: cert/run --host
Tag: #gd-refactor
Bug: 159815595
Change-Id: Ibd7f98ea29d06c0a1b8864fe08adf95572c06ca4
parent 7fc3ba30

system/gd/l2cap/classic/internal/signalling_manager.cc

+26 −36
Original line number Diff line number Diff line
@@ -89,22 +89,13 @@ void ClassicSignallingManager::OnCommandReject(CommandRejectView command_reject_ 
}



void ClassicSignallingManager::SendConnectionRequest(Psm psm, Cid local_cid) {

  PendingConnection pending{

      .local_cid = local_cid,

  };

  pending_security_requests_[psm] = pending;

  dynamic_service_manager_->GetSecurityEnforcementInterface()->Enforce(

      link_->GetDevice(), dynamic_service_manager_->GetService(psm)->GetSecurityPolicy(),

      handler_->BindOnceOn(this, &ClassicSignallingManager::on_security_result_for_outgoing, psm));

      link_->GetDevice(),

      dynamic_service_manager_->GetService(psm)->GetSecurityPolicy(),

      handler_->BindOnceOn(this, &ClassicSignallingManager::on_security_result_for_outgoing, psm, local_cid));

}



void ClassicSignallingManager::on_security_result_for_outgoing(Psm psm, bool result) {

  ASSERT_LOG(pending_security_requests_.find(psm) != pending_security_requests_.end(),

             "Received security result without pending request");



  auto request = pending_security_requests_[psm];

  pending_security_requests_.erase(psm);



void ClassicSignallingManager::on_security_result_for_outgoing(Psm psm, Cid local_cid, bool result) {

  if (!result) {

    LOG_WARN("Security requirement can't be satisfied. Dropping connection request");

    DynamicChannelManager::ConnectionResult connection_result{
@@ -112,12 +103,11 @@ void ClassicSignallingManager::on_security_result_for_outgoing(Psm psm, bool res 
        .hci_error = hci::ErrorCode::SUCCESS,

        .l2cap_connection_response_result = ConnectionResponseResult::NO_RESOURCES_AVAILABLE,

    };

    link_->OnOutgoingConnectionRequestFail(request.local_cid, connection_result);

    link_->OnOutgoingConnectionRequestFail(local_cid, connection_result);

    return;

  }



  PendingCommand pending_command = {

      next_signal_id_, CommandCode::CONNECTION_REQUEST, psm, request.local_cid, {}, {}, {}};

  PendingCommand pending_command = {next_signal_id_, CommandCode::CONNECTION_REQUEST, psm, local_cid, {}, {}, {}};

  next_signal_id_++;

  pending_commands_.push(std::move(pending_command));

  if (command_just_sent_.signal_id_ == kInvalidSignalId) {
@@ -191,40 +181,40 @@ void ClassicSignallingManager::OnConnectionRequest(SignalId signal_id, Psm psm, 
    return;

  }



  PendingConnection pending{

      .remote_cid = remote_cid,

      .incoming_signal_id = signal_id,

  };

  pending_security_requests_[psm] = pending;

  dynamic_service_manager_->GetSecurityEnforcementInterface()->Enforce(

      link_->GetDevice(), dynamic_service_manager_->GetService(psm)->GetSecurityPolicy(),

      handler_->BindOnceOn(this, &ClassicSignallingManager::on_security_result_for_incoming, psm));

      link_->GetDevice(),

      dynamic_service_manager_->GetService(psm)->GetSecurityPolicy(),

      handler_->BindOnceOn(

          this, &ClassicSignallingManager::on_security_result_for_incoming, psm, remote_cid, signal_id));

}



void ClassicSignallingManager::on_security_result_for_incoming(Psm psm, bool result) {

  ASSERT_LOG(pending_security_requests_.find(psm) != pending_security_requests_.end(),

             "Received security result without pending request");



  auto request = pending_security_requests_[psm];

  pending_security_requests_.erase(psm);

  auto signal_id = request.incoming_signal_id;

void ClassicSignallingManager::on_security_result_for_incoming(

    Psm psm, Cid remote_cid, SignalId signal_id, bool result) {

  if (!result) {

    send_connection_response(signal_id, request.remote_cid, request.local_cid, ConnectionResponseResult::SECURITY_BLOCK,

    send_connection_response(

        signal_id,

        remote_cid,

        0,

        ConnectionResponseResult::SECURITY_BLOCK,

        ConnectionResponseStatus::NO_FURTHER_INFORMATION_AVAILABLE);

    DynamicChannelManager::ConnectionResult connection_result{

        .connection_result_code = DynamicChannelManager::ConnectionResultCode::FAIL_SECURITY_BLOCK,

        .hci_error = hci::ErrorCode::SUCCESS,

        .l2cap_connection_response_result = ConnectionResponseResult::NO_RESOURCES_AVAILABLE,

    };

    link_->OnOutgoingConnectionRequestFail(request.local_cid, connection_result);

    link_->OnOutgoingConnectionRequestFail(0, connection_result);

  }



  auto new_channel = link_->AllocateDynamicChannel(psm, request.remote_cid);

  auto new_channel = link_->AllocateDynamicChannel(psm, remote_cid);

  if (new_channel == nullptr) {

    LOG_WARN("Can't allocate dynamic channel");

    return;

  }

  send_connection_response(signal_id, request.remote_cid, new_channel->GetCid(), ConnectionResponseResult::SUCCESS,

  send_connection_response(

      signal_id,

      remote_cid,

      new_channel->GetCid(),

      ConnectionResponseResult::SUCCESS,

      ConnectionResponseStatus::NO_FURTHER_INFORMATION_AVAILABLE);



  link_->SendInitialConfigRequestOrQueue(new_channel->GetCid());

system/gd/l2cap/classic/internal/signalling_manager.h

+2 −9
Original line number Diff line number Diff line
@@ -111,8 +111,8 @@ class ClassicSignallingManager { 
                               std::vector<std::unique_ptr<ConfigurationOption>>);



  void send_configuration_request(Cid remote_cid, std::vector<std::unique_ptr<ConfigurationOption>> config);

  void on_security_result_for_incoming(Psm psm, bool result);

  void on_security_result_for_outgoing(Psm psm, bool result);

  void on_security_result_for_incoming(Psm psm, Cid remote_cid, SignalId signal_id, bool result);

  void on_security_result_for_outgoing(Psm psm, Cid local_cid, bool result);



  os::Handler* handler_;

  Link* link_;
@@ -127,13 +127,6 @@ class ClassicSignallingManager { 
  os::Alarm alarm_;

  SignalId next_signal_id_ = kInitialSignalId;

  std::unordered_map<Cid, ChannelConfigurationState> channel_configuration_;



  struct PendingConnection {

    Cid local_cid;

    Cid remote_cid;

    SignalId incoming_signal_id;

  };

  std::unordered_map<Psm, PendingConnection> pending_security_requests_;

};



}  // namespace internal

system/gd/l2cap/le/internal/signalling_manager.cc

+9 −24
Original line number Diff line number Diff line
@@ -57,30 +57,20 @@ LeSignallingManager::~LeSignallingManager() { 
}



void LeSignallingManager::SendConnectionRequest(Psm psm, Cid local_cid, Mtu mtu) {

  PendingConnection pending{

      .local_cid = local_cid,

      .mtu = mtu,

  };

  pending_security_requests_[psm] = pending;

  dynamic_service_manager_->GetSecurityEnforcementInterface()->Enforce(

      link_->GetDevice(), dynamic_service_manager_->GetService(psm)->GetSecurityPolicy(),

      handler_->BindOnceOn(this, &LeSignallingManager::on_security_result_for_outgoing, psm));

      link_->GetDevice(),

      dynamic_service_manager_->GetService(psm)->GetSecurityPolicy(),

      handler_->BindOnceOn(this, &LeSignallingManager::on_security_result_for_outgoing, psm, local_cid, mtu));

}



void LeSignallingManager::on_security_result_for_outgoing(Psm psm, bool result) {

  ASSERT_LOG(pending_security_requests_.find(psm) != pending_security_requests_.end(),

             "Received security result without pending request");



  auto request = pending_security_requests_[psm];

  pending_security_requests_.erase(psm);



void LeSignallingManager::on_security_result_for_outgoing(Psm psm, Cid local_cid, Mtu mtu, bool result) {

  if (!result) {

    LOG_WARN("Security requirement can't be satisfied. Dropping connection request");

    return;

  }



  PendingCommand pending_command = PendingCommand::CreditBasedConnectionRequest(

      next_signal_id_, psm, request.local_cid, request.mtu, link_->GetMps(), link_->GetInitialCredit());

      next_signal_id_, psm, local_cid, mtu, link_->GetMps(), link_->GetInitialCredit());

  next_signal_id_++;

  pending_commands_.push(pending_command);

  if (pending_commands_.size() == 1) {
@@ -214,18 +204,13 @@ void LeSignallingManager::OnConnectionRequest(SignalId signal_id, Psm psm, Cid r 
      .max_pdu_size = mps,

      .mtu = mtu,

  };

  pending_security_requests_[psm] = pending;

  dynamic_service_manager_->GetSecurityEnforcementInterface()->Enforce(

      link_->GetDevice(), dynamic_service_manager_->GetService(psm)->GetSecurityPolicy(),

      handler_->BindOnceOn(this, &LeSignallingManager::on_security_result_for_incoming, psm));

      link_->GetDevice(),

      dynamic_service_manager_->GetService(psm)->GetSecurityPolicy(),

      handler_->BindOnceOn(this, &LeSignallingManager::on_security_result_for_incoming, psm, pending));

}



void LeSignallingManager::on_security_result_for_incoming(Psm psm, bool result) {

  ASSERT_LOG(pending_security_requests_.find(psm) != pending_security_requests_.end(),

             "Received security result without pending request");



  auto request = pending_security_requests_[psm];

  pending_security_requests_.erase(psm);

void LeSignallingManager::on_security_result_for_incoming(Psm psm, PendingConnection request, bool result) {

  auto signal_id = request.incoming_signal_id;

  auto* service = dynamic_service_manager_->GetService(psm);

  if (!result) {

system/gd/l2cap/le/internal/signalling_manager.h

+10 −12
Original line number Diff line number Diff line
@@ -132,13 +132,21 @@ class LeSignallingManager { 
  void OnCredit(Cid remote_cid, uint16_t credits);



 private:

  struct PendingConnection {

    Cid remote_cid;

    Mtu mtu;

    uint16_t max_pdu_size;

    uint16_t initial_credits;

    SignalId incoming_signal_id;

  };



  void on_incoming_packet();

  void send_connection_response(SignalId signal_id, Cid local_cid, Mtu mtu, uint16_t mps, uint16_t initial_credit,

                                LeCreditBasedConnectionResponseResult result);

  void on_command_timeout();

  void handle_send_next_command();

  void on_security_result_for_incoming(Psm psm, bool result);

  void on_security_result_for_outgoing(Psm psm, bool result);

  void on_security_result_for_incoming(Psm psm, PendingConnection request, bool result);

  void on_security_result_for_outgoing(Psm psm, Cid local_cid, Mtu mtu, bool result);



  os::Handler* handler_;

  Link* link_;
@@ -151,16 +159,6 @@ class LeSignallingManager { 
  PendingCommand command_just_sent_;

  os::Alarm alarm_;

  SignalId next_signal_id_ = kInitialSignalId;



  struct PendingConnection {

    Cid local_cid;

    Cid remote_cid;

    Mtu mtu;

    uint16_t max_pdu_size;

    uint16_t initial_credits;

    SignalId incoming_signal_id;

  };

  std::unordered_map<Psm, PendingConnection> pending_security_requests_;

};



}  // namespace internal