From d54a9e0a4194f6843406bc67f68045f59f9a190d Mon Sep 17 00:00:00 2001 From: Pawel Dziepak Date: Wed, 30 Oct 2013 02:57:45 +0100 Subject: [PATCH] kernel: Do not use gSchedulerLock when accesing UID and GID Reads and writes to uid_t and gid_t are atomic anyway. The only real problem that may happen here is inconsistent state of triples effective_{u, g}id, saved_set_{u, g}id, real_{u, g}id, but team locks protect us against that. --- src/system/kernel/signal.cpp | 15 ++++----------- src/system/kernel/usergroup.cpp | 26 +++----------------------- 2 files changed, 7 insertions(+), 34 deletions(-) diff --git a/src/system/kernel/signal.cpp b/src/system/kernel/signal.cpp index a28533f645..1d61b15202 100644 --- a/src/system/kernel/signal.cpp +++ b/src/system/kernel/signal.cpp @@ -315,7 +315,6 @@ Signal::SetTo(uint32 number) fErrorCode = 0; fSendingProcess = team->id; fSendingUser = team->effective_uid; - // assuming scheduler lock is being held fStatus = 0; fPollBand = 0; fAddress = NULL; @@ -1322,19 +1321,13 @@ has_signals_pending(Thread* thread) /*! Checks whether the current user has permission to send a signal to the given target team. - The caller must hold the scheduler lock or \a team's lock. - \param team The target team. - \param schedulerLocked \c true, if the caller holds the scheduler lock, - \c false otherwise. */ static bool -has_permission_to_signal(Team* team, bool schedulerLocked) +has_permission_to_signal(Signal* signal, Team* team) { // get the current user - uid_t currentUser = schedulerLocked - ? thread_get_current_thread()->team->effective_uid - : geteuid(); + uid_t currentUser = signal->SendingUser(); // root is omnipotent -- in the other cases the current user must match the // target team's @@ -1372,7 +1365,7 @@ send_signal_to_thread_locked(Thread* thread, uint32 signalNumber, BReference signalReference(signal, true); if ((flags & B_CHECK_PERMISSION) != 0) { - if (!has_permission_to_signal(thread->team, true)) + if (!has_permission_to_signal(signal, thread->team)) return EPERM; } @@ -1566,7 +1559,7 @@ send_signal_to_team_locked(Team* team, uint32 signalNumber, Signal* signal, BReference signalReference(signal, true); if ((flags & B_CHECK_PERMISSION) != 0) { - if (!has_permission_to_signal(team, true)) + if (!has_permission_to_signal(signal, team)) return EPERM; } diff --git a/src/system/kernel/usergroup.cpp b/src/system/kernel/usergroup.cpp index 5a54ea07c9..9be52f5a0c 100644 --- a/src/system/kernel/usergroup.cpp +++ b/src/system/kernel/usergroup.cpp @@ -54,7 +54,6 @@ common_setregid(gid_t rgid, gid_t egid, bool setAllIfPrivileged, bool kernel) // setgid() semantics: If privileged set both, real, effective and // saved set-gid, otherwise set the effective gid. if (privileged) { - InterruptsSpinLocker schedulerLocker(gSchedulerLock); team->saved_set_gid = rgid; team->real_gid = rgid; team->effective_gid = rgid; @@ -91,7 +90,6 @@ common_setregid(gid_t rgid, gid_t egid, bool setAllIfPrivileged, bool kernel) } // Getting here means all checks were successful -- set the gids. - InterruptsSpinLocker schedulerLocker(gSchedulerLock); team->real_gid = rgid; team->effective_gid = egid; team->saved_set_gid = ssgid; @@ -119,7 +117,6 @@ common_setreuid(uid_t ruid, uid_t euid, bool setAllIfPrivileged, bool kernel) // setuid() semantics: If privileged set both, real, effective and // saved set-uid, otherwise set the effective uid. if (privileged) { - InterruptsSpinLocker schedulerLocker(gSchedulerLock); team->saved_set_uid = ruid; team->real_uid = ruid; team->effective_uid = ruid; @@ -156,7 +153,6 @@ common_setreuid(uid_t ruid, uid_t euid, bool setAllIfPrivileged, bool kernel) } // Getting here means all checks were successful -- set the uids. - InterruptsSpinLocker schedulerLocker(gSchedulerLock); team->real_uid = ruid; team->effective_uid = euid; team->saved_set_uid = ssuid; @@ -253,8 +249,6 @@ common_setgroups(int groupCount, const gid_t* groupList, bool kernel) void inherit_parent_user_and_group(Team* team, Team* parent) { - InterruptsSpinLocker schedulerLocker(gSchedulerLock); - team->saved_set_uid = parent->saved_set_uid; team->real_uid = parent->real_uid; team->effective_uid = parent->effective_uid; @@ -262,8 +256,6 @@ inherit_parent_user_and_group(Team* team, Team* parent) team->real_gid = parent->real_gid; team->effective_gid = parent->effective_gid; - schedulerLocker.Unlock(); - malloc_referenced_acquire(parent->supplementary_groups); team->supplementary_groups = parent->supplementary_groups; team->supplementary_group_count = parent->supplementary_group_count; @@ -279,7 +271,6 @@ update_set_id_user_and_group(Team* team, const char* file) return status; TeamLocker teamLocker(team); - InterruptsSpinLocker schedulerLocker(gSchedulerLock); if ((st.st_mode & S_ISUID) != 0) { team->saved_set_uid = st.st_uid; @@ -300,8 +291,6 @@ _kern_getgid(bool effective) { Team* team = thread_get_current_thread()->team; - InterruptsSpinLocker schedulerLocker(gSchedulerLock); - return effective ? team->effective_gid : team->real_gid; } @@ -311,8 +300,6 @@ _kern_getuid(bool effective) { Team* team = thread_get_current_thread()->team; - InterruptsSpinLocker schedulerLocker(gSchedulerLock); - return effective ? team->effective_uid : team->real_uid; } @@ -353,8 +340,6 @@ _user_getgid(bool effective) { Team* team = thread_get_current_thread()->team; - TeamLocker teamLocker(team); - return effective ? team->effective_gid : team->real_gid; } @@ -364,8 +349,6 @@ _user_getuid(bool effective) { Team* team = thread_get_current_thread()->team; - TeamLocker teamLocker(team); - return effective ? team->effective_uid : team->real_uid; } @@ -395,12 +378,9 @@ ssize_t _user_setgroups(int groupCount, const gid_t* groupList) { // check privilege - { - Team* team = thread_get_current_thread()->team; - TeamLocker teamLocker(team); - if (!is_privileged(team)) - return EPERM; - } + Team* team = thread_get_current_thread()->team; + if (!is_privileged(team)) + return EPERM; return common_setgroups(groupCount, groupList, false); }