git: f872814e2d7a - stable/13 - cred: proc_set_cred(), proc_unset_cred(): Update user's process count
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Fri, 17 Jan 2025 12:27:20 UTC
The branch stable/13 has been updated by olce:
URL: https://cgit.FreeBSD.org/src/commit/?id=f872814e2d7a8841411569fc707b028463c7656b
commit f872814e2d7a8841411569fc707b028463c7656b
Author: Olivier Certner <olce@FreeBSD.org>
AuthorDate: 2024-08-02 15:57:51 +0000
Commit: Olivier Certner <olce@FreeBSD.org>
CommitDate: 2025-01-17 12:24:53 +0000
cred: proc_set_cred(), proc_unset_cred(): Update user's process count
As a process really changes credentials at the moment proc_set_cred() or
proc_unset_cred() is called, these functions are the proper locations to
perform the update of the new and old real users' process count (using
chgproccnt()).
Before this change, change_ruid() instead would perform that update,
although it operates only on a passed credential which is a priori not
tied to the calling process (or not to any process at all). This was
arguably a flaw of commit b1fc0ec1a7a49ded, r77183, based on its commit
message, and in particular the portion "(...) In each case, the call now
acts on a credential not a process (...)".
Fixing this makes using change_ruid() more natural when building
candidate credentials that in the end are not applied to a process,
e.g., because of some intervening privilege check. Also, it removes
a hack around this unwanted process count change in unionfs.
We also introduce the new proc_set_cred_enforce_proc_lim() so that
callers can respect the per-user process limit, and will use it for the
upcoming setcred(). We plan to change all callers of proc_set_cred() to
call this new function instead at some point. In the meantime, both
proc_set_cred() and the new function will coexist.
As detailed in some proc_set_cred_enforce_proc_lim()'s comment, checking
against the process limit is currently flawed as the kernel doesn't
really maintain the number of processes per UID (besides RLIMIT_NPROC,
this in fact also applies to RLIMIT_KQUEUES, RLIMIT_NPTS, RLIMIT_SBSIZE
and RLIMIT_SWAP). The applied limit is currently that of the old real
UID. Root (or a process granted with PRIV_PROC_LIMIT) is not subject to
this limit.
Approved by: markj (mentor)
Fixes: b1fc0ec1a7a49ded
MFC after: 2 weeks
Sponsored by: The FreeBSD Foundation
Differential Revision: https://reviews.freebsd.org/D46923
(cherry picked from commit d2be7ed63affd8af5fe6203002b7cc3cbe7f7891)
Additional changes for this MFC:
1. <sys/types.h> was added as an include in <sys/ucred.h>, as some of
its types are necessary whether the header is included by the kernel
or userland. Some later -CURRENT commits added it, but are not
planned to be MFCed (mac_do(4) series, which doesn't exist in
stable/13).
2. A number of files in 'lib/libprocstat' that include (indirectly)
<sys/ucred.h> with _KERNEL defined were patched to include
<stdbool.h> beforehand, so that 'bool', which is part of the new
signature for proc_set_cred*(), is defined when <sys/ucred.h> is
processed (<sys/types.h> does not define it when _KERNEL is defined).
---
lib/libprocstat/msdosfs.c | 1 +
lib/libprocstat/smbfs.c | 2 ++
lib/libprocstat/udf.c | 2 ++
lib/libprocstat/zfs.c | 1 +
sys/fs/unionfs/union_subr.c | 6 ----
sys/kern/kern_exit.c | 10 ++----
sys/kern/kern_fork.c | 2 +-
sys/kern/kern_prot.c | 81 +++++++++++++++++++++++++++++++++++----------
sys/sys/ucred.h | 6 ++--
9 files changed, 78 insertions(+), 33 deletions(-)
diff --git a/lib/libprocstat/msdosfs.c b/lib/libprocstat/msdosfs.c
index 2af34f856e50..7e73360757bc 100644
--- a/lib/libprocstat/msdosfs.c
+++ b/lib/libprocstat/msdosfs.c
@@ -35,6 +35,7 @@
* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
* SUCH DAMAGE.
*/
+#include <stdbool.h>
#include <sys/cdefs.h>
#include <sys/param.h>
diff --git a/lib/libprocstat/smbfs.c b/lib/libprocstat/smbfs.c
index f705ccd207dd..854fd3eb986c 100644
--- a/lib/libprocstat/smbfs.c
+++ b/lib/libprocstat/smbfs.c
@@ -25,6 +25,8 @@
* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
* SUCH DAMAGE.
*/
+#include <stdbool.h>
+
#include <sys/cdefs.h>
#include <sys/param.h>
#include <sys/stat.h>
diff --git a/lib/libprocstat/udf.c b/lib/libprocstat/udf.c
index 43e79c39a62d..dc6cadeeea6d 100644
--- a/lib/libprocstat/udf.c
+++ b/lib/libprocstat/udf.c
@@ -25,6 +25,8 @@
* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
* SUCH DAMAGE.
*/
+#include <stdbool.h>
+
#include <sys/cdefs.h>
#include <sys/param.h>
#include <sys/stat.h>
diff --git a/lib/libprocstat/zfs.c b/lib/libprocstat/zfs.c
index 9ca43d6f6331..a84083a054db 100644
--- a/lib/libprocstat/zfs.c
+++ b/lib/libprocstat/zfs.c
@@ -25,6 +25,7 @@
* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
* SUCH DAMAGE.
*/
+#include <stdbool.h>
#include <sys/param.h>
#define _KERNEL
diff --git a/sys/fs/unionfs/union_subr.c b/sys/fs/unionfs/union_subr.c
index 22c8ffe88bde..56c16fc9ed6e 100644
--- a/sys/fs/unionfs/union_subr.c
+++ b/sys/fs/unionfs/union_subr.c
@@ -775,11 +775,6 @@ unionfs_mkshadowdir(struct unionfs_mount *ump, struct vnode *udvp,
/* Authority change to root */
rootinfo = uifind((uid_t)0);
cred = crdup(cnp->cn_cred);
- /*
- * The calls to chgproccnt() are needed to compensate for change_ruid()
- * calling chgproccnt().
- */
- chgproccnt(cred->cr_ruidinfo, 1, 0);
change_euid(cred, rootinfo);
change_ruid(cred, rootinfo);
change_svuid(cred, (uid_t)0);
@@ -831,7 +826,6 @@ unionfs_mkshadowdir_free_out:
unionfs_mkshadowdir_abort:
cnp->cn_cred = credbk;
- chgproccnt(cred->cr_ruidinfo, -1, 0);
crfree(cred);
return (error);
diff --git a/sys/kern/kern_exit.c b/sys/kern/kern_exit.c
index bd820aae8197..64fef905daa4 100644
--- a/sys/kern/kern_exit.c
+++ b/sys/kern/kern_exit.c
@@ -976,11 +976,6 @@ proc_reap(struct thread *td, struct proc *p, int *status, int options)
ruadd(&q->p_stats->p_cru, &q->p_crux, &p->p_ru, &p->p_rux);
PROC_UNLOCK(q);
- /*
- * Decrement the count of procs running with this uid.
- */
- (void)chgproccnt(p->p_ucred->cr_ruidinfo, -1, 0);
-
/*
* Destroy resource accounting information associated with the process.
*/
@@ -994,9 +989,10 @@ proc_reap(struct thread *td, struct proc *p, int *status, int options)
racct_proc_exit(p);
/*
- * Free credentials, arguments, and sigacts.
+ * Free credentials, arguments, and sigacts, and decrement the count of
+ * processes running with this uid.
*/
- proc_unset_cred(p);
+ proc_unset_cred(p, true);
pargs_drop(p->p_args);
p->p_args = NULL;
sigacts_free(p->p_sigacts);
diff --git a/sys/kern/kern_fork.c b/sys/kern/kern_fork.c
index 413453f9bf8f..29b3e5d8d682 100644
--- a/sys/kern/kern_fork.c
+++ b/sys/kern/kern_fork.c
@@ -1087,7 +1087,7 @@ fail0:
#endif
racct_proc_exit(newproc);
fail1:
- proc_unset_cred(newproc);
+ proc_unset_cred(newproc, false);
fail2:
if (vm2 != NULL)
vmspace_free(vm2);
diff --git a/sys/kern/kern_prot.c b/sys/kern/kern_prot.c
index 755bdca4fd73..655ee776b6a7 100644
--- a/sys/kern/kern_prot.c
+++ b/sys/kern/kern_prot.c
@@ -564,7 +564,7 @@ sys_setuid(struct thread *td, struct setuid_args *uap)
#endif
{
/*
- * Set the real uid and transfer proc count to new user.
+ * Set the real uid.
*/
if (uid != oldcred->cr_ruid) {
change_ruid(newcred, uip);
@@ -590,6 +590,9 @@ sys_setuid(struct thread *td, struct setuid_args *uap)
change_euid(newcred, uip);
setsugid(p);
}
+ /*
+ * This also transfers the proc count to the new user.
+ */
proc_set_cred(p, newcred);
#ifdef RACCT
racct_proc_ucred_changed(p, oldcred, newcred);
@@ -2276,31 +2279,76 @@ cru2xt(struct thread *td, struct xucred *xcr)
/*
* Change process credentials.
+ *
* Callers are responsible for providing the reference for passed credentials
- * and for freeing old ones.
+ * and for freeing old ones. Calls chgproccnt() to correctly account the
+ * current process to the proper real UID, if the latter has changed. Returns
+ * whether the operation was successful. Failure can happen only on
+ * 'enforce_proc_lim' being true and if no new process can be accounted to the
+ * new real UID because of the current limit (see the inner comment for more
+ * details) and the caller does not have privilege (PRIV_PROC_LIMIT) to override
+ * that.
*/
-void
-proc_set_cred(struct proc *p, struct ucred *newcred)
+static bool
+_proc_set_cred(struct proc *p, struct ucred *newcred, bool enforce_proc_lim)
{
- struct ucred *cr;
+ struct ucred *const oldcred = p->p_ucred;
- cr = p->p_ucred;
- MPASS(cr != NULL);
+ MPASS(oldcred != NULL);
PROC_LOCK_ASSERT(p, MA_OWNED);
KASSERT(newcred->cr_users == 0, ("%s: users %d not 0 on cred %p",
__func__, newcred->cr_users, newcred));
- mtx_lock(&cr->cr_mtx);
- KASSERT(cr->cr_users > 0, ("%s: users %d not > 0 on cred %p",
- __func__, cr->cr_users, cr));
- cr->cr_users--;
- mtx_unlock(&cr->cr_mtx);
+ KASSERT(newcred->cr_ref == 1, ("%s: ref %ld not 1 on cred %p",
+ __func__, newcred->cr_ref, newcred));
+
+ if (newcred->cr_ruidinfo != oldcred->cr_ruidinfo) {
+ /*
+ * XXXOC: This check is flawed but nonetheless the best we can
+ * currently do as we don't really track limits per UID contrary
+ * to what we pretend in setrlimit(2). Until this is reworked,
+ * we just check here that the number of processes for our new
+ * real UID doesn't exceed this process' process number limit
+ * (which is meant to be associated with the current real UID).
+ */
+ const int proccnt_changed = chgproccnt(newcred->cr_ruidinfo, 1,
+ enforce_proc_lim ? lim_cur_proc(p, RLIMIT_NPROC) : 0);
+
+ if (!proccnt_changed) {
+ if (priv_check_cred(oldcred, PRIV_PROC_LIMIT) != 0)
+ return (false);
+ (void)chgproccnt(newcred->cr_ruidinfo, 1, 0);
+ }
+ }
+
+ mtx_lock(&oldcred->cr_mtx);
+ KASSERT(oldcred->cr_users > 0, ("%s: users %d not > 0 on cred %p",
+ __func__, oldcred->cr_users, oldcred));
+ oldcred->cr_users--;
+ mtx_unlock(&oldcred->cr_mtx);
p->p_ucred = newcred;
newcred->cr_users = 1;
PROC_UPDATE_COW(p);
+ if (newcred->cr_ruidinfo != oldcred->cr_ruidinfo)
+ (void)chgproccnt(oldcred->cr_ruidinfo, -1, 0);
+ return (true);
+}
+
+void
+proc_set_cred(struct proc *p, struct ucred *newcred)
+{
+ bool success = _proc_set_cred(p, newcred, false);
+
+ MPASS(success);
+}
+
+bool
+proc_set_cred_enforce_proc_lim(struct proc *p, struct ucred *newcred)
+{
+ return (_proc_set_cred(p, newcred, true));
}
void
-proc_unset_cred(struct proc *p)
+proc_unset_cred(struct proc *p, bool decrement_proc_count)
{
struct ucred *cr;
@@ -2315,6 +2363,8 @@ proc_unset_cred(struct proc *p)
KASSERT(cr->cr_ref > 0, ("%s: ref %d not > 0 on cred %p",
__func__, cr->cr_ref, cr));
mtx_unlock(&cr->cr_mtx);
+ if (decrement_proc_count)
+ (void)chgproccnt(cr->cr_ruidinfo, -1, 0);
crfree(cr);
}
@@ -2599,8 +2649,7 @@ change_egid(struct ucred *newcred, gid_t egid)
/*-
* Change a process's real uid.
* Side effects: newcred->cr_ruid will be updated, newcred->cr_ruidinfo
- * will be updated, and the old and new cr_ruidinfo proc
- * counts will be updated.
+ * will be updated.
* References: newcred must be an exclusive credential reference for the
* duration of the call.
*/
@@ -2608,12 +2657,10 @@ void
change_ruid(struct ucred *newcred, struct uidinfo *ruip)
{
- (void)chgproccnt(newcred->cr_ruidinfo, -1, 0);
newcred->cr_ruid = ruip->ui_uid;
uihold(ruip);
uifree(newcred->cr_ruidinfo);
newcred->cr_ruidinfo = ruip;
- (void)chgproccnt(newcred->cr_ruidinfo, 1, 0);
}
/*-
diff --git a/sys/sys/ucred.h b/sys/sys/ucred.h
index f5e2757f7417..d8729b49c2dd 100644
--- a/sys/sys/ucred.h
+++ b/sys/sys/ucred.h
@@ -34,6 +34,7 @@
#ifndef _SYS_UCRED_H_
#define _SYS_UCRED_H_
+#include <sys/types.h>
#if defined(_KERNEL) || defined(_WANT_UCRED)
#include <sys/_lock.h>
#include <sys/_mutex.h>
@@ -158,8 +159,9 @@ void crcopy(struct ucred *dest, struct ucred *src);
struct ucred *crcopysafe(struct proc *p, struct ucred *cr);
struct ucred *crdup(struct ucred *cr);
void crextend(struct ucred *cr, int n);
-void proc_set_cred(struct proc *p, struct ucred *cr);
-void proc_unset_cred(struct proc *p);
+void proc_set_cred(struct proc *p, struct ucred *newcred);
+bool proc_set_cred_enforce_proc_lim(struct proc *p, struct ucred *newcred);
+void proc_unset_cred(struct proc *p, bool decrement_proc_count);
void crfree(struct ucred *cr);
struct ucred *crcowsync(void);
struct ucred *crget(void);