libpldmresponder: fix a potential pldm crash
In the current state, pldm does create the sensor/effecter even in the
absence of the underlying dbus resource. There is a bug in the responder
code due to which we emplace an empty dbus value map into the handler
with a sensor ID. And if the remote PLDM endpoint queries for the state
of the sensor, pldm would crash since it tries to read content from the
empty dbus value map. This commit would fix the crash by adding
necessary bound checks.
Testing:
1. Unit tests passed.
2. Functionally verified the fix by giving a dummy dbus object path and
making sure that the sensor/effecter is not created & also ensured that
pldm does not crash.
Change-Id: I5fb06677f6ae1bd74f9ec741b311f59737caf79d
Signed-off-by: Manojkiran Eda <manojkiran.eda@gmail.com>
diff --git a/libpldmresponder/pdr_numeric_effecter.hpp b/libpldmresponder/pdr_numeric_effecter.hpp
index 17ed002..b213912 100644
--- a/libpldmresponder/pdr_numeric_effecter.hpp
+++ b/libpldmresponder/pdr_numeric_effecter.hpp
@@ -52,7 +52,6 @@
sizeof(pldm_pdr_hdr);
pdr->terminus_handle = e.value("terminus_handle", 0);
- pdr->effecter_id = handler.getNextEffecterId();
try
{
@@ -220,9 +219,10 @@
error(
"D-Bus object path does not exist, effecter ID: {EFFECTER_ID}",
"EFFECTER_ID", static_cast<uint16_t>(pdr->effecter_id));
+ continue;
}
dbusMappings.emplace_back(std::move(dbusMapping));
-
+ pdr->effecter_id = handler.getNextEffecterId();
handler.addDbusObjMaps(
pdr->effecter_id,
std::make_tuple(std::move(dbusMappings), std::move(dbusValMaps)));
diff --git a/libpldmresponder/pdr_state_effecter.hpp b/libpldmresponder/pdr_state_effecter.hpp
index 68cfb00..17a7af9 100644
--- a/libpldmresponder/pdr_state_effecter.hpp
+++ b/libpldmresponder/pdr_state_effecter.hpp
@@ -70,7 +70,6 @@
pdr->hdr.length = pdrSize - sizeof(pldm_pdr_hdr);
pdr->terminus_handle = TERMINUS_HANDLE;
- pdr->effecter_id = handler.getNextEffecterId();
try
{
@@ -161,19 +160,22 @@
error(
"Failed to create effecter PDR, D-Bus object '{PATH}' returned {ERROR}",
"PATH", objectPath, "ERROR", e);
- continue;
+ break;
}
-
dbusMappings.emplace_back(std::move(dbusMapping));
dbusValMaps.emplace_back(std::move(dbusIdToValMap));
}
- handler.addDbusObjMaps(
- pdr->effecter_id,
- std::make_tuple(std::move(dbusMappings), std::move(dbusValMaps)));
- pldm::responder::pdr_utils::PdrEntry pdrEntry{};
- pdrEntry.data = entry.data();
- pdrEntry.size = pdrSize;
- repo.addRecord(pdrEntry);
+ if (!(dbusMappings.empty() && dbusValMaps.empty()))
+ {
+ pdr->effecter_id = handler.getNextEffecterId();
+ handler.addDbusObjMaps(pdr->effecter_id,
+ std::make_tuple(std::move(dbusMappings),
+ std::move(dbusValMaps)));
+ pldm::responder::pdr_utils::PdrEntry pdrEntry{};
+ pdrEntry.data = entry.data();
+ pdrEntry.size = pdrSize;
+ repo.addRecord(pdrEntry);
+ }
}
}
diff --git a/libpldmresponder/pdr_state_sensor.hpp b/libpldmresponder/pdr_state_sensor.hpp
index aa29859..ea24497 100644
--- a/libpldmresponder/pdr_state_sensor.hpp
+++ b/libpldmresponder/pdr_state_sensor.hpp
@@ -73,7 +73,6 @@
HTOLE16(pdr->hdr.length);
pdr->terminus_handle = TERMINUS_HANDLE;
- pdr->sensor_id = handler.getNextSensorId();
try
{
@@ -118,7 +117,6 @@
pdr->composite_sensor_count = sensors.size();
HTOLE16(pdr->terminus_handle);
- HTOLE16(pdr->sensor_id);
HTOLE16(pdr->entity_type);
HTOLE16(pdr->entity_instance);
HTOLE16(pdr->container_id);
@@ -173,21 +171,26 @@
error(
"Failed to create sensor PDR, D-Bus object '{PATH}' returned {ERROR}",
"PATH", objectPath, "ERROR", e);
- continue;
+ break;
}
-
dbusMappings.emplace_back(std::move(dbusMapping));
dbusValMaps.emplace_back(std::move(dbusIdToValMap));
}
- handler.addDbusObjMaps(
- pdr->sensor_id,
- std::make_tuple(std::move(dbusMappings), std::move(dbusValMaps)),
- pldm::responder::pdr_utils::TypeId::PLDM_SENSOR_ID);
- pldm::responder::pdr_utils::PdrEntry pdrEntry{};
- pdrEntry.data = entry.data();
- pdrEntry.size = pdrSize;
- repo.addRecord(pdrEntry);
+ if (!(dbusMappings.empty() && dbusValMaps.empty()))
+ {
+ pdr->sensor_id = handler.getNextSensorId();
+ HTOLE16(pdr->sensor_id);
+ handler.addDbusObjMaps(
+ pdr->sensor_id,
+ std::make_tuple(std::move(dbusMappings),
+ std::move(dbusValMaps)),
+ pldm::responder::pdr_utils::TypeId::PLDM_SENSOR_ID);
+ pldm::responder::pdr_utils::PdrEntry pdrEntry{};
+ pdrEntry.data = entry.data();
+ pdrEntry.size = pdrSize;
+ repo.addRecord(pdrEntry);
+ }
}
}
diff --git a/libpldmresponder/platform_state_effecter.hpp b/libpldmresponder/platform_state_effecter.hpp
index db70128..247dbfd 100644
--- a/libpldmresponder/platform_state_effecter.hpp
+++ b/libpldmresponder/platform_state_effecter.hpp
@@ -98,7 +98,17 @@
{
const auto& [dbusMappings,
dbusValMaps] = handler.getDbusObjMaps(effecterId);
- for (uint8_t currState = 0; currState < compEffecterCnt; ++currState)
+ if (dbusMappings.empty() || dbusValMaps.empty())
+ {
+ error("dbusMappings for effecter id : {EFFECTER_ID} is missing",
+ "EFFECTER_ID", effecterId);
+ return PLDM_ERROR;
+ }
+
+ for (uint8_t currState = 0;
+ currState < compEffecterCnt && currState < dbusMappings.size() &&
+ currState < dbusValMaps.size();
+ ++currState)
{
std::vector<StateSetNum> allowed{};
// computation is based on table 79 from DSP0248 v1.1.1
diff --git a/libpldmresponder/platform_state_sensor.hpp b/libpldmresponder/platform_state_sensor.hpp
index 12e4b04..e6f7629 100644
--- a/libpldmresponder/platform_state_sensor.hpp
+++ b/libpldmresponder/platform_state_sensor.hpp
@@ -147,18 +147,27 @@
const auto& [dbusMappings, dbusValMaps] = handler.getDbusObjMaps(
sensorId, pldm::responder::pdr_utils::TypeId::PLDM_SENSOR_ID);
+ if (dbusMappings.empty() || dbusValMaps.empty())
+ {
+ error("dbusMappings for sensor id : {SENSOR_ID} is missing",
+ "SENSOR_ID", sensorId);
+ return PLDM_ERROR;
+ }
pldm::responder::pdr_utils::EventStates sensorCacheforSensor{};
if (sensorCache.contains(sensorId))
{
sensorCacheforSensor = sensorCache.at(sensorId);
}
stateField.clear();
- for (std::size_t i{0}; i < sensorRearmCnt; i++)
+ for (std::size_t offset{0};
+ offset < sensorRearmCnt && offset < dbusMappings.size() &&
+ offset < dbusValMaps.size();
+ offset++)
{
- auto& dbusMapping = dbusMappings[i];
+ auto& dbusMapping = dbusMappings[offset];
uint8_t sensorEvent = getStateSensorEventState<DBusInterface>(
- dBusIntf, dbusValMaps[i], dbusMapping);
+ dBusIntf, dbusValMaps[offset], dbusMapping);
uint8_t previousState = PLDM_SENSOR_UNKNOWN;
@@ -166,16 +175,16 @@
// get_state_sensor_reading on this sensor, set the previous state
// as the current state
- if (sensorCacheforSensor.at(i) == PLDM_SENSOR_UNKNOWN)
+ if (sensorCacheforSensor.at(offset) == PLDM_SENSOR_UNKNOWN)
{
previousState = sensorEvent;
- handler.updateSensorCache(sensorId, i, previousState);
+ handler.updateSensorCache(sensorId, offset, previousState);
}
else
{
// sensor cache is not empty, so get the previous state from
// the sensor cache
- previousState = sensorCacheforSensor[i];
+ previousState = sensorCacheforSensor[offset];
}
uint8_t opState = PLDM_SENSOR_ENABLED;
if (sensorEvent == PLDM_SENSOR_UNKNOWN)