The branch main has been updated by kevans:
URL:
https://cgit.FreeBSD.org/src/commit/?id=be1f7435ef218b1df35aebf3b90dd65ffd8bbe51
commit be1f7435ef218b1df35aebf3b90dd65ffd8bbe51
Author: Kyle Evans <kev...@freebsd.org>
AuthorDate: 2025-07-31 04:44:11 +0000
Commit: Kyle Evans <kev...@freebsd.org>
CommitDate: 2025-07-31 04:44:11 +0000
kern: start tracking cr_gid outside of cr_groups[]
This is the (mostly) kernel side of de-conflating cr_gid and the
supplemental groups. The pre-existing behavior for getgroups() and
setgroups() is retained to keep the user <-> kernel boundary
functionally the same while we audit use of these syscalls, but we can
remove a lot of the internal special-casing just by reorganizing ucred
like this.
struct xucred has been altered because the cr_gid macro becomes
problematic if ucred has a real cr_gid member but xucred does not. Most
notably, they both also have cr_groups[] members, so the definition
means that we could easily have situations where we end up using the
first supplemental group as the egid in some places. We really can't
change the ABI of xucred, so instead we alias the first member to the
`cr_gid` name and maintain the status quo.
This also fixes the Linux setgroups(2)/getgroups(2) implementation to
more cleanly preserve the group set, now that we don't need to special
case cr_groups[0].
__FreeBSD_version bumped for the `struct ucred` ABI break.
For relnotes: downstreams and out-of-tree modules absolutely must fix
any references to cr_groups[0] in their code. These are almost
exclusively incorrect in the new world, and cr_gid should be used
instead. There is a cr_gid macro available in earlier FreeBSD versions
that can be used to avoid having version-dependant conditionals to refer
to the effective group id. Surrounding code may need adjusted if it
peels off the first element of cr_groups and uses the others as the
supplemental groups, since the supplemental groups start at cr_groups[0]
now if &cr_groups[0] != &cr_gid.
Relnotes: yes (see last paragraph)
Co-authored-by: olce
Differential Revision: https://reviews.freebsd.org/D51489
---
share/man/man9/ucred.9 | 5 +-
sys/compat/linux/linux_misc.c | 35 ++-----
sys/compat/linux/linux_uid16.c | 34 ++-----
sys/fs/nfs/nfs_commonport.c | 3 +-
sys/fs/nfsclient/nfs_clrpcops.c | 3 +-
sys/fs/nfsserver/nfs_nfsdport.c | 1 +
sys/kern/kern_prot.c | 199 +++++++++++++++++++++++-----------------
sys/rpc/authunix_prot.c | 6 +-
sys/rpc/svc_auth.c | 5 +-
sys/rpc/svc_auth_unix.c | 6 +-
sys/sys/ucred.h | 26 ++++--
sys/ufs/ufs/ufs_vnops.c | 20 +---
12 files changed, 164 insertions(+), 179 deletions(-)
diff --git a/share/man/man9/ucred.9 b/share/man/man9/ucred.9
index e9fe2e1d02fc..16de37dd8b35 100644
--- a/share/man/man9/ucred.9
+++ b/share/man/man9/ucred.9
@@ -24,7 +24,7 @@
.\" OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
.\" DAMAGE.
.\"
-.Dd January 23, 2019
+.Dd July 29, 2025
.Dt UCRED 9
.Os
.Sh NAME
@@ -119,8 +119,7 @@ It also truncates the group list to the current maximum
number of
groups.
No other mechanism should be used to modify the
.Va cr_groups
-array except for updating the primary group via assignment to
-.Va cr_groups[0] .
+array.
.Pp
The
.Fn cru2x
diff --git a/sys/compat/linux/linux_misc.c b/sys/compat/linux/linux_misc.c
index b88f1451f1a2..31460819e6ab 100644
--- a/sys/compat/linux/linux_misc.c
+++ b/sys/compat/linux/linux_misc.c
@@ -1030,47 +1030,32 @@ linux_setgroups(struct thread *td, struct
linux_setgroups_args *args)
{
struct ucred *newcred, *oldcred;
l_gid_t *linux_gidset;
- gid_t *bsd_gidset;
int ngrp, error;
struct proc *p;
ngrp = args->gidsetsize;
- if (ngrp < 0 || ngrp >= ngroups_max + 1)
+ if (ngrp < 0 || ngrp >= ngroups_max)
return (EINVAL);
linux_gidset = malloc(ngrp * sizeof(*linux_gidset), M_LINUX, M_WAITOK);
error = copyin(args->grouplist, linux_gidset, ngrp * sizeof(l_gid_t));
if (error)
goto out;
newcred = crget();
- crextend(newcred, ngrp + 1);
+ crextend(newcred, ngrp);
p = td->td_proc;
PROC_LOCK(p);
oldcred = p->p_ucred;
crcopy(newcred, oldcred);
- /*
- * cr_groups[0] holds egid. Setting the whole set from
- * the supplied set will cause egid to be changed too.
- * Keep cr_groups[0] unchanged to prevent that.
- */
-
if ((error = priv_check_cred(oldcred, PRIV_CRED_SETGROUPS)) != 0) {
PROC_UNLOCK(p);
crfree(newcred);
goto out;
}
- if (ngrp > 0) {
- newcred->cr_ngroups = ngrp + 1;
-
- bsd_gidset = newcred->cr_groups;
- ngrp--;
- while (ngrp >= 0) {
- bsd_gidset[ngrp + 1] = linux_gidset[ngrp];
- ngrp--;
- }
- } else
- newcred->cr_ngroups = 1;
+ newcred->cr_ngroups = ngrp;
+ for (int i = 0; i < ngrp; i++)
+ newcred->cr_groups[i] = linux_gidset[i];
setsugid(p);
proc_set_cred(p, newcred);
@@ -1092,13 +1077,7 @@ linux_getgroups(struct thread *td, struct
linux_getgroups_args *args)
cred = td->td_ucred;
bsd_gidset = cred->cr_groups;
- bsd_gidsetsz = cred->cr_ngroups - 1;
-
- /*
- * cr_groups[0] holds egid. Returning the whole set
- * here will cause a duplicate. Exclude cr_groups[0]
- * to prevent that.
- */
+ bsd_gidsetsz = cred->cr_ngroups;
if ((ngrp = args->gidsetsize) == 0) {
td->td_retval[0] = bsd_gidsetsz;
@@ -1112,7 +1091,7 @@ linux_getgroups(struct thread *td, struct
linux_getgroups_args *args)
linux_gidset = malloc(bsd_gidsetsz * sizeof(*linux_gidset),
M_LINUX, M_WAITOK);
while (ngrp < bsd_gidsetsz) {
- linux_gidset[ngrp] = bsd_gidset[ngrp + 1];
+ linux_gidset[ngrp] = bsd_gidset[ngrp];
ngrp++;
}
diff --git a/sys/compat/linux/linux_uid16.c b/sys/compat/linux/linux_uid16.c
index a0c9f1c39198..9fe799c0b9de 100644
--- a/sys/compat/linux/linux_uid16.c
+++ b/sys/compat/linux/linux_uid16.c
@@ -87,12 +87,11 @@ linux_setgroups16(struct thread *td, struct
linux_setgroups16_args *args)
{
struct ucred *newcred, *oldcred;
l_gid16_t *linux_gidset;
- gid_t *bsd_gidset;
int ngrp, error;
struct proc *p;
ngrp = args->gidsetsize;
- if (ngrp < 0 || ngrp >= ngroups_max + 1)
+ if (ngrp < 0 || ngrp >= ngroups_max)
return (EINVAL);
linux_gidset = malloc(ngrp * sizeof(*linux_gidset), M_LINUX, M_WAITOK);
error = copyin(args->gidset, linux_gidset, ngrp * sizeof(l_gid16_t));
@@ -106,12 +105,6 @@ linux_setgroups16(struct thread *td, struct
linux_setgroups16_args *args)
PROC_LOCK(p);
oldcred = crcopysafe(p, newcred);
- /*
- * cr_groups[0] holds egid. Setting the whole set from
- * the supplied set will cause egid to be changed too.
- * Keep cr_groups[0] unchanged to prevent that.
- */
-
if ((error = priv_check_cred(oldcred, PRIV_CRED_SETGROUPS)) != 0) {
PROC_UNLOCK(p);
crfree(newcred);
@@ -121,18 +114,9 @@ linux_setgroups16(struct thread *td, struct
linux_setgroups16_args *args)
goto out;
}
- if (ngrp > 0) {
- newcred->cr_ngroups = ngrp + 1;
-
- bsd_gidset = newcred->cr_groups;
- ngrp--;
- while (ngrp >= 0) {
- bsd_gidset[ngrp + 1] = linux_gidset[ngrp];
- ngrp--;
- }
- }
- else
- newcred->cr_ngroups = 1;
+ newcred->cr_ngroups = ngrp;
+ for (int i = 0; i < ngrp; i++)
+ newcred->cr_groups[i] = linux_gidset[i];
setsugid(td->td_proc);
proc_set_cred(p, newcred);
@@ -155,13 +139,7 @@ linux_getgroups16(struct thread *td, struct
linux_getgroups16_args *args)
cred = td->td_ucred;
bsd_gidset = cred->cr_groups;
- bsd_gidsetsz = cred->cr_ngroups - 1;
-
- /*
- * cr_groups[0] holds egid. Returning the whole set
- * here will cause a duplicate. Exclude cr_groups[0]
- * to prevent that.
- */
+ bsd_gidsetsz = cred->cr_ngroups;
if ((ngrp = args->gidsetsize) == 0) {
td->td_retval[0] = bsd_gidsetsz;
@@ -175,7 +153,7 @@ linux_getgroups16(struct thread *td, struct
linux_getgroups16_args *args)
linux_gidset = malloc(bsd_gidsetsz * sizeof(*linux_gidset),
M_LINUX, M_WAITOK);
while (ngrp < bsd_gidsetsz) {
- linux_gidset[ngrp] = bsd_gidset[ngrp + 1];
+ linux_gidset[ngrp] = bsd_gidset[ngrp];
ngrp++;
}
diff --git a/sys/fs/nfs/nfs_commonport.c b/sys/fs/nfs/nfs_commonport.c
index 222cfc03e4b3..e382b22fed74 100644
--- a/sys/fs/nfs/nfs_commonport.c
+++ b/sys/fs/nfs/nfs_commonport.c
@@ -380,8 +380,7 @@ newnfs_setroot(struct ucred *cred)
cred->cr_uid = 0;
cred->cr_gid = 0;
- /* XXXKE Fix this if cr_gid gets separated out. */
- cred->cr_ngroups = 1;
+ cred->cr_ngroups = 0;
}
/*
diff --git a/sys/fs/nfsclient/nfs_clrpcops.c b/sys/fs/nfsclient/nfs_clrpcops.c
index 36b534be531e..920fcf7b8c61 100644
--- a/sys/fs/nfsclient/nfs_clrpcops.c
+++ b/sys/fs/nfsclient/nfs_clrpcops.c
@@ -6934,8 +6934,7 @@ nfscl_dofflayoutio(vnode_t vp, struct uio *uiop, int
*iomode, int *must_commit,
tcred = NFSNEWCRED(cred);
tcred->cr_uid = flp->nfsfl_ffm[mirror].user;
tcred->cr_gid = flp->nfsfl_ffm[mirror].group;
- /* XXXKE Fix this if cr_gid gets separated out. */
- tcred->cr_ngroups = 1;
+ tcred->cr_ngroups = 0;
} else
tcred = cred;
if (rwflag == NFSV4OPEN_ACCESSREAD)
diff --git a/sys/fs/nfsserver/nfs_nfsdport.c b/sys/fs/nfsserver/nfs_nfsdport.c
index 4f0d5946d6b9..9e1a198bf34a 100644
--- a/sys/fs/nfsserver/nfs_nfsdport.c
+++ b/sys/fs/nfsserver/nfs_nfsdport.c
@@ -3463,6 +3463,7 @@ nfsd_excred(struct nfsrv_descript *nd, struct nfsexstuff
*exp,
NFSVNO_EXPORTANON(exp) ||
(nd->nd_flag & ND_AUTHNONE) != 0) {
nd->nd_cred->cr_uid = credanon->cr_uid;
+ nd->nd_cred->cr_gid = credanon->cr_gid;
/*
* 'credanon' is already a 'struct ucred' that was built
* internally with calls to crsetgroups_fallback(), so
diff --git a/sys/kern/kern_prot.c b/sys/kern/kern_prot.c
index 0f0bc056cafd..48ab7c0b520b 100644
--- a/sys/kern/kern_prot.c
+++ b/sys/kern/kern_prot.c
@@ -99,12 +99,11 @@ static inline void
groups_check_positive_len(int ngrp)
{
MPASS2(ngrp >= 0, "negative number of groups");
- MPASS2(ngrp != 0, "at least one group expected (effective GID)");
}
static inline void
groups_check_max_len(int ngrp)
{
- MPASS2(ngrp <= ngroups_max + 1, "too many groups");
+ MPASS2(ngrp <= ngroups_max, "too many supplementary groups");
}
static void groups_normalize(int *ngrp, gid_t *groups);
@@ -321,10 +320,17 @@ int
sys_getgroups(struct thread *td, struct getgroups_args *uap)
{
struct ucred *cred;
+ gid_t *ugidset;
int ngrp, error;
cred = td->td_ucred;
- ngrp = cred->cr_ngroups;
+
+ /*
+ * cr_gid has been moved out of cr_groups, but we'll continue exporting
+ * the egid as groups[0] for the time being until we audit userland for
+ * any surprises.
+ */
+ ngrp = cred->cr_ngroups + 1;
if (uap->gidsetsize == 0) {
error = 0;
@@ -333,7 +339,14 @@ sys_getgroups(struct thread *td, struct getgroups_args
*uap)
if (uap->gidsetsize < ngrp)
return (EINVAL);
- error = copyout(cred->cr_groups, uap->gidset, ngrp * sizeof(gid_t));
+ ugidset = uap->gidset;
+ error = copyout(&cred->cr_gid, ugidset, sizeof(*ugidset));
+ if (error != 0)
+ goto out;
+
+ if (ngrp > 1)
+ error = copyout(cred->cr_groups, ugidset + 1,
+ (ngrp - 1) * sizeof(*ugidset));
out:
td->td_retval[0] = ngrp;
return (error);
@@ -499,8 +512,8 @@ gidp_cmp(const void *p1, const void *p2)
}
/*
- * Final storage for groups (including the effective GID) will be returned via
- * 'groups'. '*groups' must be NULL on input, and if not equal to
'smallgroups'
+ * Final storage for supplementary groups will be returned via 'groups'.
+ * '*groups' must be NULL on input, and if not equal to 'smallgroups'
* on output, must be freed (M_TEMP) *even if* an error is returned.
*/
static int
@@ -525,15 +538,15 @@ kern_setcred_copyin_supp_groups(struct setcred *const
wcred,
* now, to avoid having to allocate and copy again the
* supplementary groups.
*/
- *groups = wcred->sc_supp_groups_nb < CRED_SMALLGROUPS_NB ?
- smallgroups : malloc((wcred->sc_supp_groups_nb + 1) *
+ *groups = wcred->sc_supp_groups_nb <= CRED_SMALLGROUPS_NB ?
+ smallgroups : malloc(wcred->sc_supp_groups_nb *
sizeof(*groups), M_TEMP, M_WAITOK);
- error = copyin(wcred->sc_supp_groups, *groups + 1,
+ error = copyin(wcred->sc_supp_groups, *groups,
wcred->sc_supp_groups_nb * sizeof(*groups));
if (error != 0)
return (error);
- wcred->sc_supp_groups = *groups + 1;
+ wcred->sc_supp_groups = *groups;
} else {
wcred->sc_supp_groups_nb = 0;
wcred->sc_supp_groups = NULL;
@@ -652,9 +665,8 @@ sys_setcred(struct thread *td, struct setcred_args *uap)
* CAUTION: This function normalizes groups in 'wcred'.
*
* If 'preallocated_groups' is non-NULL, it must be an already allocated array
- * of size 'wcred->sc_supp_groups_nb + 1', with the supplementary groups
- * starting at index 1, and 'wcred->sc_supp_groups' then must point to the
first
- * supplementary group.
+ * of size 'wcred->sc_supp_groups_nb' containing the supplementary groups, and
+ * 'wcred->sc_supp_groups' then must point to it.
*/
int
kern_setcred(struct thread *const td, const u_int flags,
@@ -685,13 +697,14 @@ kern_setcred(struct thread *const td, const u_int flags,
return (EINVAL);
if (preallocated_groups != NULL) {
groups = preallocated_groups;
- MPASS(preallocated_groups + 1 == wcred->sc_supp_groups);
+ MPASS(preallocated_groups == wcred->sc_supp_groups);
} else {
- groups = wcred->sc_supp_groups_nb < CRED_SMALLGROUPS_NB
?
- smallgroups :
- malloc((wcred->sc_supp_groups_nb + 1) *
- sizeof(*groups), M_TEMP, M_WAITOK);
- memcpy(groups + 1, wcred->sc_supp_groups,
+ if (wcred->sc_supp_groups_nb <= CRED_SMALLGROUPS_NB)
+ groups = smallgroups;
+ else
+ groups = malloc(wcred->sc_supp_groups_nb *
+ sizeof(*groups), M_TEMP, M_WAITOK);
+ memcpy(groups, wcred->sc_supp_groups,
wcred->sc_supp_groups_nb * sizeof(*groups));
}
}
@@ -726,16 +739,12 @@ kern_setcred(struct thread *const td, const u_int flags,
if (flags & SETCREDF_SVGID)
AUDIT_ARG_SGID(wcred->sc_svgid);
if (flags & SETCREDF_SUPP_GROUPS) {
- int ngrp = wcred->sc_supp_groups_nb;
-
/*
* Output the raw supplementary groups array for better
* traceability.
*/
- AUDIT_ARG_GROUPSET(groups + 1, ngrp);
- ++ngrp;
- groups_normalize(&ngrp, groups);
- wcred->sc_supp_groups_nb = ngrp - 1;
+ AUDIT_ARG_GROUPSET(groups, wcred->sc_supp_groups_nb);
+ groups_normalize(&wcred->sc_supp_groups_nb, groups);
}
/*
@@ -746,7 +755,7 @@ kern_setcred(struct thread *const td, const u_int flags,
new_cred = crget();
to_free_cred = new_cred;
if (flags & SETCREDF_SUPP_GROUPS)
- crextend(new_cred, wcred->sc_supp_groups_nb + 1);
+ crextend(new_cred, wcred->sc_supp_groups_nb);
#ifdef MAC
mac_cred_setcred_enter();
@@ -773,16 +782,11 @@ kern_setcred(struct thread *const td, const u_int flags,
/*
* Change groups.
- *
- * crsetgroups_internal() changes both the effective and supplementary
- * ones.
*/
- if (flags & SETCREDF_SUPP_GROUPS) {
- groups[0] = flags & SETCREDF_GID ? wcred->sc_gid :
- new_cred->cr_gid;
- crsetgroups_internal(new_cred, wcred->sc_supp_groups_nb + 1,
+ if (flags & SETCREDF_SUPP_GROUPS)
+ crsetgroups_internal(new_cred, wcred->sc_supp_groups_nb,
groups);
- } else if (flags & SETCREDF_GID)
+ if (flags & SETCREDF_GID)
change_egid(new_cred, wcred->sc_gid);
if (flags & SETCREDF_RGID)
change_rgid(new_cred, wcred->sc_rgid);
@@ -1206,6 +1210,7 @@ sys_setgroups(struct thread *td, struct setgroups_args
*uap)
* setgroups() differ.
*/
gidsetsize = uap->gidsetsize;
+ /* XXXKE Limit to ngroups_max when we change the userland interface. */
if (gidsetsize > ngroups_max + 1 || gidsetsize < 0)
return (EINVAL);
@@ -1233,29 +1238,49 @@ kern_setgroups(struct thread *td, int *ngrpp, gid_t
*groups)
struct proc *p = td->td_proc;
struct ucred *newcred, *oldcred;
int ngrp, error;
+ gid_t egid;
ngrp = *ngrpp;
/* Sanity check size. */
+ /* XXXKE Limit to ngroups_max when we change the userland interface. */
if (ngrp < 0 || ngrp > ngroups_max + 1)
return (EINVAL);
AUDIT_ARG_GROUPSET(groups, ngrp);
+ /*
+ * setgroups(0, NULL) is a legitimate way of clearing the groups vector
+ * on non-BSD systems (which generally do not have the egid in the
+ * groups[0]). We risk security holes when running non-BSD software if
+ * we do not do the same. So we allow and treat 0 for 'ngrp' specially
+ * below (twice).
+ */
if (ngrp != 0) {
- /* We allow and treat 0 specially below. */
- groups_normalize(ngrpp, groups);
- ngrp = *ngrpp;
+ /*
+ * To maintain userland compat for now, we use the first group
+ * as our egid and we'll use the rest as our supplemental
+ * groups.
+ */
+ egid = groups[0];
+ ngrp--;
+ groups++;
+
+ groups_normalize(&ngrp, groups);
+ *ngrpp = ngrp;
}
newcred = crget();
- if (ngrp != 0)
- crextend(newcred, ngrp);
+ crextend(newcred, ngrp);
PROC_LOCK(p);
oldcred = crcopysafe(p, newcred);
#ifdef MAC
- error = ngrp == 0 ?
- /* If 'ngrp' is 0, we'll keep just the current effective GID. */
- mac_cred_check_setgroups(oldcred, 1, oldcred->cr_groups) :
- mac_cred_check_setgroups(oldcred, ngrp, groups);
+ /*
+ * We pass NULL here explicitly if we don't have any supplementary
+ * groups mostly for the sake of normalization, but also to avoid/detect
+ * a situation where a MAC module has some assumption about the layout
+ * of `groups` matching historical behavior.
+ */
+ error = mac_cred_check_setgroups(oldcred, ngrp,
+ ngrp == 0 ? NULL : groups);
if (error)
goto fail;
#endif
@@ -1264,16 +1289,14 @@ kern_setgroups(struct thread *td, int *ngrpp, gid_t
*groups)
if (error)
goto fail;
- if (ngrp == 0) {
- /*
- * setgroups(0, NULL) is a legitimate way of clearing the
- * groups vector on non-BSD systems (which generally do not
- * have the egid in the groups[0]). We risk security holes
- * when running non-BSD software if we do not do the same.
- */
- newcred->cr_ngroups = 1;
- } else
- crsetgroups_internal(newcred, ngrp, groups);
+ /*
+ * If some groups were passed, the first one is currently the desired
+ * egid. This code is to be removed (along with some commented block
+ * above) when setgroups() is changed to take only supplementary groups.
+ */
+ if (ngrp != 0)
+ newcred->cr_gid = egid;
+ crsetgroups_internal(newcred, ngrp, groups);
setsugid(p);
proc_set_cred(p, newcred);
@@ -1693,11 +1716,11 @@ groups_check_normalized(int ngrp, const gid_t *groups)
groups_check_positive_len(ngrp);
groups_check_max_len(ngrp);
- if (ngrp == 1)
+ if (ngrp <= 1)
return;
- prev_g = groups[1];
- for (int i = 2; i < ngrp; ++i) {
+ prev_g = groups[0];
+ for (int i = 1; i < ngrp; ++i) {
const gid_t g = groups[i];
if (prev_g >= g)
@@ -1723,7 +1746,7 @@ group_is_supplementary(const gid_t gid, const struct
ucred *const cred)
* Perform a binary search of the supplementary groups. This is
* possible because we sort the groups in crsetgroups().
*/
- return (bsearch(&gid, cred->cr_groups + 1, cred->cr_ngroups - 1,
+ return (bsearch(&gid, cred->cr_groups, cred->cr_ngroups,
sizeof(gid), gidp_cmp) != NULL);
}
@@ -2588,11 +2611,6 @@ void
crcopy(struct ucred *dest, struct ucred *src)
{
- /*
- * Ideally, 'cr_ngroups' should be moved out of 'struct ucred''s bcopied
- * area, but this would break the ABI, so is deferred until there is
- * a compelling need to change it.
- */
bcopy(&src->cr_startcopy, &dest->cr_startcopy,
(unsigned)((caddr_t)&src->cr_endcopy -
(caddr_t)&src->cr_startcopy));
@@ -2634,11 +2652,17 @@ cru2x(struct ucred *cr, struct xucred *xcr)
bzero(xcr, sizeof(*xcr));
xcr->cr_version = XUCRED_VERSION;
xcr->cr_uid = cr->cr_uid;
+ xcr->cr_gid = cr->cr_gid;
- ngroups = MIN(cr->cr_ngroups, XU_NGROUPS);
+ /*
+ * We use a union to alias cr_gid to cr_groups[0] in the xucred, so
+ * this is kind of ugly; cr_ngroups still includes the egid for our
+ * purposes to avoid bumping the xucred version.
+ */
+ ngroups = MIN(cr->cr_ngroups + 1, nitems(xcr->cr_groups));
xcr->cr_ngroups = ngroups;
- bcopy(cr->cr_groups, xcr->cr_groups,
- ngroups * sizeof(*cr->cr_groups));
+ bcopy(cr->cr_groups, xcr->cr_sgroups,
+ (ngroups - 1) * sizeof(*cr->cr_groups));
}
void
@@ -2809,12 +2833,8 @@ crextend(struct ucred *cr, int n)
/*
* Normalizes a set of groups to be applied to a 'struct ucred'.
*
- * The set of groups is an array that must comprise the effective GID as its
- * first element (so its length cannot be 0).
- *
- * Normalization ensures that elements after the first, which stand for the
- * supplementary groups, are sorted in ascending order and do not contain
- * duplicates.
+ * Normalization ensures that the supplementary groups are sorted in ascending
+ * order and do not contain duplicates.
*/
static void
groups_normalize(int *ngrp, gid_t *groups)
@@ -2825,15 +2845,15 @@ groups_normalize(int *ngrp, gid_t *groups)
groups_check_positive_len(*ngrp);
groups_check_max_len(*ngrp);
- if (*ngrp == 1)
+ if (*ngrp <= 1)
return;
- qsort(groups + 1, *ngrp - 1, sizeof(*groups), gidp_cmp);
+ qsort(groups, *ngrp, sizeof(*groups), gidp_cmp);
/* Remove duplicates. */
- prev_g = groups[1];
- ins_idx = 2;
- for (int i = 2; i < *ngrp; ++i) {
+ prev_g = groups[0];
+ ins_idx = 1;
+ for (int i = ins_idx; i < *ngrp; ++i) {
const gid_t g = groups[i];
if (g != prev_g) {
@@ -2876,7 +2896,7 @@ crsetgroups_internal(struct ucred *cr, int ngrp, const
gid_t *groups)
* Copy groups in to a credential after expanding it if required.
*
* May sleep in order to allocate memory (except if, e.g., crextend() was
called
- * before with 'ngrp' or greater). Truncates the list to (ngroups_max + 1) if
+ * before with 'ngrp' or greater). Truncates the list to ngroups_max if
* it is too large. Array 'groups' doesn't need to be sorted. 'ngrp' must be
* strictly positive.
*/
@@ -2884,8 +2904,8 @@ void
crsetgroups(struct ucred *cr, int ngrp, const gid_t *groups)
{
- if (ngrp > ngroups_max + 1)
- ngrp = ngroups_max + 1;
+ if (ngrp > ngroups_max)
+ ngrp = ngroups_max;
/*
* crextend() asserts that groups are not set, as it may allocate a new
* backing storage without copying the content of the old one. Since we
@@ -2893,6 +2913,9 @@ crsetgroups(struct ucred *cr, int ngrp, const gid_t
*groups)
* consider the old ones thrown away.
*/
cr->cr_ngroups = 0;
+ if (ngrp == 0)
+ return;
+
crextend(cr, ngrp);
crsetgroups_internal(cr, ngrp, groups);
groups_normalize(&cr->cr_ngroups, cr->cr_groups);
@@ -2902,18 +2925,22 @@ crsetgroups(struct ucred *cr, int ngrp, const gid_t
*groups)
* Same as crsetgroups() but accepts an empty groups array.
*
* This function ensures that an effective GID is always present in
credentials.
- * An empty array is treated as a one-size one holding the passed effective GID
- * fallback.
+ * An empty array will only set the effective GID to the fallback, while a
+ * non-empty array will peel off groups[0] to set as the effective GID and use
+ * the remainder, if any, as supplementary groups.
*/
void
crsetgroups_fallback(struct ucred *cr, int ngrp, const gid_t *groups,
const gid_t fallback)
{
- if (ngrp == 0)
- /* Shortcut. */
- crsetgroups_internal(cr, 1, &fallback);
- else
- crsetgroups(cr, ngrp, groups);
+ if (ngrp == 0) {
+ cr->cr_gid = fallback;
+ cr->cr_ngroups = 0;
+ return;
+ }
+
+ crsetgroups(cr, ngrp - 1, groups + 1);
+ cr->cr_gid = groups[0];
}
/*
diff --git a/sys/rpc/authunix_prot.c b/sys/rpc/authunix_prot.c
index 7b531946488a..b107d5541c50 100644
--- a/sys/rpc/authunix_prot.c
+++ b/sys/rpc/authunix_prot.c
@@ -96,8 +96,12 @@ xdr_authunix_parms(XDR *xdrs, uint32_t *time, struct xucred
*cred)
if (!xdr_uint32_t(xdrs, &cred->cr_gid))
return (FALSE);
- /* XXXKE Fix this is cr_gid gets separated out. */
if (xdrs->x_op == XDR_ENCODE) {
+ /*
+ * Note that this is a `struct xucred`, which maintains its
+ * historical layout of preserving the egid in cr_ngroups and
+ * cr_groups[0] == egid.
+ */
ngroups = cred->cr_ngroups - 1;
if (ngroups > NGRPS)
ngroups = NGRPS;
diff --git a/sys/rpc/svc_auth.c b/sys/rpc/svc_auth.c
index 92f1ee0f2844..838fa9ed313a 100644
--- a/sys/rpc/svc_auth.c
+++ b/sys/rpc/svc_auth.c
@@ -39,6 +39,7 @@
*/
#include <sys/param.h>
+#include <sys/conf.h>
#include <sys/lock.h>
#include <sys/mutex.h>
#include <sys/proc.h>
@@ -191,7 +192,7 @@ svc_getcred(struct svc_req *rqst, struct ucred **crp, int
*flavorp)
return (FALSE);
cr = crget();
cr->cr_uid = cr->cr_ruid = cr->cr_svuid = xprt->xp_uid;
- crsetgroups(cr, xprt->xp_ngrps, xprt->xp_gidp);
+ crsetgroups_fallback(cr, xprt->xp_ngrps, xprt->xp_gidp,
GID_NOGROUP);
cr->cr_rgid = cr->cr_svgid = cr->cr_gid;
cr->cr_prison = curthread->td_ucred->cr_prison;
prison_hold(cr->cr_prison);
@@ -206,7 +207,7 @@ svc_getcred(struct svc_req *rqst, struct ucred **crp, int
*flavorp)
return (FALSE);
cr = crget();
cr->cr_uid = cr->cr_ruid = cr->cr_svuid = xcr->cr_uid;
- crsetgroups(cr, xcr->cr_ngroups, xcr->cr_groups);
+ crsetgroups_fallback(cr, xcr->cr_ngroups, xcr->cr_groups,
GID_NOGROUP);
cr->cr_rgid = cr->cr_svgid = cr->cr_gid;
cr->cr_prison = curthread->td_ucred->cr_prison;
prison_hold(cr->cr_prison);
diff --git a/sys/rpc/svc_auth_unix.c b/sys/rpc/svc_auth_unix.c
index b10ef33be704..963f4f272964 100644
--- a/sys/rpc/svc_auth_unix.c
+++ b/sys/rpc/svc_auth_unix.c
@@ -89,8 +89,12 @@ _svcauth_unix(struct svc_req *rqst, struct rpc_msg *msg)
stat = AUTH_BADCRED;
goto done;
}
- /* XXXKE Fix this if cr_gid gets separated out. */
for (i = 0; i < gid_len; i++) {
+ /*
+ * Note that this is a `struct xucred`, which maintains
+ * its historical layout of preserving the egid in
+ * cr_ngroups and cr_groups[0] == egid.
+ */
if (i + 1 < XU_NGROUPS)
xcr->cr_groups[i + 1] = IXDR_GET_INT32(buf);
else
diff --git a/sys/sys/ucred.h b/sys/sys/ucred.h
index ddd8f3ddb63d..4831d8cb6e1b 100644
--- a/sys/sys/ucred.h
+++ b/sys/sys/ucred.h
@@ -76,15 +76,12 @@ struct ucred {
u_int cr_users; /* (c) proc + thread using this cred */
u_int cr_flags; /* credential flags */
struct auditinfo_addr cr_audit; /* Audit properties. */
+ int cr_ngroups; /* number of supplementary groups */
#define cr_startcopy cr_uid
uid_t cr_uid; /* effective user id */
uid_t cr_ruid; /* real user id */
uid_t cr_svuid; /* saved user id */
- /*
- * XXXOC: On the next ABI change, please move 'cr_ngroups' out of the
- * copied area (crcopy() already copes with this change).
- */
- int cr_ngroups; /* number of groups */
+ gid_t cr_gid; /* effective group id */
gid_t cr_rgid; /* real group id */
gid_t cr_svgid; /* saved group id */
struct uidinfo *cr_uidinfo; /* per euid resource consumption */
@@ -111,8 +108,20 @@ struct ucred {
struct xucred {
u_int cr_version; /* structure layout version */
uid_t cr_uid; /* effective user id */
- short cr_ngroups; /* number of groups */
- gid_t cr_groups[XU_NGROUPS]; /* groups */
+ short cr_ngroups; /* number of groups (incl. cr_gid). */
+ union {
+ /*
+ * Special little hack to avoid needing a cr_gid macro, which
+ * would cause problems if one were to use it with struct ucred
+ * which also has a cr_groups member.
+ */
+ struct {
+ gid_t cr_gid; /* effective group id */
+ gid_t cr_sgroups[XU_NGROUPS - 1];
+ };
+
+ gid_t cr_groups[XU_NGROUPS]; /* groups */
+ };
union {
void *_cr_unused1; /* compatibility with old ucred */
pid_t cr_pid;
@@ -120,9 +129,6 @@ struct xucred {
};
#define XUCRED_VERSION 0
-/* This can be used for both ucred and xucred structures. */
-#define cr_gid cr_groups[0]
-
struct mac;
/*
* Structure to pass as an argument to the setcred() system call.
diff --git a/sys/ufs/ufs/ufs_vnops.c b/sys/ufs/ufs/ufs_vnops.c
index ee2188baf28d..ffc993aef9fc 100644
--- a/sys/ufs/ufs/ufs_vnops.c
+++ b/sys/ufs/ufs/ufs_vnops.c
@@ -2038,7 +2038,6 @@ ufs_mkdir(
{
#ifdef QUOTA
struct ucred ucred, *ucp;
- gid_t ucred_group;
ucp = cnp->cn_cred;
#endif
/*
@@ -2065,13 +2064,8 @@ ufs_mkdir(
*/
ucred.cr_ref = 1;
ucred.cr_uid = ip->i_uid;
-
- /*
- * XXXKE Fix this is cr_gid gets separated out
- */
- ucred.cr_ngroups = 1;
- ucred.cr_groups = &ucred_group;
- ucred.cr_gid = ucred_group = dp->i_gid;
+ ucred.cr_gid = dp->i_gid;
+ ucred.cr_ngroups = 0;
ucp = &ucred;
}
#endif
@@ -2802,7 +2796,6 @@ ufs_makeinode(int mode, struct vnode *dvp, struct vnode
**vpp,
{
#ifdef QUOTA
struct ucred ucred, *ucp;
- gid_t ucred_group;
ucp = cnp->cn_cred;
#endif
/*
@@ -2828,13 +2821,8 @@ ufs_makeinode(int mode, struct vnode *dvp, struct vnode
**vpp,
*/
ucred.cr_ref = 1;
ucred.cr_uid = ip->i_uid;
-
- /*
- * XXXKE Fix this is cr_gid gets separated out
- */
- ucred.cr_ngroups = 1;
- ucred.cr_groups = &ucred_group;
- ucred.cr_gid = ucred_group = pdir->i_gid;
+ ucred.cr_gid = pdir->i_gid;
+ ucred.cr_ngroups = 0;
ucp = &ucred;
#endif
} else {
