From 9b4181938593419f0be071baffa355a3b1a882bf Mon Sep 17 00:00:00 2001 From: Karsten Sperling <113487422+ksperling-apple@users.noreply.github.com> Date: Fri, 20 Dec 2024 03:49:01 +1300 Subject: [PATCH] Simplify AutoCommissioner::SetCommissioningParameters (#36896) Unconditionally clear members that point to buffers, and handle them via the explicit code that copies the pointed-to data. The logic to work out whether any pointers need to be cleared was quite complicated. Use memmove() instead of memcpy() since src and dst may overlap / be identical. Always assign mNeedIcdRegistration when the kReadCommissioningInfo step finishes, instead of relying on SetCommissioningParameters to clear it. --- src/controller/AutoCommissioner.cpp | 84 ++++++----------------------- src/lib/support/Span.h | 9 ++-- 2 files changed, 22 insertions(+), 71 deletions(-) diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp index a373b6b34af13f..5e9e37461dec1a 100644 --- a/src/controller/AutoCommissioner.cpp +++ b/src/controller/AutoCommissioner.cpp @@ -49,25 +49,6 @@ void AutoCommissioner::SetOperationalCredentialsDelegate(OperationalCredentialsD mOperationalCredentialsDelegate = operationalCredentialsDelegate; } -// Returns true if maybeUnsafeSpan is pointing to a buffer that we're not sure -// will live for long enough. knownSafeSpan, if it has a value, points to a -// buffer that we _are_ sure will live for long enough. -template -static bool IsUnsafeSpan(const Optional & maybeUnsafeSpan, const Optional & knownSafeSpan) -{ - if (!maybeUnsafeSpan.HasValue()) - { - return false; - } - - if (!knownSafeSpan.HasValue()) - { - return true; - } - - return maybeUnsafeSpan.Value().data() != knownSafeSpan.Value().data(); -} - CHIP_ERROR AutoCommissioner::VerifyICDRegistrationInfo(const CommissioningParameters & params) { ChipLogProgress(Controller, "Checking ICD registration parameters"); @@ -101,44 +82,16 @@ CHIP_ERROR AutoCommissioner::VerifyICDRegistrationInfo(const CommissioningParame CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParameters & params) { - // Make sure any members that point to buffers that we are not pointing to - // our own buffers are not going to dangle. We can skip this step if all - // the buffers pointers that we don't plan to re-point to our own buffers - // below are already pointing to the same things as our own buffer pointers - // (so that we know they have to be safe somehow). - // - // The checks are a bit painful, because Span does not have a usable - // operator==, and in any case, we want to compare for pointer equality, not - // data equality. - bool haveMaybeDanglingBufferPointers = - ((params.GetNOCChainGenerationParameters().HasValue() && - (!mParams.GetNOCChainGenerationParameters().HasValue() || - params.GetNOCChainGenerationParameters().Value().nocsrElements.data() != - mParams.GetNOCChainGenerationParameters().Value().nocsrElements.data() || - params.GetNOCChainGenerationParameters().Value().signature.data() != - mParams.GetNOCChainGenerationParameters().Value().signature.data())) || - IsUnsafeSpan(params.GetRootCert(), mParams.GetRootCert()) || IsUnsafeSpan(params.GetNoc(), mParams.GetNoc()) || - IsUnsafeSpan(params.GetIcac(), mParams.GetIcac()) || IsUnsafeSpan(params.GetIpk(), mParams.GetIpk()) || - IsUnsafeSpan(params.GetAttestationElements(), mParams.GetAttestationElements()) || - IsUnsafeSpan(params.GetAttestationSignature(), mParams.GetAttestationSignature()) || - IsUnsafeSpan(params.GetPAI(), mParams.GetPAI()) || IsUnsafeSpan(params.GetDAC(), mParams.GetDAC()) || - IsUnsafeSpan(params.GetTimeZone(), mParams.GetTimeZone()) || - IsUnsafeSpan(params.GetDSTOffsets(), mParams.GetDSTOffsets()) || - IsUnsafeSpan(params.GetICDSymmetricKey(), mParams.GetICDSymmetricKey()) || - (params.GetDefaultNTP().HasValue() && !params.GetDefaultNTP().Value().IsNull() && - params.GetDefaultNTP().Value().Value().data() != mDefaultNtp)); - + // Our logic below assumes that we can modify mParams without affecting params. + VerifyOrReturnError(¶ms != &mParams, CHIP_NO_ERROR); + + // Copy the whole struct (scalars and pointers), but clear any members that might point to + // external buffers. For those members we have to copy the data over into our own buffers below. + // Note that all of the copy operations use memmove() instead of memcpy(), because the caller + // may be passing a modified shallow copy of our CommissioningParmeters, i.e. where various spans + // already point into the buffers we're copying into, and memcpy() with overlapping buffers is UB. mParams = params; - - mNeedIcdRegistration = false; - - if (haveMaybeDanglingBufferPointers) - { - mParams.ClearExternalBufferDependentValues(); - } - - // For members of params that point to some sort of buffer, we have to copy - // the data over into our own buffers. + mParams.ClearExternalBufferDependentValues(); if (params.GetThreadOperationalDataset().HasValue()) { @@ -146,11 +99,9 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam if (dataset.size() > CommissioningParameters::kMaxThreadDatasetLen) { ChipLogError(Controller, "Thread operational data set is too large"); - // Make sure our buffer pointers don't dangle. - mParams.ClearExternalBufferDependentValues(); return CHIP_ERROR_INVALID_ARGUMENT; } - memcpy(mThreadOperationalDataset, dataset.data(), dataset.size()); + memmove(mThreadOperationalDataset, dataset.data(), dataset.size()); ChipLogProgress(Controller, "Setting thread operational dataset from parameters"); mParams.SetThreadOperationalDataset(ByteSpan(mThreadOperationalDataset, dataset.size())); } @@ -162,12 +113,10 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam creds.credentials.size() > CommissioningParameters::kMaxCredentialsLen) { ChipLogError(Controller, "Wifi credentials are too large"); - // Make sure our buffer pointers don't dangle. - mParams.ClearExternalBufferDependentValues(); return CHIP_ERROR_INVALID_ARGUMENT; } - memcpy(mSsid, creds.ssid.data(), creds.ssid.size()); - memcpy(mCredentials, creds.credentials.data(), creds.credentials.size()); + memmove(mSsid, creds.ssid.data(), creds.ssid.size()); + memmove(mCredentials, creds.credentials.data(), creds.credentials.size()); ChipLogProgress(Controller, "Setting wifi credentials from parameters"); mParams.SetWiFiCredentials( WiFiCredentials(ByteSpan(mSsid, creds.ssid.size()), ByteSpan(mCredentials, creds.credentials.size()))); @@ -184,8 +133,6 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam else { ChipLogError(Controller, "Country code is too large: %u", static_cast(code.size())); - // Make sure our buffer pointers don't dangle. - mParams.ClearExternalBufferDependentValues(); return CHIP_ERROR_INVALID_ARGUMENT; } } @@ -195,7 +142,7 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam { ChipLogProgress(Controller, "Setting attestation nonce from parameters"); VerifyOrReturnError(params.GetAttestationNonce().Value().size() == sizeof(mAttestationNonce), CHIP_ERROR_INVALID_ARGUMENT); - memcpy(mAttestationNonce, params.GetAttestationNonce().Value().data(), params.GetAttestationNonce().Value().size()); + memmove(mAttestationNonce, params.GetAttestationNonce().Value().data(), params.GetAttestationNonce().Value().size()); } else { @@ -208,7 +155,7 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam { ChipLogProgress(Controller, "Setting CSR nonce from parameters"); VerifyOrReturnError(params.GetCSRNonce().Value().size() == sizeof(mCSRNonce), CHIP_ERROR_INVALID_ARGUMENT); - memcpy(mCSRNonce, params.GetCSRNonce().Value().data(), params.GetCSRNonce().Value().size()); + memmove(mCSRNonce, params.GetCSRNonce().Value().data(), params.GetCSRNonce().Value().size()); } else { @@ -271,7 +218,7 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam ReturnErrorOnFailure(VerifyICDRegistrationInfo(params)); // The values must be valid now. - memcpy(mICDSymmetricKey, params.GetICDSymmetricKey().Value().data(), params.GetICDSymmetricKey().Value().size()); + memmove(mICDSymmetricKey, params.GetICDSymmetricKey().Value().data(), params.GetICDSymmetricKey().Value().size()); mParams.SetICDSymmetricKey(ByteSpan(mICDSymmetricKey)); mParams.SetICDCheckInNodeId(params.GetICDCheckInNodeId().Value()); mParams.SetICDMonitoredSubject(params.GetICDMonitoredSubject().Value()); @@ -787,6 +734,7 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio } } + mNeedIcdRegistration = false; if (mParams.GetICDRegistrationStrategy() != ICDRegistrationStrategy::kIgnore) { if (mDeviceCommissioningInfo.icd.isLIT && mDeviceCommissioningInfo.icd.checkInProtocolSupport) diff --git a/src/lib/support/Span.h b/src/lib/support/Span.h index bba719e69e779c..18b077659be047 100644 --- a/src/lib/support/Span.h +++ b/src/lib/support/Span.h @@ -374,7 +374,8 @@ inline CHIP_ERROR CopySpanToMutableSpan(ByteSpan span_to_copy, MutableByteSpan & { VerifyOrReturnError(out_buf.size() >= span_to_copy.size(), CHIP_ERROR_BUFFER_TOO_SMALL); - memcpy(out_buf.data(), span_to_copy.data(), span_to_copy.size()); + // There is no guarantee that span_to_copy and out_buf don't overlap, so use memmove() + memmove(out_buf.data(), span_to_copy.data(), span_to_copy.size()); out_buf.reduce_size(span_to_copy.size()); return CHIP_NO_ERROR; @@ -384,7 +385,8 @@ inline CHIP_ERROR CopyCharSpanToMutableCharSpan(CharSpan cspan_to_copy, MutableC { VerifyOrReturnError(out_buf.size() >= cspan_to_copy.size(), CHIP_ERROR_BUFFER_TOO_SMALL); - memcpy(out_buf.data(), cspan_to_copy.data(), cspan_to_copy.size()); + // There is no guarantee that cspan_to_copy and out_buf don't overlap, so use memmove() + memmove(out_buf.data(), cspan_to_copy.data(), cspan_to_copy.size()); out_buf.reduce_size(cspan_to_copy.size()); return CHIP_NO_ERROR; @@ -400,7 +402,8 @@ inline void CopyCharSpanToMutableCharSpanWithTruncation(CharSpan span_to_copy, M { size_t size_to_copy = std::min(span_to_copy.size(), out_span.size()); - memcpy(out_span.data(), span_to_copy.data(), size_to_copy); + // There is no guarantee that span_to_copy and out_buf don't overlap, so use memmove() + memmove(out_span.data(), span_to_copy.data(), size_to_copy); out_span.reduce_size(size_to_copy); }