From d1ed23d8cc918167701a01a049b165bfcc12857b Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Fri, 22 Jul 2022 13:25:43 -0400 Subject: [PATCH] Fix crash if commissionee drops PASE session before AddNOC. Several changes here: 1. Fix ReleaseCommissioneeDevice so it does not leave mDeviceBeingCommissioned dangling. 2. Ensure that we have null-checks for mDeviceBeingCommissioned before all uses, in case it got nulled out by StopPairing or the like while some async operation was in flight. This changes: * DeviceCommissioner::DisarmDone * DeviceCommissioner::OnDeviceAttestationInformationVerification 3. Remove kSessionEstablishmentTimeout so we don't have a random hardcoded timeout partway through commissioning. Fixes https://github.com/project-chip/connectedhomeip/issues/21106 Fixes https://github.com/project-chip/connectedhomeip/issues/14650 --- src/controller/CHIPDeviceController.cpp | 44 ++++++++----------------- src/controller/CHIPDeviceController.h | 4 --- 2 files changed, 14 insertions(+), 34 deletions(-) diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 9543c1c3414e94..06d6e8039afca3 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -97,8 +97,6 @@ using namespace chip::Encoding; using namespace chip::Protocols::UserDirectedCommissioning; #endif // CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY -constexpr uint32_t kSessionEstablishmentTimeout = 40 * kMillisecondsPerSecond; - DeviceController::DeviceController() { mState = State::NotInitialized; @@ -546,6 +544,10 @@ void DeviceCommissioner::ReleaseCommissioneeDevice(CommissioneeDeviceProxy * dev { mDeviceInPASEEstablishment = nullptr; } + if (mDeviceBeingCommissioned == device) + { + mDeviceBeingCommissioned = nullptr; + } } CHIP_ERROR DeviceCommissioner::GetDeviceBeingCommissioned(NodeId deviceId, CommissioneeDeviceProxy ** out_device) @@ -759,9 +761,6 @@ CHIP_ERROR DeviceCommissioner::Commission(NodeId remoteDeviceId) ChipLogProgress(Controller, "Commission called for node ID 0x" ChipLogFormatX64, ChipLogValueX64(remoteDeviceId)); - mSystemState->SystemLayer()->StartTimer(chip::System::Clock::Milliseconds32(kSessionEstablishmentTimeout), - OnSessionEstablishmentTimeoutCallback, this); - mDefaultCommissioner->SetOperationalCredentialsDelegate(mOperationalCredentialsDelegate); if (device->IsSecureConnected()) { @@ -859,9 +858,6 @@ void DeviceCommissioner::RendezvousCleanup(CHIP_ERROR status) void DeviceCommissioner::OnSessionEstablishmentError(CHIP_ERROR err) { - // PASE session establishment failure. - mSystemState->SystemLayer()->CancelTimer(OnSessionEstablishmentTimeoutCallback, this); - if (mPairingDelegate != nullptr) { mPairingDelegate->OnStatusUpdate(DevicePairingDelegate::SecurePairingFailed); @@ -977,6 +973,12 @@ void DeviceCommissioner::OnDeviceAttestationInformationVerification(void * conte MATTER_TRACE_EVENT_SCOPE("OnDeviceAttestationInformationVerification", "DeviceCommissioner"); DeviceCommissioner * commissioner = reinterpret_cast(context); + if (!commissioner->mDeviceBeingCommissioned) + { + ChipLogError(Controller, "Device attestation verification result received when we're not commissioning a device"); + return; + } + if (result != AttestationVerificationResult::kSuccess) { CommissioningDelegate::CommissioningReport report; @@ -1365,8 +1367,6 @@ CHIP_ERROR DeviceCommissioner::OnOperationalCredentialsProvisioningCompletion(De ChipLogProgress(Controller, "Operational credentials provisioned on device %p", device); VerifyOrReturnError(device != nullptr, CHIP_ERROR_INVALID_ARGUMENT); - mSystemState->SystemLayer()->CancelTimer(OnSessionEstablishmentTimeoutCallback, this); - if (mPairingDelegate != nullptr) { mPairingDelegate->OnStatusUpdate(DevicePairingDelegate::SecurePairingSuccess); @@ -1397,26 +1397,6 @@ void DeviceCommissioner::CloseBleConnection() } #endif -void DeviceCommissioner::OnSessionEstablishmentTimeout() -{ - // This is called from the session establishment timer. Please see - // https://github.com/project-chip/connectedhomeip/issues/14650 - VerifyOrReturn(mState == State::Initialized); - VerifyOrReturn(mDeviceBeingCommissioned != nullptr); - - StopPairing(mDeviceBeingCommissioned->GetDeviceId()); - - if (mPairingDelegate != nullptr) - { - mPairingDelegate->OnPairingComplete(CHIP_ERROR_TIMEOUT); - } -} - -void DeviceCommissioner::OnSessionEstablishmentTimeoutCallback(System::Layer * aLayer, void * aAppState) -{ - static_cast(aAppState)->OnSessionEstablishmentTimeout(); -} - CHIP_ERROR DeviceCommissioner::DiscoverCommissionableNodes(Dnssd::DiscoveryFilter filter) { ReturnErrorOnFailure(SetUpNodeDiscovery()); @@ -1524,6 +1504,10 @@ void DeviceCommissioner::OnDisarmFailsafeFailure(void * context, CHIP_ERROR erro void DeviceCommissioner::DisarmDone() { + // If someone nulled out our mDeviceBeingCommissioned, there's nothing else + // to do here. + VerifyOrReturn(mDeviceBeingCommissioned != nullptr); + // At this point, we also want to close off the pase session so we need to re-establish CommissioneeDeviceProxy * commissionee = FindCommissioneeDevice(mDeviceBeingCommissioned->GetDeviceId()); diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h index 3ac9f77ebfa2d7..67ecfe2146e4b5 100644 --- a/src/controller/CHIPDeviceController.h +++ b/src/controller/CHIPDeviceController.h @@ -677,10 +677,6 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, CHIP_ERROR LoadKeyId(PersistentStorageDelegate * delegate, uint16_t & out); - void OnSessionEstablishmentTimeout(); - - static void OnSessionEstablishmentTimeoutCallback(System::Layer * aLayer, void * aAppState); - /* This function sends a Device Attestation Certificate chain request to the device. The function does not hold a reference to the device object. */