The branch main has been updated by olce: URL: https://cgit.FreeBSD.org/src/commit/?id=4cd93df95e697942adf0ff038fc8f357cbb07cf9
commit 4cd93df95e697942adf0ff038fc8f357cbb07cf9 Author: Olivier Certner <[email protected]> AuthorDate: 2025-11-14 18:22:53 +0000 Commit: Olivier Certner <[email protected]> CommitDate: 2025-11-24 20:31:06 +0000 setcred(): Remove an optimization for when cr_groups[0] was the egid Because setcred() has (always) treated the effective GID separately from the supplementary groups, when cr_groups[0] was storing the effective GID, it internally needed to build an array containing both the effective GID and the specified supplementary groups to eventually call crsetgroups_internal(). As kern_setcred() was only used to actually implement user_setcred()/sys_setcred(), which need to allocate a buffer to copy in the userland groups array into, some optimization was put in place where these would allocate an array with one more element than 'wc_supp_groups', copyin() the latter into the subarray starting at index 1 and pass the pointer to the whole array to kern_setcred() in 'preallocated_groups'. This would allow kern_setcred() not to have to allocate memory again to make room for the additional effective GID. Since commit be1f7435ef21 ("kern: start tracking cr_gid outside of cr_groups[]"), crsetgroups_internal() only takes supplementary groups, so this machinery has become obsolete. It was not removed as part of that commit, but just minimally amended to simplify the changes and lower the risks. Finally remove it. Reviewed by: kevans MFC after: 2 weeks Sponsored by: The FreeBSD Foundation Differential Revision: https://reviews.freebsd.org/D53772 --- sys/kern/kern_prot.c | 112 ++++++++++++++++++++------------------------------ sys/sys/syscallsubr.h | 2 +- 2 files changed, 46 insertions(+), 68 deletions(-) diff --git a/sys/kern/kern_prot.c b/sys/kern/kern_prot.c index 8021beed90e8..34d68927be71 100644 --- a/sys/kern/kern_prot.c +++ b/sys/kern/kern_prot.c @@ -526,44 +526,44 @@ gidp_cmp(const void *p1, const void *p2) } /* - * 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. + * '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. */ static int kern_setcred_copyin_supp_groups(struct setcred *const wcred, - const u_int flags, gid_t *const smallgroups, gid_t **const groups) + const u_int flags, gid_t *const smallgroups) { - MPASS(*groups == NULL); + gid_t *groups; + int error; - if (flags & SETCREDF_SUPP_GROUPS) { - int error; + if ((flags & SETCREDF_SUPP_GROUPS) == 0) { + wcred->sc_supp_groups_nb = 0; + wcred->sc_supp_groups = NULL; + return (0); + } - /* - * Check for the limit for number of groups right now in order - * to limit the amount of bytes to copy. - */ - if (wcred->sc_supp_groups_nb > ngroups_max) - return (EINVAL); + /* + * 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); - /* - * Since we are going to be copying the supplementary groups - * from userland, make room also for the effective GID right - * 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 * - sizeof(*groups), M_TEMP, M_WAITOK); + 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(*groups)); - if (error != 0) - return (error); - wcred->sc_supp_groups = *groups; - } else { + 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; - wcred->sc_supp_groups = NULL; + return (error); } return (0); @@ -578,7 +578,6 @@ user_setcred(struct thread *td, const u_int flags, struct setcred *const wcred) void *umac; #endif gid_t smallgroups[CRED_SMALLGROUPS_NB]; - gid_t *groups = NULL; int error; /* @@ -602,8 +601,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, - &groups); + error = kern_setcred_copyin_supp_groups(wcred, flags, smallgroups); if (error != 0) goto free_groups; @@ -616,7 +614,7 @@ user_setcred(struct thread *td, const u_int flags, struct setcred *const wcred) } #endif - error = kern_setcred(td, flags, wcred, groups); + error = kern_setcred(td, flags, wcred); #ifdef MAC if (wcred->sc_label != NULL) @@ -624,8 +622,8 @@ user_setcred(struct thread *td, const u_int flags, struct setcred *const wcred) #endif free_groups: - if (groups != smallgroups) - free(groups, M_TEMP); + if (wcred->sc_supp_groups != smallgroups) + free(wcred->sc_supp_groups, M_TEMP); return (error); } @@ -654,24 +652,18 @@ 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' 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, - struct setcred *const wcred, gid_t *preallocated_groups) + struct setcred *const wcred) { struct proc *const p = td->td_proc; - struct ucred *new_cred, *old_cred, *to_free_cred; + struct ucred *new_cred, *old_cred, *to_free_cred = NULL; struct uidinfo *uip = NULL, *ruip = NULL; #ifdef MAC void *mac_set_proc_data = NULL; bool proc_label_set = false; #endif - gid_t *groups = NULL; - gid_t smallgroups[CRED_SMALLGROUPS_NB]; int error; bool cred_set = false; @@ -683,32 +675,18 @@ kern_setcred(struct thread *const td, const u_int flags, * Part 1: We allocate and perform preparatory operations with no locks. */ - if (flags & SETCREDF_SUPP_GROUPS) { - if (wcred->sc_supp_groups_nb > ngroups_max) + if ((flags & SETCREDF_SUPP_GROUPS) != 0 && + wcred->sc_supp_groups_nb > ngroups_max) return (EINVAL); - if (preallocated_groups != NULL) { - groups = preallocated_groups; - MPASS(preallocated_groups == wcred->sc_supp_groups); - } else { - 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)); - } - } if (flags & SETCREDF_MAC_LABEL) { #ifdef MAC error = mac_set_proc_prepare(td, wcred->sc_label, &mac_set_proc_data); if (error != 0) - goto free_groups; + return (error); #else - error = ENOTSUP; - goto free_groups; + return (ENOTSUP); #endif } @@ -734,8 +712,10 @@ kern_setcred(struct thread *const td, const u_int flags, * Output the raw supplementary groups array for better * traceability. */ - AUDIT_ARG_GROUPSET(groups, wcred->sc_supp_groups_nb); - groups_normalize(&wcred->sc_supp_groups_nb, groups); + AUDIT_ARG_GROUPSET(wcred->sc_supp_groups, + wcred->sc_supp_groups_nb); + groups_normalize(&wcred->sc_supp_groups_nb, + wcred->sc_supp_groups); } /* @@ -776,7 +756,7 @@ kern_setcred(struct thread *const td, const u_int flags, */ if (flags & SETCREDF_SUPP_GROUPS) crsetgroups_internal(new_cred, wcred->sc_supp_groups_nb, - groups); + wcred->sc_supp_groups); if (flags & SETCREDF_GID) change_egid(new_cred, wcred->sc_gid); if (flags & SETCREDF_RGID) @@ -863,9 +843,7 @@ unlock_finish: uifree(uip); if (ruip != NULL) uifree(ruip); -free_groups: - if (groups != preallocated_groups && groups != smallgroups) - free(groups, M_TEMP); /* Deals with 'groups' being NULL. */ + return (error); } diff --git a/sys/sys/syscallsubr.h b/sys/sys/syscallsubr.h index 350e4073604e..4ddd2eba25c8 100644 --- a/sys/sys/syscallsubr.h +++ b/sys/sys/syscallsubr.h @@ -326,7 +326,7 @@ int kern_select(struct thread *td, int nd, fd_set *fd_in, fd_set *fd_ou, int kern_sendit(struct thread *td, int s, struct msghdr *mp, int flags, struct mbuf *control, enum uio_seg segflg); int kern_setcred(struct thread *const td, const u_int flags, - struct setcred *const wcred, gid_t *preallocated_groups); + struct setcred *const wcred); int kern_setgroups(struct thread *td, int *ngrpp, gid_t *groups); int kern_setitimer(struct thread *, u_int, struct itimerval *, struct itimerval *);
