From 9cc183a531e1e309a99784f65b15c0fb1a18ddef Mon Sep 17 00:00:00 2001 From: P Dheeraj Srujan Kumar Date: Fri, 6 May 2022 06:11:20 +0530 Subject: Update to internal 1-0.91-67 Signed-off-by: P Dheeraj Srujan Kumar --- ...gmt-Fix-for-user-privilege-race-condition.patch | 152 +++++++++++++++++++++ .../ipmi/phosphor-ipmi-host_%.bbappend | 1 + .../ipmi/phosphor-ipmi-net_%.bbappend | 10 +- 3 files changed, 154 insertions(+), 9 deletions(-) create mode 100644 meta-openbmc-mods/meta-common/recipes-phosphor/ipmi/phosphor-ipmi-host/0064-user_mgmt-Fix-for-user-privilege-race-condition.patch (limited to 'meta-openbmc-mods/meta-common/recipes-phosphor/ipmi') diff --git a/meta-openbmc-mods/meta-common/recipes-phosphor/ipmi/phosphor-ipmi-host/0064-user_mgmt-Fix-for-user-privilege-race-condition.patch b/meta-openbmc-mods/meta-common/recipes-phosphor/ipmi/phosphor-ipmi-host/0064-user_mgmt-Fix-for-user-privilege-race-condition.patch new file mode 100644 index 000000000..7f0a63b77 --- /dev/null +++ b/meta-openbmc-mods/meta-common/recipes-phosphor/ipmi/phosphor-ipmi-host/0064-user_mgmt-Fix-for-user-privilege-race-condition.patch @@ -0,0 +1,152 @@ +From 2c40b63025d39382db78dc4ec62e6fb55c7c66a7 Mon Sep 17 00:00:00 2001 +From: Jayaprakash Mutyala +Date: Fri, 1 Oct 2021 16:01:57 +0000 +Subject: [PATCH] user_mgmt: Fix for user privilege race condition + +This is porting of main line upstream review change #47382. + +The ipmid and netipmid processes cache IPMI user data in a shared file. +The current implementation has coherency and consistency problems: + +Coherence: If a user account is created and immediately enabled with +IPMI commands, the updated data may not be propagated to netipmid. This +condition can last indefinitely, so the cache is not coherent. The +problem is caused by a lock that doesn't work and allows both processes +to register signal handlers that write to the file. + +Consistency: This cache scheme does not have a strict (or linearizable) +consistency model. The ipmid and netipmid processes have an inconsistent +view of the user database until changes propagate to netipmid. Cache +file reads are controlled by mtime comparisons with a one-second +granularity. + +So mitigated the second problem by using the full 10ms resolution of +mtime. Now a new user is ready to use much faster than a client can +submit two commands. + +Mitigating the second (consistency) problem does not fix the first +problem. It might hide it, but the result will still depend on +non-deterministic timing of DBus signals and mtime granularity. + +To fix the coherency problem, changed sigHndlrLock to use a different +file that isn't closed after each operation. Closing a POSIX file lock +releases the lock. + +Tested: +1. Verified using IPMI commands by creating multiple users continuously. +Successfully created all users and able to perform RMCPP with that user. + +Command: ipmitool user set name +Response: //User created successfully +Command: ipmitool user set password +Response: Set User Password command successful +Command: ipmitool channel setaccess link=on ipmi=on + callin=on privilege=4 +Response: Set User Access (channel id ) successful. +Command: ipmitool raw 0x6 0x43 0x9 0x4 0x0 + // Set User Access Command +Response: //Success +Command: ipmitool user enable +Response: //Success +Command: ipmitool -H -U -P -C 17 -I + lanplus raw 0x06 0x01 +Response: //Success + +Signed-off-by: Luke Phillips +Signed-off-by: Jayaprakash Mutyala +Change-Id: If5ede3b0f97a2ba2b33cf358a9aaaf93d765d359 +--- + user_channel/user_mgmt.cpp | 27 +++++++++++++++++++++------ + user_channel/user_mgmt.hpp | 4 ++-- + 2 files changed, 23 insertions(+), 8 deletions(-) + +diff --git a/user_channel/user_mgmt.cpp b/user_channel/user_mgmt.cpp +index 2459de7..1644154 100644 +--- a/user_channel/user_mgmt.cpp ++++ b/user_channel/user_mgmt.cpp +@@ -70,6 +70,8 @@ static constexpr const char* getObjectMethod = "GetObject"; + static constexpr const char* ipmiUserMutex = "ipmi_usr_mutex"; + static constexpr const char* ipmiMutexCleanupLockFile = + "/var/lib/ipmi/ipmi_usr_mutex_cleanup"; ++static constexpr const char* ipmiUserSignalLockFile = ++ "/var/lib/ipmi/ipmi_usr_signal_mutex"; + static constexpr const char* ipmiUserDataFile = "/var/lib/ipmi/ipmi_user.json"; + static constexpr const char* ipmiGrpName = "ipmi"; + static constexpr size_t privNoAccess = 0xF; +@@ -1504,8 +1506,10 @@ void UserAccess::deleteUserIndex(const size_t& usrIdx) + + void UserAccess::checkAndReloadUserData() + { +- std::time_t updateTime = getUpdatedFileTime(); +- if (updateTime != fileLastUpdatedTime || updateTime == -EIO) ++ std::timespec updateTime = getUpdatedFileTime(); ++ if ((updateTime.tv_sec != fileLastUpdatedTime.tv_sec || ++ updateTime.tv_nsec != fileLastUpdatedTime.tv_nsec) || ++ (updateTime.tv_sec == 0 && updateTime.tv_nsec == 0)) + { + std::fill(reinterpret_cast(&usersTbl), + reinterpret_cast(&usersTbl) + sizeof(usersTbl), 0); +@@ -1557,15 +1561,15 @@ void UserAccess::getSystemPrivAndGroups() + return; + } + +-std::time_t UserAccess::getUpdatedFileTime() ++std::timespec UserAccess::getUpdatedFileTime() + { + struct stat fileStat; + if (stat(ipmiUserDataFile, &fileStat) != 0) + { + log("Error in getting last updated time stamp"); +- return -EIO; ++ return std::timespec{0, 0}; + } +- return fileStat.st_mtime; ++ return fileStat.st_mtim; + } + + void UserAccess::getUserProperties(const DbusUserObjProperties& properties, +@@ -1631,7 +1635,18 @@ void UserAccess::cacheUserDataFile() + } + writeUserData(); + } +- sigHndlrLock = boost::interprocess::file_lock(ipmiUserDataFile); ++ // Create lock file if it does not exist ++ int fd = open(ipmiUserSignalLockFile, O_CREAT | O_TRUNC | O_SYNC, ++ S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH); ++ if (fd < 0) ++ { ++ log("Error in creating IPMI user signal lock file"); ++ throw std::ios_base::failure( ++ "Error in creating temporary IPMI user signal lock file"); ++ } ++ close(fd); ++ ++ sigHndlrLock = boost::interprocess::file_lock(ipmiUserSignalLockFile); + // Register it for single object and single process either netipimd / + // host-ipmid + if (userUpdatedSignal == nullptr && sigHndlrLock.try_lock()) +diff --git a/user_channel/user_mgmt.hpp b/user_channel/user_mgmt.hpp +index 833f6a1..602d549 100644 +--- a/user_channel/user_mgmt.hpp ++++ b/user_channel/user_mgmt.hpp +@@ -379,7 +379,7 @@ class UserAccess + std::vector availablePrivileges; + std::vector availableGroups; + sdbusplus::bus::bus bus; +- std::time_t fileLastUpdatedTime; ++ std::timespec fileLastUpdatedTime; + bool signalHndlrObject = false; + boost::interprocess::file_lock sigHndlrLock; + boost::interprocess::file_lock mutexCleanupLock; +@@ -388,7 +388,7 @@ class UserAccess + * + * @return time stamp or -EIO for failure + */ +- std::time_t getUpdatedFileTime(); ++ std::timespec getUpdatedFileTime(); + + /** @brief function to available system privileges and groups + * +-- +2.17.1 + diff --git a/meta-openbmc-mods/meta-common/recipes-phosphor/ipmi/phosphor-ipmi-host_%.bbappend b/meta-openbmc-mods/meta-common/recipes-phosphor/ipmi/phosphor-ipmi-host_%.bbappend index 5c0908515..9225fb36d 100644 --- a/meta-openbmc-mods/meta-common/recipes-phosphor/ipmi/phosphor-ipmi-host_%.bbappend +++ b/meta-openbmc-mods/meta-common/recipes-phosphor/ipmi/phosphor-ipmi-host_%.bbappend @@ -10,6 +10,7 @@ SRC_URI += "file://phosphor-ipmi-host.service \ file://0059-Move-Set-SOL-config-parameter-to-host-ipmid.patch \ file://0060-Move-Get-SOL-config-parameter-to-host-ipmid.patch \ file://0063-Save-the-pre-timeout-interrupt-in-dbus-property.patch \ + file://0064-user_mgmt-Fix-for-user-privilege-race-condition.patch \ " EXTRA_OECONF:append = " --disable-i2c-whitelist-check" diff --git a/meta-openbmc-mods/meta-common/recipes-phosphor/ipmi/phosphor-ipmi-net_%.bbappend b/meta-openbmc-mods/meta-common/recipes-phosphor/ipmi/phosphor-ipmi-net_%.bbappend index 853d68d87..d7204990d 100644 --- a/meta-openbmc-mods/meta-common/recipes-phosphor/ipmi/phosphor-ipmi-net_%.bbappend +++ b/meta-openbmc-mods/meta-common/recipes-phosphor/ipmi/phosphor-ipmi-net_%.bbappend @@ -3,20 +3,12 @@ inherit useradd # TODO: This should be removed, once up-stream bump up # issue is resolved SRC_URI += "git://github.com/openbmc/phosphor-net-ipmid" -SRCREV = "af23add2a2cf73226cdc72af4793fde6357e8932" +SRCREV = "2528dfbdfdac5e0167d6529a25ee12b556577e1a" USERADD_PACKAGES = "${PN}" # add a group called ipmi GROUPADD_PARAM:${PN} = "ipmi " -# Default rmcpp iface is eth0; channel 1 -# Add channel 2 instance (eth1) -RMCPP_EXTRA = "eth1" -SYSTEMD_SERVICE:${PN} += " \ - ${PN}@${RMCPP_EXTRA}.service \ - ${PN}@${RMCPP_EXTRA}.socket \ - " - FILESEXTRAPATHS:prepend := "${THISDIR}/${PN}:" SRC_URI += " file://10-nice-rules.conf \ -- cgit v1.2.3