git: 000d5b52c19f - main - setcred(2): Fix a panic on too many groups from latest commit

The branch main has been updated by olce:

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

commit 000d5b52c19ff3858a6f0cbb405d47713c4267a4
Author:     Olivier Certner <olce@FreeBSD.org>
AuthorDate: 2025-11-27 09:04:50 +0000
Commit:     Olivier Certner <olce@FreeBSD.org>
CommitDate: 2025-11-27 10:05:50 +0000

    setcred(2): Fix a panic on too many groups from latest commit
    
    kern_setcred_copyin_supp_groups() is documented to always set
    'sc_supp_groups', but did not do it if there are more supplementary
    groups than 'ngroups_max'.  Also, that case was omitted from the herald
    comment.  Add it there, also including it as a case where
    'sc_supp_groups_nb' is reset to 0 as a security measure.
    
    Initially, kern_setcred_copyin_supp_groups() had the usual property that
    nothing had to be freed on it returning an error, but was then converted
    to relying on the caller to free() even on error, and this part was
    missed during the conversion.  The benefits of this unusual convention
    are that we can zero or NULLify groups-related attributes in advance,
    preventing inadvertent use of stale data (defensive security measure),
    and we can avoid some small code duplication (no need to have two same
    calls to free()).  This makes sense as kern_setcred_copyin_supp_groups()
    is meant to be a private sub-routine of user_setcred() only.  While
    here, rename kern_setcred_copyin_supp_groups() =>
    user_setcred_copyin_supp_groups().
    
    Reported by:    pho
    Fixes:          4cd93df95e69 ("setcred(): Remove an optimization for when cr_groups[0] was the egid")
    Sponsored by:   The FreeBSD Foundation
---
 sys/kern/kern_prot.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/sys/kern/kern_prot.c b/sys/kern/kern_prot.c
index 34d68927be71..b1e4b731145e 100644
--- a/sys/kern/kern_prot.c
+++ b/sys/kern/kern_prot.c
@@ -529,44 +529,54 @@ gidp_cmp(const void *p1, const void *p2)
  * 'smallgroups' must be an (uninitialized) array of length CRED_SMALLGROUPS_NB.
  * Always sets 'sc_supp_groups', either to a valid kernel-space groups array
  * (which may or may not be 'smallgroups'), or NULL if SETCREDF_SUPP_GROUPS was
- * not specified, or a buffer containing garbage on copyin() failure.  In the
- * last two cases, 'sc_supp_groups_nb' is additionally set to 0 as a security
- * measure.  'sc_supp_groups' must be freed (M_TEMP) if not equal to
- * 'smallgroups' even on failure.
+ * not specified or there are too many groups, or a buffer containing garbage on
+ * copyin() failure.  In the last two cases, 'sc_supp_groups_nb' is additionally
+ * set to 0 as a security measure.  'sc_supp_groups' must be freed (M_TEMP) if
+ * not equal to 'smallgroups' even on failure.
  */
 static int
-kern_setcred_copyin_supp_groups(struct setcred *const wcred,
+user_setcred_copyin_supp_groups(struct setcred *const wcred,
     const u_int flags, gid_t *const smallgroups)
 {
 	gid_t *groups;
 	int error;
 
 	if ((flags & SETCREDF_SUPP_GROUPS) == 0) {
-		wcred->sc_supp_groups_nb = 0;
-		wcred->sc_supp_groups = NULL;
-		return (0);
+		error = 0;
+		goto reset_groups_exit;
 	}
 
 	/*
 	 * Check the number of groups' limit right now in order to limit the
 	 * amount of bytes to copy.
 	 */
-	if (wcred->sc_supp_groups_nb > ngroups_max)
-		return (EINVAL);
+	if (wcred->sc_supp_groups_nb > ngroups_max) {
+		error = EINVAL;
+		goto reset_groups_exit;
+	}
 
 	groups = wcred->sc_supp_groups_nb <= CRED_SMALLGROUPS_NB ?
 	    smallgroups : malloc(wcred->sc_supp_groups_nb * sizeof(gid_t),
 	    M_TEMP, M_WAITOK);
-
 	error = copyin(wcred->sc_supp_groups, groups,
 	    wcred->sc_supp_groups_nb * sizeof(gid_t));
 	wcred->sc_supp_groups = groups;
+
 	if (error != 0) {
 		wcred->sc_supp_groups_nb = 0;
+		/*
+		 * 'sc_supp_groups' must be freed by caller if not
+		 * 'smallgroups'.
+		 */
 		return (error);
 	}
 
 	return (0);
+
+reset_groups_exit:
+	wcred->sc_supp_groups_nb = 0;
+	wcred->sc_supp_groups = NULL;
+	return (error);
 }
 
 int
@@ -601,7 +611,7 @@ user_setcred(struct thread *td, const u_int flags, struct setcred *const wcred)
 	 * alternative for 32-bit compatibility as 'gid_t' has the same size
 	 * everywhere.
 	 */
-	error = kern_setcred_copyin_supp_groups(wcred, flags, smallgroups);
+	error = user_setcred_copyin_supp_groups(wcred, flags, smallgroups);
 	if (error != 0)
 		goto free_groups;