dbus-sdr: Add sensor mutability
Sensor ValueMutability interface has already been merged into
openbmc/phosphor-dbus-interfaces here:
https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-dbus-interfaces/+/36333
This change adds the IPMI server-side changes, namely, the test for
the "Mutable" member of this interface existing and being "true".
If so, it grants external write permission to this sensor, otherwise,
it will remain read-only (which is the default).
It replaces a previous compile-time constant that could only be changed
at compilation time, and would affect all sensors globally, neither of
which was desirable.
This "Mutable" interface boolean can be used to grant write
permission to sensors, such as external sensors and fan PWM sensors in
manual mode. IPMI setting sensor reading will check the mutability
first.
It achieves feature parity with the old
"mutability: Mutability::Write|Mutability::Read" settings,
in the old hardcoded YAML configuration files.
Also see the dbus-sensors changes, namely, the reading of this
parameter from entity-manager configuration, and setting
this D-Bus property accordingly, if "Mutable" is true:
https://gerrit.openbmc-project.xyz/c/openbmc/dbus-sensors/+/45405
Tested: With both 45405 and 45407 changes in, this feature has been
working nicely in our local environment for some time now.
Signed-off-by: Jie Yang <jjy@google.com>
Change-Id: I4ecff1a0424c0bc23d3a90466e1bb4b655f07859
Signed-off-by: Josh Lehan <krellan@google.com>
diff --git a/configure.ac b/configure.ac
index a7586b7..8ec8c40 100644
--- a/configure.ac
+++ b/configure.ac
@@ -266,25 +266,6 @@
)
AM_CONDITIONAL([FEATURE_IPMI_WHITELIST], [test x$ipmi_whitelist = xtrue])
-# Dynamic sensors stack write permission is disabled by default; offer a way to enable it
-# Change to true if you wish to allow external IPMI users to modify your sensor
-# values, and you are OK with the security implications of doing so.
-AC_ARG_ENABLE([dynamic_sensors_write],
- [ --enable-dynamic_sensors_write Enable/disable Dynamic Sensors writes],
- [case "${enableval}" in
- yes) dynamic_sensors_write=true ;;
- no) dynamic_sensors_write=false ;;
- *) AC_MSG_ERROR([bad value ${enableval} for --enable-dynamic_sensors_write]) ;;
- esac],[dynamic_sensors_write=false]
- )
-
-AS_IF([test x$dynamic_sensors_write = xtrue],
- AC_MSG_NOTICE([Enabling dynamic sensors write feature])
- [cpp_flags="$cpp_flags -DFEATURE_DYNAMIC_SENSORS_WRITE"]
- AC_SUBST([CPPFLAGS], [$cpp_flags]),
- AC_MSG_WARN([Disabling dynamic sensors write feature])
-)
-
# Dynamic sensors stack is enabled by default; offer a way to disable it
AC_ARG_ENABLE([dynamic_sensors],
[ --enable-dynamic_sensors Enable/disable Dynamic Sensors stack],
diff --git a/dbus-sdr/sdrutils.cpp b/dbus-sdr/sdrutils.cpp
index ede14bf..3df0dc1 100644
--- a/dbus-sdr/sdrutils.cpp
+++ b/dbus-sdr/sdrutils.cpp
@@ -93,6 +93,7 @@
// Add sensors to SensorTree
static constexpr const std::array sensorInterfaces = {
"xyz.openbmc_project.Sensor.Value",
+ "xyz.openbmc_project.Sensor.ValueMutability",
"xyz.openbmc_project.Sensor.Threshold.Warning",
"xyz.openbmc_project.Sensor.Threshold.Critical"};
static constexpr const std::array vrInterfaces = {
@@ -139,6 +140,7 @@
sensorUpdatedIndex++;
// The SDR is being regenerated, wipe the old stats
sdrStatsTable.wipeTable();
+ sdrWriteTable.wipeTable();
return sensorUpdatedIndex;
}
diff --git a/dbus-sdr/sensorcommands.cpp b/dbus-sdr/sensorcommands.cpp
index 3cebeb0..854d55f 100644
--- a/dbus-sdr/sensorcommands.cpp
+++ b/dbus-sdr/sensorcommands.cpp
@@ -545,6 +545,12 @@
return ipmi::responseResponseError();
}
+ // Only allow external SetSensor if write permission granted
+ if (!details::sdrWriteTable.getWritePermission(sensorNumber))
+ {
+ return ipmi::responseResponseError();
+ }
+
auto value =
sensor::calculateValue(reading, sensorMap, sensorObject->second);
if (!value)
@@ -1605,10 +1611,18 @@
// Remember the sensor name, as determined for this sensor number
details::sdrStatsTable.updateName(sensornumber, name);
-#ifdef FEATURE_DYNAMIC_SENSORS_WRITE
- // Set the sensor settable state to true by default
- get_sdr::body::init_settable_state(true, &record.body);
-#endif
+ bool sensorSettable = false;
+ auto mutability =
+ sensorMap.find("xyz.openbmc_project.Sensor.ValueMutability");
+ if (mutability != sensorMap.end())
+ {
+ sensorSettable =
+ mappedVariant<bool>(mutability->second, "Mutable", false);
+ }
+ get_sdr::body::init_settable_state(sensorSettable, &record.body);
+
+ // Grant write permission to sensors deemed externally settable
+ details::sdrWriteTable.setWritePermission(sensornumber, sensorSettable);
IPMIThresholds thresholdData;
try
@@ -2212,12 +2226,10 @@
ipmi::sensor_event::cmdPlatformEvent,
ipmi::Privilege::Operator, ipmiSenPlatformEvent);
-#ifdef FEATURE_DYNAMIC_SENSORS_WRITE
// <Set Sensor Reading and Event Status>
ipmi::registerHandler(ipmi::prioOpenBmcBase, ipmi::netFnSensor,
ipmi::sensor_event::cmdSetSensorReadingAndEvtSts,
ipmi::Privilege::Operator, ipmiSetSensorReading);
-#endif
// <Get Sensor Reading>
ipmi::registerHandler(ipmi::prioOpenBmcBase, ipmi::netFnSensor,
diff --git a/include/dbus-sdr/sdrutils.hpp b/include/dbus-sdr/sdrutils.hpp
index 3933245..cfd76b3 100644
--- a/include/dbus-sdr/sdrutils.hpp
+++ b/include/dbus-sdr/sdrutils.hpp
@@ -209,9 +209,61 @@
}
};
+class IPMIWriteEntry
+{
+ private:
+ bool writePermission = false;
+
+ public:
+ bool getWritePermission(void) const
+ {
+ return writePermission;
+ }
+
+ void setWritePermission(bool permission)
+ {
+ writePermission = permission;
+ }
+};
+
+class IPMIWriteTable
+{
+ private:
+ std::vector<IPMIWriteEntry> entries;
+
+ private:
+ void padEntries(size_t index)
+ {
+ // Pad vector until entries[index] becomes a valid index
+ if (entries.size() <= index)
+ {
+ entries.resize(index + 1);
+ }
+ }
+
+ public:
+ void wipeTable(void)
+ {
+ entries.clear();
+ }
+
+ bool getWritePermission(size_t index)
+ {
+ padEntries(index);
+ return entries[index].getWritePermission();
+ }
+
+ void setWritePermission(size_t index, bool permission)
+ {
+ padEntries(index);
+ entries[index].setWritePermission(permission);
+ }
+};
+
// Store information for threshold sensors and they are not used by VR
// sensors. These objects are global singletons, used from a variety of places.
inline IPMIStatsTable sdrStatsTable;
+inline IPMIWriteTable sdrWriteTable;
/**
* Search ObjectMapper for sensors and update them to subtree.