From nobody Fri Nov 15 10:49:01 2024 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4XqYfj3Qj5z5cyqD; Fri, 15 Nov 2024 10:49:01 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R10" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4XqYfj2Lj6z4X27; Fri, 15 Nov 2024 10:49:01 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1731667741; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=zsrhTNYxTvtPSepQE7HqpmzLwEbFXcsdlXKthahzS3c=; b=DyWYMpO09zsrVp9Karf+LYSWuUQzpt+MoA2VZ1vLv4bzSCFQb60zYd8H3MCtU5y5uPnULh NIkklGFUWYZd5aIf3RWN83d5a/13r/pbAHjUajKjykyeW/FZovJOk2+xY6upHWHqMziAhE wIVyP41oSAjahe6Ib+yQXel2HB9LmU2uqxMeN1/oeFZGNfe1sA6lKooMJ1YHQpSl7In7Ne l7YjyYY0xMAKy5SL/ssI4p6LNP8xBhF9VzXYwCkLyQiG1yTv/vuq6jJXwCR1tf9pj2uEyX hRzVwtSx3xBuyRlgHCWz/MhL+Eb6e7iM5cKqMDq/rI/Dmvyq+9EKL9PBNLyoew== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1731667741; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=zsrhTNYxTvtPSepQE7HqpmzLwEbFXcsdlXKthahzS3c=; b=VVLPZIOKQ0qgXeoSIXRVcVVeYbhVa/RPh1zU4uyskyQmfpHWKUuu3MNHZGukd04HWwSlar UDzosNCoUqbYYBEUpyp4zPMurK8VsllSD0L7dS6UNzLgxxaD2CdU3ZeWX5PQAgekqHG1wn MUtxPUPGUUZpMNfMtMqJZpaDypLCiO8cLUUKdiV7z7LX6WhFd++WU1wdq7GeDpnVpMon8/ Ppj5gw3lwZmurlbMl54ALYlDbvijroSV3htj+ORMoCBkWFkn03jp+mrYb4MqeSBXOv1dC5 YjYyxIVbu5JFBy2wr/1cjco4dlGyy7+rif42cxTafzfPddey7wLgS867oGAgkw== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1731667741; a=rsa-sha256; cv=none; b=Zthyw6xHL89dGL6IocQXkacegIkTxJeHdgoh8XpRigvBa1tcAhGxHOrYFPiSYPPj9pRy0a suP+7VCUXWcP/1XiINy6RikHwmkj4O3uUvnu6wLVQVieT6dRAX2KresvukhfeL1aMFCdNb FmfF/r4Pd01siwUEGrekJWFYbyd4E/ap8yJ1C7RfttdCCgzfQ7X0TVAIqwD/A6i4aZVKxK gxPgNb+7TqP3NvC3m9EjdH/7rCPL7oZXH9/uF5fNRE8JNP42xZz6kT988bObMSSMXWqjkt VfLBBQMbmjj7JOYknXbeDnAgwnN7Gj5iqfr6t2Tj5DtQwEZYc5PuQz9j/fT+og== Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 4XqYfj1zDgzKVs; Fri, 15 Nov 2024 10:49:01 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.18.1/8.18.1) with ESMTP id 4AFAn1Y4018114; Fri, 15 Nov 2024 10:49:01 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.18.1/8.18.1/Submit) id 4AFAn1Eh018111; Fri, 15 Nov 2024 10:49:01 GMT (envelope-from git) Date: Fri, 15 Nov 2024 10:49:01 GMT Message-Id: <202411151049.4AFAn1Eh018111@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org From: Olivier Certner Subject: git: 847dff4bc6e0 - stable/14 - cred: crextend(): Harden, simplify List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-all@freebsd.org Sender: owner-dev-commits-src-all@FreeBSD.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: olce X-Git-Repository: src X-Git-Refname: refs/heads/stable/14 X-Git-Reftype: branch X-Git-Commit: 847dff4bc6e004fd8e8906e4dc55f39bfd0c3281 Auto-Submitted: auto-generated The branch stable/14 has been updated by olce: URL: https://cgit.FreeBSD.org/src/commit/?id=847dff4bc6e004fd8e8906e4dc55f39bfd0c3281 commit 847dff4bc6e004fd8e8906e4dc55f39bfd0c3281 Author: Olivier Certner AuthorDate: 2024-10-01 14:37:11 +0000 Commit: Olivier Certner CommitDate: 2024-11-15 10:47:42 +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 d6dd71599d86..1cc5ce46c3bf 100644 --- a/sys/kern/kern_prot.c +++ b/sys/kern/kern_prot.c @@ -888,7 +888,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); @@ -2226,6 +2227,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; dest->cr_flags = src->cr_flags; crsetgroups(dest, src->cr_ngroups, src->cr_groups); uihold(dest->cr_uidinfo); @@ -2361,45 +2369,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 8e041bb7bcbd..cd07ec3be445 100644 --- a/sys/sys/ucred.h +++ b/sys/sys/ucred.h @@ -75,6 +75,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 uidinfo *cr_ruidinfo; /* per ruid resource consumption */ struct prison *cr_prison; /* jail(2) */ struct loginclass *cr_loginclass; /* login class */ - 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 */