git: a10de3d1167a - stable/13 - cred: crextend(): Harden, simplify

The branch stable/13 has been updated by olce:

URL: https://cgit.FreeBSD.org/src/commit/?id=a10de3d1167a5e5039dedfa3848afd3158faeb93

commit a10de3d1167a5e5039dedfa3848afd3158faeb93
Author:     Olivier Certner <olce@FreeBSD.org>
AuthorDate: 2024-10-01 14:37:11 +0000
Commit:     Olivier Certner <olce@FreeBSD.org>
CommitDate: 2024-11-15 12:59:09 +0000

    cred: crextend(): Harden, simplify
    
    Harden by adding more assertions, and a plain panic in case of an
    unrepresentable size for the groups array (this can never happen after
    the change of the 'kern.ngroups' computation to impose some not too high
    maximum value a few commits ago).  Fix an impact in kern_setgroups().
    
    Simplify by removing the iterative process whose purpose is actually to
    determine the closest power of two that is greater than the wanted
    number of bytes.  Using the proper target quantity (number of bytes)
    incidentally helps with eliminating divisions (and the reliance on
    sizeof(gid_t) being a power of two).
    
    Reviewed by:    mhorne (older version)
    Approved by:    markj (mentor)
    MFC after:      3 days
    Differential Revision:  https://reviews.freebsd.org/D46915
    
    (cherry picked from commit ea26c0e797525634dd25dede233ff2ded053cf2d)
    
    Approved by:    markj (mentor)
---
 sys/kern/kern_prot.c | 55 +++++++++++++++++++++++++++++++---------------------
 sys/sys/ucred.h      |  6 +++++-
 2 files changed, 38 insertions(+), 23 deletions(-)

diff --git a/sys/kern/kern_prot.c b/sys/kern/kern_prot.c
index ede79f7d6bd2..ae3667146519 100644
--- a/sys/kern/kern_prot.c
+++ b/sys/kern/kern_prot.c
@@ -884,7 +884,8 @@ kern_setgroups(struct thread *td, int *ngrpp, gid_t *groups)
 		ngrp = *ngrpp;
 	}
 	newcred = crget();
-	crextend(newcred, ngrp);
+	if (ngrp != 0)
+		crextend(newcred, ngrp);
 	PROC_LOCK(p);
 	oldcred = crcopysafe(p, newcred);
 
@@ -2224,6 +2225,13 @@ crcopy(struct ucred *dest, struct ucred *src)
 	bcopy(&src->cr_startcopy, &dest->cr_startcopy,
 	    (unsigned)((caddr_t)&src->cr_endcopy -
 		(caddr_t)&src->cr_startcopy));
+	/*
+	 * Avoids an assertion in crsetgroups() -> crextend().  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 real
+	 * need to change the ABI.
+	 */
+	dest->cr_ngroups = 0;
 	crsetgroups(dest, src->cr_ngroups, src->cr_groups);
 	uihold(dest->cr_uidinfo);
 	uihold(dest->cr_ruidinfo);
@@ -2358,45 +2366,48 @@ crcopysafe(struct proc *p, struct ucred *cr)
 }
 
 /*
- * Extend the passed in credential to hold n items.
+ * Extend the passed-in credentials to hold n groups.
+ *
+ * Must not be called after groups have been set.
  */
 void
 crextend(struct ucred *cr, int n)
 {
-	int cnt;
+	size_t nbytes;
 
 	MPASS2(cr->cr_ref == 1, "'cr_ref' must be 1 (referenced, unshared)");
+	MPASS2(cr->cr_ngroups == 0, "groups on 'cr' already set!");
+	groups_check_positive_len(n);
+	groups_check_max_len(n);
 
-	/* Truncate? */
 	if (n <= cr->cr_agroups)
 		return;
 
+	nbytes = n * sizeof(gid_t);
+	if (nbytes < n)
+		panic("Too many groups (memory size overflow)! "
+		    "Computation of 'kern.ngroups' should have prevented this, "
+		    "please fix it. In the meantime, reduce 'kern.ngroups'.");
+
 	/*
-	 * We extend by 2 each time since we're using a power of two
-	 * allocator until we need enough groups to fill a page.
-	 * Once we're allocating multiple pages, only allocate as many
-	 * as we actually need.  The case of processes needing a
-	 * non-power of two number of pages seems more likely than
-	 * a real world process that adds thousands of groups one at a
-	 * time.
+	 * We allocate a power of 2 larger than 'nbytes', except when that
+	 * exceeds PAGE_SIZE, in which case we allocate the right multiple of
+	 * pages.  We assume PAGE_SIZE is a power of 2 (the call to roundup2()
+	 * below) but do not need to for sizeof(gid_t).
 	 */
-	if ( n < PAGE_SIZE / sizeof(gid_t) ) {
-		if (cr->cr_agroups == 0)
-			cnt = MAX(1, MINALLOCSIZE / sizeof(gid_t));
-		else
-			cnt = cr->cr_agroups * 2;
-
-		while (cnt < n)
-			cnt *= 2;
+	if (nbytes < PAGE_SIZE) {
+		if (!powerof2(nbytes))
+			/* fls*() return a bit index starting at 1. */
+			nbytes = 1 << flsl(nbytes);
 	} else
-		cnt = roundup2(n, PAGE_SIZE / sizeof(gid_t));
+		nbytes = roundup2(nbytes, PAGE_SIZE);
 
 	/* Free the old array. */
 	if (cr->cr_groups != cr->cr_smallgroups)
 		free(cr->cr_groups, M_CRED);
 
-	cr->cr_groups = malloc(cnt * sizeof(gid_t), M_CRED, M_WAITOK | M_ZERO);
-	cr->cr_agroups = cnt;
+	cr->cr_groups = malloc(nbytes, M_CRED, M_WAITOK | M_ZERO);
+	cr->cr_agroups = nbytes / sizeof(gid_t);
 }
 
 /*
diff --git a/sys/sys/ucred.h b/sys/sys/ucred.h
index 8152f962f3cb..eb5f0ad839e5 100644
--- a/sys/sys/ucred.h
+++ b/sys/sys/ucred.h
@@ -74,6 +74,10 @@ struct ucred {
 	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_rgid;		/* real group id */
 	gid_t	cr_svgid;		/* saved group id */
@@ -82,7 +86,7 @@ struct ucred {
 	struct prison	*cr_prison;	/* jail(2) */
 	struct loginclass	*cr_loginclass; /* login class */
 	u_int		cr_flags;	/* credential flags */
-	void 		*cr_pspare2[2];	/* general use 2 */
+	void		*cr_pspare2[2];	/* general use 2 */
 #define	cr_endcopy	cr_label
 	struct label	*cr_label;	/* MAC label */
 	gid_t	*cr_groups;		/* groups */