From 56ebe19506b53b48f1475bd418e626aac5999746 Mon Sep 17 00:00:00 2001 From: Kamil Kasperczyk Date: Thu, 20 Feb 2025 13:44:56 +0100 Subject: [PATCH] Replaced str... with strn... methods Static code analysis shown that there are several places that str.. methods are used, what may lead to the bad memory access. Replaced all of them with corresponding strn... methods. --- src/app/server/Server.cpp | 4 +-- src/inet/InetInterface.cpp | 17 +++++----- src/inet/UDPEndPointImplSockets.cpp | 3 +- src/lib/dnssd/Discovery_ImplPlatform.cpp | 5 +-- src/lib/dnssd/Types.h | 14 +++++--- src/platform/Zephyr/BLEManagerImpl.cpp | 2 +- .../Zephyr/KeyValueStoreManagerImpl.cpp | 2 +- src/platform/Zephyr/Logging.cpp | 2 +- src/platform/Zephyr/ZephyrConfig.cpp | 32 +++++++++++++++++-- 9 files changed, 58 insertions(+), 23 deletions(-) diff --git a/src/app/server/Server.cpp b/src/app/server/Server.cpp index 3cc918f91416d0..1f4d82f7c8aab4 100644 --- a/src/app/server/Server.cpp +++ b/src/app/server/Server.cpp @@ -695,7 +695,7 @@ CHIP_ERROR Server::SendUserDirectedCommissioningRequest(Transport::PeerAddress c CHIP_ERROR err; // only populate fields left blank by the client - if (strlen(id.GetInstanceName()) == 0) + if (strnlen(id.GetInstanceName(), Dnssd::Commission::kInstanceNameMaxLength + 1) == 0) { ChipLogDetail(AppServer, "Server::SendUserDirectedCommissioningRequest() Instance Name not known"); char nameBuffer[Dnssd::Commission::kInstanceNameMaxLength + 1]; @@ -738,7 +738,7 @@ CHIP_ERROR Server::SendUserDirectedCommissioningRequest(Transport::PeerAddress c } } - if (strlen(id.GetDeviceName()) == 0) + if (strnlen(id.GetDeviceName(), Dnssd::kKeyDeviceNameMaxLength + 1) == 0) { char deviceName[Dnssd::kKeyDeviceNameMaxLength + 1] = {}; if (!DeviceLayer::ConfigurationMgr().IsCommissionableDeviceNameEnabled() || diff --git a/src/inet/InetInterface.cpp b/src/inet/InetInterface.cpp index 4595dfef449fe3..604c8a7a2fdf99 100644 --- a/src/inet/InetInterface.cpp +++ b/src/inet/InetInterface.cpp @@ -69,7 +69,7 @@ namespace Inet { #if CHIP_SYSTEM_CONFIG_USE_OPEN_THREAD_ENDPOINT CHIP_ERROR InterfaceId::GetInterfaceName(char * nameBuf, size_t nameBufSize) const { - if (mPlatformInterface && nameBufSize >= kMaxIfNameLength) + if (mPlatformInterface && nameBufSize >= InterfaceId::kMaxIfNameLength) { nameBuf[0] = 'o'; nameBuf[1] = 't'; @@ -84,7 +84,7 @@ CHIP_ERROR InterfaceId::GetInterfaceName(char * nameBuf, size_t nameBufSize) con } CHIP_ERROR InterfaceId::InterfaceNameToId(const char * intfName, InterfaceId & interface) { - if (strlen(intfName) < 3) + if (strnlen(intfName, kMaxIfNameLength) < 3) { return INET_ERROR_UNKNOWN_INTERFACE; } @@ -210,7 +210,7 @@ CHIP_ERROR InterfaceId::GetInterfaceName(char * nameBuf, size_t nameBufSize) con CHIP_ERROR InterfaceId::InterfaceNameToId(const char * intfName, InterfaceId & interface) { - if (strlen(intfName) < 3) + if (strnlen(intfName, InterfaceId::kMaxIfNameLength) < 3) { return INET_ERROR_UNKNOWN_INTERFACE; } @@ -445,7 +445,7 @@ CHIP_ERROR InterfaceId::GetInterfaceName(char * nameBuf, size_t nameBufSize) con { return CHIP_ERROR_POSIX(errno); } - size_t nameLength = strlen(intfName); + size_t nameLength = strnlen(intfName, IF_NAMESIZE); if (nameLength >= nameBufSize) { return CHIP_ERROR_BUFFER_TOO_SMALL; @@ -579,7 +579,8 @@ InterfaceId InterfaceIterator::GetInterfaceId() CHIP_ERROR InterfaceIterator::GetInterfaceName(char * nameBuf, size_t nameBufSize) { VerifyOrReturnError(HasCurrent(), CHIP_ERROR_INCORRECT_STATE); - VerifyOrReturnError(strlen(mIntfArray[mCurIntf].if_name) < nameBufSize, CHIP_ERROR_BUFFER_TOO_SMALL); + VerifyOrReturnError(strnlen(mIntfArray[mCurIntf].if_name, InterfaceId::kMaxIfNameLength) < nameBufSize, + CHIP_ERROR_BUFFER_TOO_SMALL); Platform::CopyString(nameBuf, nameBufSize, mIntfArray[mCurIntf].if_name); return CHIP_NO_ERROR; } @@ -728,7 +729,7 @@ InterfaceId InterfaceAddressIterator::GetInterfaceId() CHIP_ERROR InterfaceAddressIterator::GetInterfaceName(char * nameBuf, size_t nameBufSize) { VerifyOrReturnError(HasCurrent(), CHIP_ERROR_INCORRECT_STATE); - VerifyOrReturnError(strlen(mCurAddr->ifa_name) < nameBufSize, CHIP_ERROR_BUFFER_TOO_SMALL); + VerifyOrReturnError(strnlen(mCurAddr->ifa_name, InterfaceId::kMaxIfNameLength) < nameBufSize, CHIP_ERROR_BUFFER_TOO_SMALL); Platform::CopyString(nameBuf, nameBufSize, mCurAddr->ifa_name); return CHIP_NO_ERROR; } @@ -802,7 +803,7 @@ CHIP_ERROR InterfaceId::GetInterfaceName(char * nameBuf, size_t nameBufSize) con return CHIP_ERROR_INCORRECT_STATE; } const char * name = net_if_get_device(currentInterface)->name; - size_t nameLength = strlen(name); + size_t nameLength = strnlen(name, InterfaceId::kMaxIfNameLength); if (nameLength >= nameBufSize) { return CHIP_ERROR_BUFFER_TOO_SMALL; @@ -825,7 +826,7 @@ CHIP_ERROR InterfaceId::InterfaceNameToId(const char * intfName, InterfaceId & i while ((currentInterface = net_if_get_by_index(++currentId)) != nullptr) { - if (strcmp(net_if_get_device(currentInterface)->name, intfName) == 0) + if (strncmp(net_if_get_device(currentInterface)->name, intfName, InterfaceId::kMaxIfNameLength) == 0) { interface = InterfaceId(currentId); return CHIP_NO_ERROR; diff --git a/src/inet/UDPEndPointImplSockets.cpp b/src/inet/UDPEndPointImplSockets.cpp index 811d8a50d6194a..6c3be9a01d5533 100644 --- a/src/inet/UDPEndPointImplSockets.cpp +++ b/src/inet/UDPEndPointImplSockets.cpp @@ -234,7 +234,8 @@ CHIP_ERROR UDPEndPointImplSockets::BindInterfaceImpl(IPAddressType addressType, { status = CHIP_ERROR_POSIX(errno); } - else if (setsockopt(mSocket, SOL_SOCKET, SO_BINDTODEVICE, interfaceName, socklen_t(strlen(interfaceName))) == -1) + else if (setsockopt(mSocket, SOL_SOCKET, SO_BINDTODEVICE, interfaceName, socklen_t(strnlen(interfaceName, IF_NAMESIZE))) == + -1) { status = CHIP_ERROR_POSIX(errno); } diff --git a/src/lib/dnssd/Discovery_ImplPlatform.cpp b/src/lib/dnssd/Discovery_ImplPlatform.cpp index e83a88915f2ea4..f52030607f4c3f 100644 --- a/src/lib/dnssd/Discovery_ImplPlatform.cpp +++ b/src/lib/dnssd/Discovery_ImplPlatform.cpp @@ -102,7 +102,7 @@ static void HandleNodeBrowse(void * context, DnssdService * services, size_t ser auto & ipAddress = services[i].mAddress; // mType(service name) exactly matches with operational service name - bool isOperationalBrowse = strcmp(services[i].mType, kOperationalServiceName) == 0; + bool isOperationalBrowse = strncmp(services[i].mType, kOperationalServiceName, sizeof(kOperationalServiceName)) == 0; // For operational browse result we currently don't need IP address hence skip resolution and handle differently. if (isOperationalBrowse) @@ -110,7 +110,8 @@ static void HandleNodeBrowse(void * context, DnssdService * services, size_t ser HandleNodeOperationalBrowse(context, &services[i], error); } // check whether SRV, TXT and AAAA records were received in DNS responses - else if (strlen(services[i].mHostName) == 0 || services[i].mTextEntrySize == 0 || !ipAddress.has_value()) + else if (strnlen(services[i].mHostName, chip::Dnssd::kHostNameMaxLength + 1) == 0 || services[i].mTextEntrySize == 0 || + !ipAddress.has_value()) { ChipDnssdResolve(&services[i], services[i].mInterface, HandleNodeResolve, context); } diff --git a/src/lib/dnssd/Types.h b/src/lib/dnssd/Types.h index 60a265e46e425e..7f65fb99bcf386 100644 --- a/src/lib/dnssd/Types.h +++ b/src/lib/dnssd/Types.h @@ -64,7 +64,8 @@ struct DiscoveryFilter } if (type == DiscoveryFilterType::kInstanceName) { - return (instanceName != nullptr) && (other.instanceName != nullptr) && (strcmp(instanceName, other.instanceName) == 0); + return (instanceName != nullptr) && (other.instanceName != nullptr) && + (strncmp(instanceName, other.instanceName, Common::kInstanceNameMaxLength + 1) == 0); } return code == other.code; @@ -121,7 +122,7 @@ struct CommonResolutionData (mrpRetryIntervalActive.has_value() && (*mrpRetryIntervalActive > defaultMRPConfig->mActiveRetransTimeout)); } - bool IsHost(const char * host) const { return strcmp(host, hostName) == 0; } + bool IsHost(const char * host) const { return strncmp(host, hostName, kHostNameMaxLength + 1) == 0; } void Reset() { @@ -244,7 +245,10 @@ struct CommissionNodeData : public CommonResolutionData new (this) CommissionNodeData(); } - bool IsInstanceName(const char * instance) const { return strcmp(instance, instanceName) == 0; } + bool IsInstanceName(const char * instance) const + { + return strncmp(instance, instanceName, Common::kInstanceNameMaxLength + 1) == 0; + } void LogDetail() const { @@ -257,7 +261,7 @@ struct CommissionNodeData : public CommonResolutionData Encoding::BytesToUppercaseHexString(rotatingId, rotatingIdLen, rotatingIdString, sizeof(rotatingIdString)); ChipLogDetail(Discovery, "\tRotating ID: %s", rotatingIdString); } - if (strlen(deviceName) != 0) + if (strnlen(deviceName, kMaxDeviceNameLen + 1) != 0) { ChipLogDetail(Discovery, "\tDevice Name: %s", deviceName); } @@ -277,7 +281,7 @@ struct CommissionNodeData : public CommonResolutionData { ChipLogDetail(Discovery, "\tLong Discriminator: %u", longDiscriminator); } - if (strlen(pairingInstruction) != 0) + if (strnlen(pairingInstruction, kMaxPairingInstructionLen + 1) != 0) { ChipLogDetail(Discovery, "\tPairing Instruction: %s", pairingInstruction); } diff --git a/src/platform/Zephyr/BLEManagerImpl.cpp b/src/platform/Zephyr/BLEManagerImpl.cpp index 6b8ba4b6989ce6..3d8d770ebcb17e 100644 --- a/src/platform/Zephyr/BLEManagerImpl.cpp +++ b/src/platform/Zephyr/BLEManagerImpl.cpp @@ -297,7 +297,7 @@ inline CHIP_ERROR BLEManagerImpl::PrepareAdvertisingRequest() static_assert(sizeof(serviceData) == 10, "Unexpected size of BLE advertising data!"); const char * name = bt_get_name(); - const uint8_t nameSize = static_cast(strlen(name)); + const uint8_t nameSize = static_cast(strnlen(name, CONFIG_BT_DEVICE_NAME_MAX)); Encoding::LittleEndian::Put16(serviceData.uuid, UUID16_CHIPoBLEService.val); ReturnErrorOnFailure(ConfigurationMgr().GetBLEDeviceIdentificationInfo(serviceData.deviceIdInfo)); diff --git a/src/platform/Zephyr/KeyValueStoreManagerImpl.cpp b/src/platform/Zephyr/KeyValueStoreManagerImpl.cpp index 95c004e7f7a8d8..fc1f32a9fe8425 100644 --- a/src/platform/Zephyr/KeyValueStoreManagerImpl.cpp +++ b/src/platform/Zephyr/KeyValueStoreManagerImpl.cpp @@ -56,7 +56,7 @@ constexpr size_t kEmptyValueSize = sizeof(kEmptyValue); CHIP_ERROR MakeFullKey(char (&fullKey)[SETTINGS_MAX_NAME_LEN + 1], const char * key) { VerifyOrReturnError(key != nullptr, CHIP_ERROR_INVALID_ARGUMENT); - strcpy(fullKey, CHIP_DEVICE_CONFIG_SETTINGS_KEY "/"); + strncpy(fullKey, CHIP_DEVICE_CONFIG_SETTINGS_KEY "/", SETTINGS_MAX_NAME_LEN + 1); char * dest = fullKey + strlen(CHIP_DEVICE_CONFIG_SETTINGS_KEY "/"); char * destEnd = fullKey + SETTINGS_MAX_NAME_LEN; diff --git a/src/platform/Zephyr/Logging.cpp b/src/platform/Zephyr/Logging.cpp index 4f7322534e331e..61714432ba7531 100644 --- a/src/platform/Zephyr/Logging.cpp +++ b/src/platform/Zephyr/Logging.cpp @@ -47,7 +47,7 @@ void ENFORCE_FORMAT(3, 0) LogV(const char * module, uint8_t category, const char char formattedMsg[CHIP_CONFIG_LOG_MESSAGE_MAX_SIZE]; snprintfcb(formattedMsg, sizeof(formattedMsg), "[%s]", module); - const size_t prefixLen = strlen(formattedMsg); + const size_t prefixLen = strnlen(formattedMsg, CHIP_CONFIG_LOG_MESSAGE_MAX_SIZE); vsnprintfcb(formattedMsg + prefixLen, sizeof(formattedMsg) - prefixLen, msg, v); const char * allocatedMsg = formattedMsg; diff --git a/src/platform/Zephyr/ZephyrConfig.cpp b/src/platform/Zephyr/ZephyrConfig.cpp index f548e95bca09a7..aefaa6434c59b3 100644 --- a/src/platform/Zephyr/ZephyrConfig.cpp +++ b/src/platform/Zephyr/ZephyrConfig.cpp @@ -31,6 +31,7 @@ #include #include +#include namespace chip { namespace DeviceLayer { @@ -48,6 +49,12 @@ namespace Internal { #define NAMESPACE_CONFIG CHIP_DEVICE_CONFIG_SETTINGS_KEY "/cfg/" #define NAMESPACE_COUNTERS CHIP_DEVICE_CONFIG_SETTINGS_KEY "/ctr/" +#if DT_HAS_CHOSEN(zephyr_settings_partition) +#define SETTINGS_PARTITION DT_FIXED_PARTITION_ID(DT_CHOSEN(zephyr_settings_partition)) +#else +#define SETTINGS_PARTITION FIXED_PARTITION_ID(storage_partition) +#endif + // Keys stored in the chip factory nam const ZephyrConfig::Key ZephyrConfig::kConfigKey_SerialNum = CONFIG_KEY(NAMESPACE_FACTORY "serial-num"); const ZephyrConfig::Key ZephyrConfig::kConfigKey_MfrDeviceId = CONFIG_KEY(NAMESPACE_FACTORY "device-id"); @@ -78,6 +85,8 @@ const ZephyrConfig::Key ZephyrConfig::kCounterKey_RebootCount = CONFIG const ZephyrConfig::Key ZephyrConfig::kCounterKey_BootReason = CONFIG_KEY(NAMESPACE_COUNTERS "boot-reason"); const ZephyrConfig::Key ZephyrConfig::kCounterKey_TotalOperationalHours = CONFIG_KEY(NAMESPACE_COUNTERS "total-operational-hours"); +static size_t sSectorSize = 0; + namespace { constexpr const char * sAllResettableConfigKeys[] = { @@ -161,6 +170,23 @@ CHIP_ERROR ZephyrConfig::Init() if (settings_subsys_init() != 0) return CHIP_ERROR_PERSISTED_STORAGE_FAILED; + flash_sector flashSector; + uint32_t sectorCnt = 1; + int err = flash_area_get_sectors(SETTINGS_PARTITION, §orCnt, &flashSector); + + if (err != 0) + { + return System::MapErrorZephyr(err); + } + +#if defined(CONFIG_SETTINGS_NVS) + sSectorSize = CONFIG_SETTINGS_NVS_SECTOR_SIZE_MULT * flashSector.fs_size; +#elif defined(CONFIG_SETTINGS_ZMS) + sSectorSize = CONFIG_SETTINGS_ZMS_SECTOR_SIZE_MULT * flashSector.fs_size; +#else + return CHIP_ERROR_NOT_IMPLEMENTED; +#endif + return CHIP_NO_ERROR; } @@ -229,7 +255,9 @@ CHIP_ERROR ZephyrConfig::WriteConfigValue(Key key, uint64_t val) CHIP_ERROR ZephyrConfig::WriteConfigValueStr(Key key, const char * str) { - return WriteConfigValueStr(key, str, str ? strlen(str) : 0); + /* The max value size is smaller than the sector size by several ATE size, but the API to get the exact size is not available. + * The backend implementation validates the detailed size, so we don't need to do that here. */ + return WriteConfigValueStr(key, str, str ? strnlen(str, sSectorSize) : 0); } CHIP_ERROR ZephyrConfig::WriteConfigValueStr(Key key, const char * str, size_t strLen) @@ -291,7 +319,7 @@ void ZephyrConfig::RunConfigUnitTest() bool ZephyrConfig::BuildCounterConfigKey(::chip::Platform::PersistedStorage::Key counterId, char key[SETTINGS_MAX_NAME_LEN]) { constexpr size_t KEY_PREFIX_LEN = sizeof(NAMESPACE_COUNTERS) - 1; - const size_t keySuffixLen = strlen(counterId) + 1; // including null-character + const size_t keySuffixLen = strnlen(counterId, SETTINGS_MAX_NAME_LEN) + 1; // including null-character if (KEY_PREFIX_LEN + keySuffixLen > SETTINGS_MAX_NAME_LEN) return false;