On Sat, Dec 03, 2022 at 08:45:52PM +0300, Vitaliy Makkoveev wrote:
> On Thu, Dec 01, 2022 at 10:50:03PM +0300, Vitaliy Makkoveev wrote:
> > On Thu, Dec 01, 2022 at 11:28:59AM -0800, Philip Guenther wrote:
> > > On Thu, Dec 1, 2022 at 10:31 AM Vitaliy Makkoveev <m...@openbsd.org>
> > > wrote:
> > > ...
> > >
> > > > --- sys/sys/sysctl.h 7 Nov 2022 14:25:44 -0000 1.231
> > > > +++ sys/sys/sysctl.h 1 Dec 2022 18:15:06 -0000
> > > > @@ -587,7 +587,7 @@ struct kinfo_vmentry {
> > > >
> > > > #define _FILL_KPROC_MIN(a,b) (((a)<(b))?(a):(b))
> > > >
> > > > -#define FILL_KPROC(kp, copy_str, p, pr, uc, pg, paddr, \
> > > > +#define FILL_KPROC(kp, copy_str, p, pr, pg, paddr, \
> > > > praddr, sess, vm, lim, sa, isthread, show_addresses) \
> > > >
> > > ...
> > >
> > > > - (kp)->p_svgid = (uc)->cr_svgid;
> > > > \
> > > > + PR_LOCK(pr);
> > > > \
> > > > + (kp)->p_uid = (pr)->ps_ucred->cr_uid;
> > > > \
> > > >
> > >
> > > Nope. As the block comment about this notes, FILL_KPROC() is shared
> > > between the kernel and libkvm and takes each structure pointer separately
> > > as, for example, pr->ps_ucred has the kva address, not the address of the
> > > ucred struct that libkvm has separately read into user memory.
> > >
> > > Now, you _could_ have libkvm update pr->ps_ucred to point to its
> > > user-space
> > > copy. However, that would make ucred handling different from the other
> > > sub-structures of struct proc and MOST of those we need the real kva for
> > > the show_address functionality.
> > >
> > > Not sure if this is the yak-shave you want right now...
> > >
> > > (libkvm will obviously also need no-op #defines for PR_LOCK() etc)
> > >
> >
> > I missed this.
> >
> > Since `ps_ucred' is immutable, we could bump it's reference and use it
> > without holding `ps_mtx':
> >
> > mtx_enter(&pr->ps_mtx);
> > prucred = crhold(pr->ps_ucred);
> > mtx_leave(&pr->ps_mtx);
> >
> > FILL_KPROC(ki, strlcpy, p, pr, prucred, pr->ps_pgrp, ...);
> > crfree(prucred);
> >
> > Otherwise, we could grab `ps_mtx' mutex outside FILL_KPROC(), so dummy
> > PR_LOCK() define will be not required in userland.
> >
> > The diff below follows the first way.
> >
>
> A little update. Kernel lock is not required within dorefreshcreds()
> because `ps_mtx' mutex(9) is taken. Also forgotten makesyscalls.sh
> generated chunks removed.
>
>
Anyone?
The `pr' locking for FILL_KPROC() usage is discussible. We already use
PR_LOCK() for `sa' copying, which is used within FILL_KPROC() as `sa',
but not pr->ps_sigacts. So the same could be done for `uc' copying too:
#define FILL_KPROC(kp, copy_str, p, pr, uc, pg, paddr, \
praddr, sess, vm, lim, sa, isthread, show_addresses) \
do { \
PR_LOCK(pr); \
(kp)->p_uid = (uc)->cr_uid; \
(kp)->p_ruid = (uc)->cr_ruid; \
/* ... */ \
PR_UNLOCK(pr); \
PR_LOCK(pr); \
(kp)->p_sigignore = (sa) ? (sa)->ps_sigignore : 0; \
/* ... */ \
PR_UNLOCK(pr); \
> Index: sys/kern/kern_acct.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_acct.c,v
> retrieving revision 1.47
> diff -u -p -r1.47 kern_acct.c
> --- sys/kern/kern_acct.c 14 Aug 2022 01:58:27 -0000 1.47
> +++ sys/kern/kern_acct.c 3 Dec 2022 17:39:00 -0000
> @@ -221,8 +221,10 @@ acct_process(struct proc *p)
> acct.ac_io = encode_comp_t(r->ru_inblock + r->ru_oublock, 0);
>
> /* (6) The UID and GID of the process */
> + mtx_enter(&pr->ps_mtx);
> acct.ac_uid = pr->ps_ucred->cr_ruid;
> acct.ac_gid = pr->ps_ucred->cr_rgid;
> + mtx_leave(&pr->ps_mtx);
>
> /* (7) The terminal from which the process was started */
> if ((pr->ps_flags & PS_CONTROLT) &&
> Index: sys/kern/kern_exec.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_exec.c,v
> retrieving revision 1.240
> diff -u -p -r1.240 kern_exec.c
> --- sys/kern/kern_exec.c 23 Nov 2022 11:00:27 -0000 1.240
> +++ sys/kern/kern_exec.c 3 Dec 2022 17:39:00 -0000
> @@ -649,9 +649,11 @@ sys_execve(struct proc *p, void *v, regi
> if (pr->ps_ucred != cred) {
> struct ucred *ocred;
>
> - ocred = pr->ps_ucred;
> crhold(cred);
> + mtx_enter(&pr->ps_mtx);
> + ocred = pr->ps_ucred;
> pr->ps_ucred = cred;
> + mtx_leave(&pr->ps_mtx);
> crfree(ocred);
> }
>
> Index: sys/kern/kern_exit.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_exit.c,v
> retrieving revision 1.207
> diff -u -p -r1.207 kern_exit.c
> --- sys/kern/kern_exit.c 3 Nov 2022 04:56:47 -0000 1.207
> +++ sys/kern/kern_exit.c 3 Dec 2022 17:39:00 -0000
> @@ -494,7 +494,9 @@ loop:
> *retval = pr->ps_pid;
> if (info != NULL) {
> info->si_pid = pr->ps_pid;
> + mtx_enter(&pr->ps_mtx);
> info->si_uid = pr->ps_ucred->cr_uid;
> + mtx_leave(&pr->ps_mtx);
> info->si_signo = SIGCHLD;
> if (pr->ps_xsig == 0) {
> info->si_code = CLD_EXITED;
> @@ -530,7 +532,9 @@ loop:
> *retval = pr->ps_pid;
> if (info != NULL) {
> info->si_pid = pr->ps_pid;
> + mtx_enter(&pr->ps_mtx);
> info->si_uid = pr->ps_ucred->cr_uid;
> + mtx_leave(&pr->ps_mtx);
> info->si_signo = SIGCHLD;
> info->si_code = CLD_TRAPPED;
> info->si_status = pr->ps_xsig;
> @@ -553,7 +557,9 @@ loop:
> *retval = pr->ps_pid;
> if (info != 0) {
> info->si_pid = pr->ps_pid;
> + mtx_enter(&pr->ps_mtx);
> info->si_uid = pr->ps_ucred->cr_uid;
> + mtx_leave(&pr->ps_mtx);
> info->si_signo = SIGCHLD;
> info->si_code = CLD_STOPPED;
> info->si_status = pr->ps_xsig;
> @@ -572,7 +578,9 @@ loop:
> *retval = pr->ps_pid;
> if (info != NULL) {
> info->si_pid = pr->ps_pid;
> + mtx_enter(&pr->ps_mtx);
> info->si_uid = pr->ps_ucred->cr_uid;
> + mtx_leave(&pr->ps_mtx);
> info->si_signo = SIGCHLD;
> info->si_code = CLD_CONTINUED;
> info->si_status = SIGCONT;
> Index: sys/kern/kern_ktrace.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_ktrace.c,v
> retrieving revision 1.108
> diff -u -p -r1.108 kern_ktrace.c
> --- sys/kern/kern_ktrace.c 14 Aug 2022 01:58:27 -0000 1.108
> +++ sys/kern/kern_ktrace.c 3 Dec 2022 17:39:00 -0000
> @@ -693,7 +693,12 @@ int
> ktrcanset(struct proc *callp, struct process *targetpr)
> {
> struct ucred *caller = callp->p_ucred;
> - struct ucred *target = targetpr->ps_ucred;
> + struct ucred *target;
> + int ret = 0;
> +
> + mtx_enter(&targetpr->ps_mtx);
> +
> + target = targetpr->ps_ucred;
>
> if ((caller->cr_uid == target->cr_ruid &&
> target->cr_ruid == target->cr_svuid &&
> @@ -702,7 +707,9 @@ ktrcanset(struct proc *callp, struct pro
> (targetpr->ps_traceflag & KTRFAC_ROOT) == 0 &&
> !ISSET(targetpr->ps_flags, PS_SUGID)) ||
> caller->cr_uid == 0)
> - return (1);
> + ret = 1;
> +
> + mtx_leave(&targetpr->ps_mtx);
>
> - return (0);
> + return (ret);
> }
> Index: sys/kern/kern_proc.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_proc.c,v
> retrieving revision 1.92
> diff -u -p -r1.92 kern_proc.c
> --- sys/kern/kern_proc.c 14 Aug 2022 01:58:27 -0000 1.92
> +++ sys/kern/kern_proc.c 3 Dec 2022 17:39:00 -0000
> @@ -583,6 +583,7 @@ db_show_all_procs(db_expr_t addr, int ha
> break;
>
> case 'n':
> + mtx_enter(&pr->ps_mtx);
> db_printf("%6d %5d %5d %d %#10x "
> "%-12.12s %-15s\n",
> p->p_tid, ppr ? ppr->ps_pid : -1,
> @@ -590,6 +591,7 @@ db_show_all_procs(db_expr_t addr, int ha
> p->p_flag | pr->ps_flags,
> (p->p_wchan && p->p_wmesg) ?
> p->p_wmesg : "", pr->ps_comm);
> + mtx_leave(&pr->ps_mtx);
> break;
>
> case 'w':
> @@ -602,6 +604,7 @@ db_show_all_procs(db_expr_t addr, int ha
> break;
>
> case 'o':
> + mtx_enter(&pr->ps_mtx);
> db_printf("%5d %5d %#10x %#10x %3d"
> "%c %-31s\n",
> pr->ps_pid, pr->ps_ucred->cr_ruid,
> @@ -609,6 +612,7 @@ db_show_all_procs(db_expr_t addr, int ha
> CPU_INFO_UNIT(p->p_cpu),
> has_kernel_lock ? 'K' : ' ',
> pr->ps_comm);
> + mtx_leave(&pr->ps_mtx);
> break;
>
> }
> Index: sys/kern/kern_prot.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_prot.c,v
> retrieving revision 1.80
> diff -u -p -r1.80 kern_prot.c
> --- sys/kern/kern_prot.c 14 Aug 2022 01:58:27 -0000 1.80
> +++ sys/kern/kern_prot.c 3 Dec 2022 17:39:00 -0000
> @@ -351,12 +351,16 @@ sys_setresuid(struct proc *p, void *v, r
> struct process *pr = p->p_p;
> struct ucred *pruc, *newcred, *uc = p->p_ucred;
> uid_t ruid, euid, suid;
> - int error;
> + int error = 0;
>
> ruid = SCARG(uap, ruid);
> euid = SCARG(uap, euid);
> suid = SCARG(uap, suid);
>
> + newcred = crget();
> +
> + mtx_enter(&pr->ps_mtx);
> +
> /*
> * make permission checks against the thread's ucred,
> * but the actual changes will be to the process's ucred
> @@ -365,7 +369,7 @@ sys_setresuid(struct proc *p, void *v, r
> if ((ruid == (uid_t)-1 || ruid == pruc->cr_ruid) &&
> (euid == (uid_t)-1 || euid == pruc->cr_uid) &&
> (suid == (uid_t)-1 || suid == pruc->cr_svuid))
> - return (0); /* no change */
> + goto error; /* no change */
>
> /*
> * Any of the real, effective, and saved uids may be changed
> @@ -376,28 +380,25 @@ sys_setresuid(struct proc *p, void *v, r
> ruid != uc->cr_uid &&
> ruid != uc->cr_svuid &&
> (error = suser(p)))
> - return (error);
> + goto error;
>
> if (euid != (uid_t)-1 &&
> euid != uc->cr_ruid &&
> euid != uc->cr_uid &&
> euid != uc->cr_svuid &&
> (error = suser(p)))
> - return (error);
> + goto error;
>
> if (suid != (uid_t)-1 &&
> suid != uc->cr_ruid &&
> suid != uc->cr_uid &&
> suid != uc->cr_svuid &&
> (error = suser(p)))
> - return (error);
> + goto error;
>
> /*
> * Copy credentials so other references do not see our changes.
> - * ps_ucred may change during the crget().
> */
> - newcred = crget();
> - pruc = pr->ps_ucred;
> crset(newcred, pruc);
>
> /*
> @@ -411,6 +412,9 @@ sys_setresuid(struct proc *p, void *v, r
> if (suid != (uid_t)-1)
> newcred->cr_svuid = suid;
> pr->ps_ucred = newcred;
> +
> + mtx_leave(&pr->ps_mtx);
> +
> atomic_setbits_int(&p->p_p->ps_flags, PS_SUGID);
>
> /* now that we can sleep, transfer proc count to new user */
> @@ -421,6 +425,12 @@ sys_setresuid(struct proc *p, void *v, r
> crfree(pruc);
>
> return (0);
> +
> +error:
> + mtx_leave(&pr->ps_mtx);
> + crfree(newcred);
> +
> + return (error);
> }
>
> int
> @@ -460,12 +470,16 @@ sys_setresgid(struct proc *p, void *v, r
> struct process *pr = p->p_p;
> struct ucred *pruc, *newcred, *uc = p->p_ucred;
> gid_t rgid, egid, sgid;
> - int error;
> + int error = 0;
>
> rgid = SCARG(uap, rgid);
> egid = SCARG(uap, egid);
> sgid = SCARG(uap, sgid);
>
> + newcred = crget();
> +
> + mtx_enter(&pr->ps_mtx);
> +
> /*
> * make permission checks against the thread's ucred,
> * but the actual changes will be to the process's ucred
> @@ -474,7 +488,7 @@ sys_setresgid(struct proc *p, void *v, r
> if ((rgid == (gid_t)-1 || rgid == pruc->cr_rgid) &&
> (egid == (gid_t)-1 || egid == pruc->cr_gid) &&
> (sgid == (gid_t)-1 || sgid == pruc->cr_svgid))
> - return (0); /* no change */
> + goto error; /* no change */
>
> /*
> * Any of the real, effective, and saved gids may be changed
> @@ -485,28 +499,25 @@ sys_setresgid(struct proc *p, void *v, r
> rgid != uc->cr_gid &&
> rgid != uc->cr_svgid &&
> (error = suser(p)))
> - return (error);
> + goto error;
>
> if (egid != (gid_t)-1 &&
> egid != uc->cr_rgid &&
> egid != uc->cr_gid &&
> egid != uc->cr_svgid &&
> (error = suser(p)))
> - return (error);
> + goto error;
>
> if (sgid != (gid_t)-1 &&
> sgid != uc->cr_rgid &&
> sgid != uc->cr_gid &&
> sgid != uc->cr_svgid &&
> (error = suser(p)))
> - return (error);
> + goto error;
>
> /*
> * Copy credentials so other references do not see our changes.
> - * ps_ucred may change during the crget().
> */
> - newcred = crget();
> - pruc = pr->ps_ucred;
> crset(newcred, pruc);
>
> /*
> @@ -520,9 +531,18 @@ sys_setresgid(struct proc *p, void *v, r
> if (sgid != (gid_t)-1)
> newcred->cr_svgid = sgid;
> pr->ps_ucred = newcred;
> +
> + mtx_leave(&pr->ps_mtx);
> +
> atomic_setbits_int(&p->p_p->ps_flags, PS_SUGID);
> crfree(pruc);
> return (0);
> +
> +error:
> + mtx_leave(&pr->ps_mtx);
> + crfree(newcred);
> +
> + return (error);
> }
>
> int
> @@ -535,11 +555,15 @@ sys_setregid(struct proc *p, void *v, re
> struct process *pr = p->p_p;
> struct ucred *pruc, *newcred, *uc = p->p_ucred;
> gid_t rgid, egid;
> - int error;
> + int error = 0;
>
> rgid = SCARG(uap, rgid);
> egid = SCARG(uap, egid);
>
> + newcred = crget();
> +
> + mtx_enter(&pr->ps_mtx);
> +
> /*
> * make permission checks against the thread's ucred,
> * but the actual changes will be to the process's ucred
> @@ -556,7 +580,7 @@ sys_setregid(struct proc *p, void *v, re
> (egid == (gid_t)-1 || egid == pruc->cr_gid) &&
> (rgid == (gid_t)-1 || (rgid == pruc->cr_rgid &&
> pruc->cr_svgid == (egid != (gid_t)-1 ? egid : pruc->cr_gid))))
> - return (0); /* no change */
> + goto error; /* no change */
>
> /*
> * Any of the real, effective, and saved gids may be changed
> @@ -567,21 +591,18 @@ sys_setregid(struct proc *p, void *v, re
> rgid != uc->cr_gid &&
> rgid != uc->cr_svgid &&
> (error = suser(p)))
> - return (error);
> + goto error;
>
> if (egid != (gid_t)-1 &&
> egid != uc->cr_rgid &&
> egid != uc->cr_gid &&
> egid != uc->cr_svgid &&
> (error = suser(p)))
> - return (error);
> + goto error;
>
> /*
> * Copy credentials so other references do not see our changes.
> - * ps_ucred may change during the crget().
> */
> - newcred = crget();
> - pruc = pr->ps_ucred;
> crset(newcred, pruc);
>
> if (rgid != (gid_t)-1)
> @@ -599,9 +620,18 @@ sys_setregid(struct proc *p, void *v, re
> pruc->cr_svgid != (egid != (gid_t)-1 ? egid : pruc->cr_gid)))
> newcred->cr_svgid = rgid;
> pr->ps_ucred = newcred;
> +
> + mtx_leave(&pr->ps_mtx);
> +
> atomic_setbits_int(&p->p_p->ps_flags, PS_SUGID);
> crfree(pruc);
> return (0);
> +
> +error:
> + mtx_leave(&pr->ps_mtx);
> + crfree(newcred);
> +
> + return (error);
> }
>
> int
> @@ -614,11 +644,15 @@ sys_setreuid(struct proc *p, void *v, re
> struct process *pr = p->p_p;
> struct ucred *pruc, *newcred, *uc = p->p_ucred;
> uid_t ruid, euid;
> - int error;
> + int error = 0;
>
> ruid = SCARG(uap, ruid);
> euid = SCARG(uap, euid);
>
> + newcred = crget();
> +
> + mtx_enter(&pr->ps_mtx);
> +
> /*
> * make permission checks against the thread's ucred,
> * but the actual changes will be to the process's ucred
> @@ -635,7 +669,7 @@ sys_setreuid(struct proc *p, void *v, re
> (euid == (uid_t)-1 || euid == pruc->cr_uid) &&
> (ruid == (uid_t)-1 || (ruid == pruc->cr_ruid &&
> pruc->cr_svuid == (euid != (uid_t)-1 ? euid : pruc->cr_uid))))
> - return (0); /* no change */
> + goto error; /* no change */
>
> /*
> * Any of the real, effective, and saved uids may be changed
> @@ -646,21 +680,18 @@ sys_setreuid(struct proc *p, void *v, re
> ruid != uc->cr_uid &&
> ruid != uc->cr_svuid &&
> (error = suser(p)))
> - return (error);
> + goto error;
>
> if (euid != (uid_t)-1 &&
> euid != uc->cr_ruid &&
> euid != uc->cr_uid &&
> euid != uc->cr_svuid &&
> (error = suser(p)))
> - return (error);
> + goto error;
>
> /*
> * Copy credentials so other references do not see our changes.
> - * ps_ucred may change during the crget().
> */
> - newcred = crget();
> - pruc = pr->ps_ucred;
> crset(newcred, pruc);
>
> if (ruid != (uid_t)-1)
> @@ -678,6 +709,9 @@ sys_setreuid(struct proc *p, void *v, re
> pruc->cr_svuid != (euid != (uid_t)-1 ? euid : pruc->cr_uid)))
> newcred->cr_svuid = ruid;
> pr->ps_ucred = newcred;
> +
> + mtx_leave(&pr->ps_mtx);
> +
> atomic_setbits_int(&p->p_p->ps_flags, PS_SUGID);
>
> /* now that we can sleep, transfer proc count to new user */
> @@ -688,6 +722,12 @@ sys_setreuid(struct proc *p, void *v, re
> crfree(pruc);
>
> return (0);
> +
> +error:
> + mtx_leave(&pr->ps_mtx);
> + crfree(newcred);
> +
> + return (error);
> }
>
> int
> @@ -699,28 +739,29 @@ sys_setuid(struct proc *p, void *v, regi
> struct process *pr = p->p_p;
> struct ucred *pruc, *newcred, *uc = p->p_ucred;
> uid_t uid;
> - int did_real, error;
> + int did_real, error = 0;
>
> uid = SCARG(uap, uid);
>
> + newcred = crget();
> +
> + mtx_enter(&pr->ps_mtx);
> +
> pruc = pr->ps_ucred;
> if (pruc->cr_uid == uid &&
> pruc->cr_ruid == uid &&
> pruc->cr_svuid == uid)
> - return (0);
> + goto error;
>
> if (uid != uc->cr_ruid &&
> uid != uc->cr_svuid &&
> uid != uc->cr_uid &&
> (error = suser(p)))
> - return (error);
> + goto error;
>
> /*
> * Copy credentials so other references do not see our changes.
> - * ps_ucred may change during the crget().
> */
> - newcred = crget();
> - pruc = pr->ps_ucred;
> crset(newcred, pruc);
>
> /*
> @@ -734,6 +775,9 @@ sys_setuid(struct proc *p, void *v, regi
> did_real = 0;
> newcred->cr_uid = uid;
> pr->ps_ucred = newcred;
> +
> + mtx_leave(&pr->ps_mtx);
> +
> atomic_setbits_int(&p->p_p->ps_flags, PS_SUGID);
>
> /*
> @@ -746,6 +790,12 @@ sys_setuid(struct proc *p, void *v, regi
> crfree(pruc);
>
> return (0);
> +
> +error:
> + mtx_leave(&pr->ps_mtx);
> + crfree(newcred);
> +
> + return (error);
> }
>
> int
> @@ -757,29 +807,40 @@ sys_seteuid(struct proc *p, void *v, reg
> struct process *pr = p->p_p;
> struct ucred *pruc, *newcred, *uc = p->p_ucred;
> uid_t euid;
> - int error;
> + int error = 0;
>
> euid = SCARG(uap, euid);
>
> + newcred = crget();
> +
> + mtx_enter(&pr->ps_mtx);
> +
> if (pr->ps_ucred->cr_uid == euid)
> - return (0);
> + goto error;
>
> if (euid != uc->cr_ruid && euid != uc->cr_svuid &&
> (error = suser(p)))
> - return (error);
> + goto error;
>
> /*
> * Copy credentials so other references do not see our changes.
> - * ps_ucred may change during the crget().
> */
> - newcred = crget();
> pruc = pr->ps_ucred;
> crset(newcred, pruc);
> newcred->cr_uid = euid;
> pr->ps_ucred = newcred;
> +
> + mtx_leave(&pr->ps_mtx);
> +
> atomic_setbits_int(&p->p_p->ps_flags, PS_SUGID);
> crfree(pruc);
> return (0);
> +
> +error:
> + mtx_leave(&pr->ps_mtx);
> + crfree(newcred);
> +
> + return (error);
> }
>
> int
> @@ -791,28 +852,29 @@ sys_setgid(struct proc *p, void *v, regi
> struct process *pr = p->p_p;
> struct ucred *pruc, *newcred, *uc = p->p_ucred;
> gid_t gid;
> - int error;
> + int error = 0;
>
> gid = SCARG(uap, gid);
>
> + newcred = crget();
> +
> + mtx_enter(&pr->ps_mtx);
> +
> pruc = pr->ps_ucred;
> if (pruc->cr_gid == gid &&
> pruc->cr_rgid == gid &&
> pruc->cr_svgid == gid)
> - return (0);
> + goto error;
>
> if (gid != uc->cr_rgid &&
> gid != uc->cr_svgid &&
> gid != uc->cr_gid &&
> (error = suser(p)))
> - return (error);
> + goto error;
>
> /*
> * Copy credentials so other references do not see our changes.
> - * ps_ucred may change during the crget().
> */
> - newcred = crget();
> - pruc = pr->ps_ucred;
> crset(newcred, pruc);
>
> if (gid == pruc->cr_gid || suser(p) == 0) {
> @@ -821,9 +883,18 @@ sys_setgid(struct proc *p, void *v, regi
> }
> newcred->cr_gid = gid;
> pr->ps_ucred = newcred;
> +
> + mtx_leave(&pr->ps_mtx);
> +
> atomic_setbits_int(&p->p_p->ps_flags, PS_SUGID);
> crfree(pruc);
> return (0);
> +
> +error:
> + mtx_leave(&pr->ps_mtx);
> + crfree(newcred);
> +
> + return (error);
> }
>
> int
> @@ -835,29 +906,40 @@ sys_setegid(struct proc *p, void *v, reg
> struct process *pr = p->p_p;
> struct ucred *pruc, *newcred, *uc = p->p_ucred;
> gid_t egid;
> - int error;
> + int error = 0;
>
> egid = SCARG(uap, egid);
>
> + newcred = crget();
> +
> + mtx_enter(&pr->ps_mtx);
> +
> if (pr->ps_ucred->cr_gid == egid)
> - return (0);
> + goto error;
>
> if (egid != uc->cr_rgid && egid != uc->cr_svgid &&
> (error = suser(p)))
> - return (error);
> + goto error;
>
> /*
> * Copy credentials so other references do not see our changes.
> - * ps_ucred may change during the crget().
> */
> - newcred = crget();
> pruc = pr->ps_ucred;
> crset(newcred, pruc);
> newcred->cr_gid = egid;
> pr->ps_ucred = newcred;
> +
> + mtx_leave(&pr->ps_mtx);
> +
> atomic_setbits_int(&p->p_p->ps_flags, PS_SUGID);
> crfree(pruc);
> return (0);
> +
> +error:
> + mtx_leave(&pr->ps_mtx);
> + crfree(newcred);
> +
> + return (error);
> }
>
> int
> @@ -881,11 +963,13 @@ sys_setgroups(struct proc *p, void *v, r
> error = copyin(SCARG(uap, gidset), groups, ngrp * sizeof(gid_t));
> if (error == 0) {
> newcred = crget();
> + mtx_enter(&pr->ps_mtx);
> pruc = pr->ps_ucred;
> crset(newcred, pruc);
> memcpy(newcred->cr_groups, groups, ngrp * sizeof(gid_t));
> newcred->cr_ngroups = ngrp;
> pr->ps_ucred = newcred;
> + mtx_leave(&pr->ps_mtx);
> atomic_setbits_int(&p->p_p->ps_flags, PS_SUGID);
> crfree(pruc);
> }
> @@ -1119,11 +1203,11 @@ dorefreshcreds(struct process *pr, struc
> {
> struct ucred *uc = p->p_ucred;
>
> - KERNEL_LOCK(); /* XXX should be PROCESS_RLOCK(pr) */
> + mtx_enter(&pr->ps_mtx);
> if (uc != pr->ps_ucred) {
> p->p_ucred = pr->ps_ucred;
> crhold(p->p_ucred);
> crfree(uc);
> }
> - KERNEL_UNLOCK();
> + mtx_leave(&pr->ps_mtx);
> }
> Index: sys/kern/kern_resource.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_resource.c,v
> retrieving revision 1.76
> diff -u -p -r1.76 kern_resource.c
> --- sys/kern/kern_resource.c 17 Nov 2022 18:53:13 -0000 1.76
> +++ sys/kern/kern_resource.c 3 Dec 2022 17:39:00 -0000
> @@ -122,10 +122,13 @@ sys_getpriority(struct proc *curp, void
> case PRIO_USER:
> if (SCARG(uap, who) == 0)
> SCARG(uap, who) = curp->p_ucred->cr_uid;
> - LIST_FOREACH(pr, &allprocess, ps_list)
> + LIST_FOREACH(pr, &allprocess, ps_list) {
> + mtx_enter(&pr->ps_mtx);
> if (pr->ps_ucred->cr_uid == SCARG(uap, who) &&
> pr->ps_nice < low)
> low = pr->ps_nice;
> + mtx_leave(&pr->ps_mtx);
> + }
> break;
>
> default:
> @@ -178,11 +181,14 @@ sys_setpriority(struct proc *curp, void
> case PRIO_USER:
> if (SCARG(uap, who) == 0)
> SCARG(uap, who) = curp->p_ucred->cr_uid;
> - LIST_FOREACH(pr, &allprocess, ps_list)
> + LIST_FOREACH(pr, &allprocess, ps_list) {
> + mtx_enter(&pr->ps_mtx);
> if (pr->ps_ucred->cr_uid == SCARG(uap, who)) {
> error = donice(curp, pr, SCARG(uap, prio));
> found = 1;
> }
> + mtx_leave(&pr->ps_mtx);
> + }
> break;
>
> default:
> @@ -200,10 +206,15 @@ donice(struct proc *curp, struct process
> struct proc *p;
> int s;
>
> + mtx_enter(&chgpr->ps_mtx);
> if (ucred->cr_uid != 0 && ucred->cr_ruid != 0 &&
> ucred->cr_uid != chgpr->ps_ucred->cr_uid &&
> - ucred->cr_ruid != chgpr->ps_ucred->cr_uid)
> + ucred->cr_ruid != chgpr->ps_ucred->cr_uid) {
> + mtx_leave(&chgpr->ps_mtx);
> return (EPERM);
> + }
> + mtx_leave(&chgpr->ps_mtx);
> +
> if (n > PRIO_MAX)
> n = PRIO_MAX;
> if (n < PRIO_MIN)
> Index: sys/kern/kern_sig.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_sig.c,v
> retrieving revision 1.301
> diff -u -p -r1.301 kern_sig.c
> --- sys/kern/kern_sig.c 16 Oct 2022 16:27:02 -0000 1.301
> +++ sys/kern/kern_sig.c 3 Dec 2022 17:39:00 -0000
> @@ -149,7 +149,8 @@ cansignal(struct proc *p, struct process
> {
> struct process *pr = p->p_p;
> struct ucred *uc = p->p_ucred;
> - struct ucred *quc = qr->ps_ucred;
> + struct ucred *quc;
> + int ret = 0;
>
> if (uc->cr_uid == 0)
> return (1); /* root can always signal */
> @@ -157,12 +158,20 @@ cansignal(struct proc *p, struct process
> if (pr == qr)
> return (1); /* process can always signal itself */
>
> + mtx_enter(&qr->ps_mtx);
> +
> + quc = qr->ps_ucred;
> +
> /* optimization: if the same creds then the tests below will pass */
> - if (uc == quc)
> - return (1);
> + if (uc == quc) {
> + ret = 1;
> + goto out;
> + }
>
> - if (signum == SIGCONT && qr->ps_session == pr->ps_session)
> - return (1); /* SIGCONT in session */
> + if (signum == SIGCONT && qr->ps_session == pr->ps_session) {
> + ret = 1; /* SIGCONT in session */
> + goto out;
> + }
>
> /*
> * Using kill(), only certain signals can be sent to setugid
> @@ -184,17 +193,20 @@ cansignal(struct proc *p, struct process
> case SIGUSR2:
> if (uc->cr_ruid == quc->cr_ruid ||
> uc->cr_uid == quc->cr_ruid)
> - return (1);
> + ret = 1;
> }
> - return (0);
> + goto out;
> }
>
> if (uc->cr_ruid == quc->cr_ruid ||
> uc->cr_ruid == quc->cr_svuid ||
> uc->cr_uid == quc->cr_ruid ||
> uc->cr_uid == quc->cr_svuid)
> - return (1);
> - return (0);
> + ret = 1;
> +
> +out:
> + mtx_leave(&qr->ps_mtx);
> + return (ret);
> }
>
> /*
> @@ -755,13 +767,17 @@ pgsigio(struct sigio_ref *sir, int sig,
> if (sigio == NULL)
> goto out;
> if (sigio->sio_pgid > 0) {
> + mtx_enter(&sigio->sio_proc->ps_mtx);
> if (CANSIGIO(sigio->sio_ucred, sigio->sio_proc))
> prsignal(sigio->sio_proc, sig);
> + mtx_leave(&sigio->sio_proc->ps_mtx);
> } else if (sigio->sio_pgid < 0) {
> LIST_FOREACH(pr, &sigio->sio_pgrp->pg_members, ps_pglist) {
> + mtx_enter(&pr->ps_mtx);
> if (CANSIGIO(sigio->sio_ucred, pr) &&
> (checkctty == 0 || (pr->ps_flags & PS_CONTROLT)))
> prsignal(pr, sig);
> + mtx_leave(&pr->ps_mtx);
> }
> }
> out:
> Index: sys/kern/kern_sysctl.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_sysctl.c,v
> retrieving revision 1.408
> diff -u -p -r1.408 kern_sysctl.c
> --- sys/kern/kern_sysctl.c 7 Nov 2022 14:25:44 -0000 1.408
> +++ sys/kern/kern_sysctl.c 3 Dec 2022 17:39:00 -0000
> @@ -1303,8 +1303,10 @@ fill_file(struct kinfo_file *kf, struct
> /* per-process information for KERN_FILE_BY[PU]ID */
> if (pr != NULL) {
> kf->p_pid = pr->ps_pid;
> + mtx_enter(&pr->ps_mtx);
> kf->p_uid = pr->ps_ucred->cr_uid;
> kf->p_gid = pr->ps_ucred->cr_gid;
> + mtx_leave(&pr->ps_mtx);
> kf->p_tid = -1;
> strlcpy(kf->p_comm, pr->ps_comm, sizeof(kf->p_comm));
> }
> @@ -1457,10 +1459,13 @@ sysctl_file(int *name, u_int namelen, ch
> */
> if (pr->ps_flags & (PS_SYSTEM | PS_EMBRYO | PS_EXITING))
> continue;
> + mtx_enter(&pr->ps_mtx);
> if (arg >= 0 && pr->ps_ucred->cr_uid != (uid_t)arg) {
> + mtx_leave(&pr->ps_mtx);
> /* not the uid we are looking for */
> continue;
> }
> + mtx_leave(&pr->ps_mtx);
> fdp = pr->ps_fd;
> if (fdp->fd_cdir)
> FILLIT(NULL, NULL, KERN_FILE_CDIR,
> fdp->fd_cdir, pr);
> @@ -1575,13 +1580,21 @@ again:
> break;
>
> case KERN_PROC_UID:
> - if (pr->ps_ucred->cr_uid != (uid_t)arg)
> + mtx_enter(&pr->ps_mtx);
> + if (pr->ps_ucred->cr_uid != (uid_t)arg) {
> + mtx_leave(&pr->ps_mtx);
> continue;
> + }
> + mtx_leave(&pr->ps_mtx);
> break;
>
> case KERN_PROC_RUID:
> - if (pr->ps_ucred->cr_ruid != (uid_t)arg)
> + mtx_enter(&pr->ps_mtx);
> + if (pr->ps_ucred->cr_ruid != (uid_t)arg) {
> + mtx_leave(&pr->ps_mtx);
> continue;
> + }
> + mtx_leave(&pr->ps_mtx);
> break;
>
> case KERN_PROC_ALL:
> @@ -1658,15 +1671,21 @@ fill_kproc(struct process *pr, struct ki
> struct tty *tp;
> struct vmspace *vm = pr->ps_vmspace;
> struct timespec booted, st, ut, utc;
> + struct ucred *prucred;
> int isthread;
>
> isthread = p != NULL;
> if (!isthread)
> p = pr->ps_mainproc; /* XXX */
>
> - FILL_KPROC(ki, strlcpy, p, pr, pr->ps_ucred, pr->ps_pgrp,
> + mtx_enter(&pr->ps_mtx);
> + prucred = crhold(pr->ps_ucred);
> + mtx_leave(&pr->ps_mtx);
> +
> + FILL_KPROC(ki, strlcpy, p, pr, prucred, pr->ps_pgrp,
> p, pr, s, vm, pr->ps_limit, pr->ps_sigacts, isthread,
> show_pointers);
> + crfree(prucred);
>
> /* stuff that's too painful to generalize into the macros */
> if (pr->ps_pptr)
> @@ -1781,11 +1800,15 @@ sysctl_proc_args(int *name, u_int namele
> if ((vpr->ps_flags & PS_INEXEC))
> return (EBUSY);
>
> + mtx_enter(&vpr->ps_mtx);
> /* Only owner or root can get env */
> if ((op == KERN_PROC_NENV || op == KERN_PROC_ENV) &&
> (vpr->ps_ucred->cr_uid != cp->p_ucred->cr_uid &&
> - (error = suser(cp)) != 0))
> + (error = suser(cp)) != 0)) {
> + mtx_leave(&vpr->ps_mtx);
> return (error);
> + }
> + mtx_leave(&vpr->ps_mtx);
>
> ps_strings = vpr->ps_strings;
> vm = vpr->ps_vmspace;
> @@ -1966,9 +1989,13 @@ sysctl_proc_cwd(int *name, u_int namelen
> return (EINVAL);
>
> /* Only owner or root can get cwd */
> + mtx_enter(&findpr->ps_mtx);
> if (findpr->ps_ucred->cr_uid != cp->p_ucred->cr_uid &&
> - (error = suser(cp)) != 0)
> + (error = suser(cp)) != 0) {
> + mtx_leave(&findpr->ps_mtx);
> return (error);
> + }
> + mtx_leave(&findpr->ps_mtx);
>
> len = *oldlenp;
> if (len > MAXPATHLEN * 4)
> Index: sys/kern/sys_process.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/sys_process.c,v
> retrieving revision 1.89
> diff -u -p -r1.89 sys_process.c
> --- sys/kern/sys_process.c 7 Dec 2021 04:19:24 -0000 1.89
> +++ sys/kern/sys_process.c 3 Dec 2022 17:39:00 -0000
> @@ -376,10 +376,14 @@ ptrace_ctrl(struct proc *p, int req, pid
> * process which revokes its special privileges using
> * setuid() from being traced. This is good security.]
> */
> + mtx_enter(&tr->ps_mtx);
> if ((tr->ps_ucred->cr_ruid != p->p_ucred->cr_ruid ||
> ISSET(tr->ps_flags, PS_SUGIDEXEC | PS_SUGID)) &&
> - (error = suser(p)) != 0)
> + (error = suser(p)) != 0) {
> + mtx_leave(&tr->ps_mtx);
> goto fail;
> + }
> + mtx_leave(&tr->ps_mtx);
>
> /*
> * (5.5) it's not a child of the tracing process.
> @@ -822,10 +826,14 @@ process_checkioperm(struct proc *p, stru
> {
> int error;
>
> + mtx_enter(&tr->ps_mtx);
> if ((tr->ps_ucred->cr_ruid != p->p_ucred->cr_ruid ||
> ISSET(tr->ps_flags, PS_SUGIDEXEC | PS_SUGID)) &&
> - (error = suser(p)) != 0)
> + (error = suser(p)) != 0) {
> + mtx_leave(&tr->ps_mtx);
> return (error);
> + }
> + mtx_leave(&tr->ps_mtx);
>
> if ((tr->ps_pid == 1) && (securelevel > -1))
> return (EPERM);
> Index: sys/kern/syscalls.master
> ===================================================================
> RCS file: /cvs/src/sys/kern/syscalls.master,v
> retrieving revision 1.237
> diff -u -p -r1.237 syscalls.master
> --- sys/kern/syscalls.master 30 Nov 2022 10:20:37 -0000 1.237
> +++ sys/kern/syscalls.master 3 Dec 2022 17:39:00 -0000
> @@ -79,7 +79,7 @@
> 21 STD { int sys_mount(const char *type, const char *path, \
> int flags, void *data); }
> 22 STD { int sys_unmount(const char *path, int flags); }
> -23 STD { int sys_setuid(uid_t uid); }
> +23 STD NOLOCK { int sys_setuid(uid_t uid); }
> 24 STD NOLOCK { uid_t sys_getuid(void); }
> 25 STD NOLOCK { uid_t sys_geteuid(void); }
> #ifdef PTRACE
> @@ -184,7 +184,7 @@
> int flags, int fd, off_t pos); }
> 79 STD NOLOCK { int sys_getgroups(int gidsetsize, \
> gid_t *gidset); }
> -80 STD { int sys_setgroups(int gidsetsize, \
> +80 STD NOLOCK { int sys_setgroups(int gidsetsize, \
> const gid_t *gidset); }
> 81 STD { int sys_getpgrp(void); }
> 82 STD { int sys_setpgid(pid_t pid, pid_t pgid); }
> @@ -260,8 +260,8 @@
> 123 STD { int sys_fchown(int fd, uid_t uid, gid_t gid); }
> 124 STD { int sys_fchmod(int fd, mode_t mode); }
> 125 OBSOL orecvfrom
> -126 STD { int sys_setreuid(uid_t ruid, uid_t euid); }
> -127 STD { int sys_setregid(gid_t rgid, gid_t egid); }
> +126 STD NOLOCK { int sys_setreuid(uid_t ruid, uid_t euid); }
> +127 STD NOLOCK { int sys_setregid(gid_t rgid, gid_t egid); }
> 128 STD { int sys_rename(const char *from, const char *to); }
> 129 OBSOL otruncate
> 130 OBSOL oftruncate
> @@ -340,9 +340,9 @@
> 180 UNIMPL
>
> ; Syscalls 181-199 are used by/reserved for BSD
> -181 STD { int sys_setgid(gid_t gid); }
> -182 STD { int sys_setegid(gid_t egid); }
> -183 STD { int sys_seteuid(uid_t euid); }
> +181 STD NOLOCK { int sys_setgid(gid_t gid); }
> +182 STD NOLOCK { int sys_setegid(gid_t egid); }
> +183 STD NOLOCK { int sys_seteuid(uid_t euid); }
> 184 OBSOL lfs_bmapv
> 185 OBSOL lfs_markv
> 186 OBSOL lfs_segclean
> @@ -484,11 +484,11 @@
> 280 UNIMPL sys_extattr_delete_fd
> 281 STD NOLOCK { int sys_getresuid(uid_t *ruid, uid_t *euid, \
> uid_t *suid); }
> -282 STD { int sys_setresuid(uid_t ruid, uid_t euid, \
> +282 STD NOLOCK { int sys_setresuid(uid_t ruid, uid_t euid, \
> uid_t suid); }
> 283 STD NOLOCK { int sys_getresgid(gid_t *rgid, gid_t *egid, \
> gid_t *sgid); }
> -284 STD { int sys_setresgid(gid_t rgid, gid_t egid, \
> +284 STD NOLOCK { int sys_setresgid(gid_t rgid, gid_t egid, \
> gid_t sgid); }
> 285 OBSOL sys_omquery
> 286 STD { void *sys_pad_mquery(void *addr, size_t len, \
> Index: sys/sys/proc.h
> ===================================================================
> RCS file: /cvs/src/sys/sys/proc.h,v
> retrieving revision 1.335
> diff -u -p -r1.335 proc.h
> --- sys/sys/proc.h 23 Nov 2022 11:00:27 -0000 1.335
> +++ sys/sys/proc.h 3 Dec 2022 17:39:00 -0000
> @@ -135,7 +135,7 @@ struct process {
> * some signal and ptrace behaviors that need to be fixed.
> */
> struct proc *ps_mainproc;
> - struct ucred *ps_ucred; /* Process owner's identity. */
> + struct ucred *ps_ucred; /* [m] Process owner's identity. */
>
> LIST_ENTRY(process) ps_list; /* List of all processes. */
> TAILQ_HEAD(,proc) ps_threads; /* [K|S] Threads in this process. */
