git: d2be7ed63aff - main - cred: proc_set_cred(), proc_unset_cred(): Update user's process count
Date: Mon, 16 Dec 2024 14:45:34 UTC
The branch main has been updated by olce:
URL: https://cgit.FreeBSD.org/src/commit/?id=d2be7ed63affd8af5fe6203002b7cc3cbe7f7891
commit d2be7ed63affd8af5fe6203002b7cc3cbe7f7891
Author: Olivier Certner <olce@FreeBSD.org>
AuthorDate: 2024-08-02 15:57:51 +0000
Commit: Olivier Certner <olce@FreeBSD.org>
CommitDate: 2024-12-16 14:42:32 +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
---
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 | 5 +--
5 files changed, 71 insertions(+), 33 deletions(-)
diff --git a/sys/fs/unionfs/union_subr.c b/sys/fs/unionfs/union_subr.c
index b731c562f97d..edcc6716b674 100644
--- a/sys/fs/unionfs/union_subr.c
+++ b/sys/fs/unionfs/union_subr.c
@@ -920,11 +920,6 @@ unionfs_mkshadowdir(struct vnode *dvp, struct vnode *vp,
/* 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);
@@ -1046,7 +1041,6 @@ unionfs_mkshadowdir_relock:
unionfs_mkshadowdir_finish:
unionfs_clear_in_progress_flag(vp, UNIONFS_COPY_IN_PROGRESS);
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 f6263cd46d06..a67d6b422964 100644
--- a/sys/kern/kern_exit.c
+++ b/sys/kern/kern_exit.c
@@ -999,11 +999,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.
*/
@@ -1017,9 +1012,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 a66bc4be62a8..9deb21aca11d 100644
--- a/sys/kern/kern_fork.c
+++ b/sys/kern/kern_fork.c
@@ -1086,7 +1086,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 17917d2c3360..c51210a2b29b 100644
--- a/sys/kern/kern_prot.c
+++ b/sys/kern/kern_prot.c
@@ -568,7 +568,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);
@@ -594,6 +594,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);
@@ -2279,31 +2282,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;
@@ -2318,6 +2366,8 @@ proc_unset_cred(struct proc *p)
KASSERT(cr->cr_ref > 0, ("%s: ref %ld 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);
}
@@ -2602,8 +2652,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.
*/
@@ -2611,12 +2660,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 25dff911241b..ecc624dd76e6 100644
--- a/sys/sys/ucred.h
+++ b/sys/sys/ucred.h
@@ -156,8 +156,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);