Skip to content

Commit

Permalink
Simplify AutoCommissioner::SetCommissioningParameters (#36896)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@ksperling-apple
ksperling-apple authored Dec 19, 2024
1 parent aac3e3f commit 9b41819
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 71 deletions.
84 changes: 16 additions & 68 deletions src/controller/AutoCommissioner.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 <typename SpanType>
static bool IsUnsafeSpan(const Optional<SpanType> & maybeUnsafeSpan, const Optional<SpanType> & 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");
Expand Down Expand Up @@ -101,56 +82,26 @@ 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(&params != &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())
{
ByteSpan dataset = params.GetThreadOperationalDataset().Value();
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()));
}
Expand All @@ -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())));
Expand All @@ -184,8 +133,6 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam
else
{
ChipLogError(Controller, "Country code is too large: %u", static_cast<unsigned>(code.size()));
// Make sure our buffer pointers don't dangle.
mParams.ClearExternalBufferDependentValues();
return CHIP_ERROR_INVALID_ARGUMENT;
}
}
Expand All @@ -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
{
Expand All @@ -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
{
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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)
Expand Down
9 changes: 6 additions & 3 deletions src/lib/support/Span.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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);
}

Expand Down

0 comments on commit 9b41819

Please sign in to comment.