The branch main has been updated by olce: URL: https://cgit.FreeBSD.org/src/commit/?id=000d5b52c19ff3858a6f0cbb405d47713c4267a4
commit 000d5b52c19ff3858a6f0cbb405d47713c4267a4 Author: Olivier Certner <[email protected]> AuthorDate: 2025-11-27 09:04:50 +0000 Commit: Olivier Certner <[email protected]> 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;
