Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 29 Mar 2023 05:04:22 GMT
From:      Mateusz Guzik <mjg@FreeBSD.org>
To:        src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org
Subject:   git: 37337709d333 - main - cred: convert the refcount from int to long
Message-ID:  <202303290504.32T54MaU074325@gitrepo.freebsd.org>

next in thread | raw e-mail | index | archive | help
The branch main has been updated by mjg:

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

commit 37337709d3334f32650ba3a7c529fa013ed5e1f2
Author:     Mateusz Guzik <mjg@FreeBSD.org>
AuthorDate: 2023-03-22 20:42:04 +0000
Commit:     Mateusz Guzik <mjg@FreeBSD.org>
CommitDate: 2023-03-29 05:02:32 +0000

    cred: convert the refcount from int to long
    
    On 64-bit platforms this sorts out worries about mitigating bugs which
    overflow the counter, all while not pessimizng anything -- most notably
    it avoids whacking per-thread operation in favor of refcount(9) API.
    
    The struct already had two instances of 4 byte padding with 256 bytes in
    size, cr_flags gets moved around to avoid growing it.
    
    32-bit platforms could also get the extended counter, but I did not do
    it as one day(tm) the mutex protecting centralized operation should be
    replaced with atomics and 64-bit ops on 32-bit platforms remain quite
    penalizing.
    
    While worries of counter overflow are addressed, the following is not
    (just like it would not be with conversion to refcount(9)):
    - counter *underflows*
    - buffer overruns from adjacent allocations
    - UAF due to stale cred pointer
    - .. and other goodies
    
    As such, while lipstick was placed, the pig should not be participating
    in any beauty pageants.
    
    Prodded by:     emaste
    Differential Revision:  https://reviews.freebsd.org/D39220
---
 sys/kern/kern_prot.c | 11 ++++++-----
 sys/sys/proc.h       |  2 +-
 sys/sys/ucred.h      |  4 ++--
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/sys/kern/kern_prot.c b/sys/kern/kern_prot.c
index 983e6ae21493..3d8f3e222b4c 100644
--- a/sys/kern/kern_prot.c
+++ b/sys/kern/kern_prot.c
@@ -1877,7 +1877,7 @@ crunuse(struct thread *td)
 	    __func__, cr->cr_users, cr));
 	cr->cr_users--;
 	if (cr->cr_users == 0) {
-		KASSERT(cr->cr_ref > 0, ("%s: ref %d not > 0 on cred %p",
+		KASSERT(cr->cr_ref > 0, ("%s: ref %ld not > 0 on cred %p",
 		    __func__, cr->cr_ref, cr));
 		crold = cr;
 	} else {
@@ -1905,7 +1905,7 @@ crunusebatch(struct ucred *cr, int users, int ref)
 		mtx_unlock(&cr->cr_mtx);
 		return;
 	}
-	KASSERT(cr->cr_ref >= 0, ("%s: ref %d not >= 0 on cred %p",
+	KASSERT(cr->cr_ref >= 0, ("%s: ref %ld not >= 0 on cred %p",
 	    __func__, cr->cr_ref, cr));
 	if (cr->cr_ref > 0) {
 		mtx_unlock(&cr->cr_mtx);
@@ -2051,7 +2051,7 @@ crfree(struct ucred *cr)
 		mtx_unlock(&cr->cr_mtx);
 		return;
 	}
-	KASSERT(cr->cr_ref >= 0, ("%s: ref %d not >= 0 on cred %p",
+	KASSERT(cr->cr_ref >= 0, ("%s: ref %ld not >= 0 on cred %p",
 	    __func__, cr->cr_ref, cr));
 	if (cr->cr_ref > 0) {
 		mtx_unlock(&cr->cr_mtx);
@@ -2066,7 +2066,7 @@ crfree_final(struct ucred *cr)
 
 	KASSERT(cr->cr_users == 0, ("%s: users %d not == 0 on cred %p",
 	    __func__, cr->cr_users, cr));
-	KASSERT(cr->cr_ref == 0, ("%s: ref %d not == 0 on cred %p",
+	KASSERT(cr->cr_ref == 0, ("%s: ref %ld not == 0 on cred %p",
 	    __func__, cr->cr_ref, cr));
 
 	/*
@@ -2104,6 +2104,7 @@ 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));
+	dest->cr_flags = src->cr_flags;
 	crsetgroups(dest, src->cr_ngroups, src->cr_groups);
 	uihold(dest->cr_uidinfo);
 	uihold(dest->cr_ruidinfo);
@@ -2210,7 +2211,7 @@ proc_unset_cred(struct proc *p)
 	mtx_lock(&cr->cr_mtx);
 	cr->cr_users--;
 	if (cr->cr_users == 0)
-		KASSERT(cr->cr_ref > 0, ("%s: ref %d not > 0 on cred %p",
+		KASSERT(cr->cr_ref > 0, ("%s: ref %ld not > 0 on cred %p",
 		    __func__, cr->cr_ref, cr));
 	mtx_unlock(&cr->cr_mtx);
 	crfree(cr);
diff --git a/sys/sys/proc.h b/sys/sys/proc.h
index c6670bdc96f8..172e716d8785 100644
--- a/sys/sys/proc.h
+++ b/sys/sys/proc.h
@@ -321,7 +321,7 @@ struct thread {
 	int		td_errno;	/* (k) Error from last syscall. */
 	size_t		td_vslock_sz;	/* (k) amount of vslock-ed space */
 	struct kcov_info *td_kcov_info;	/* (*) Kernel code coverage data */
-	u_int		td_ucredref;	/* (k) references on td_realucred */
+	long		td_ucredref;	/* (k) references on td_realucred */
 #define	td_endzero td_sigmask
 
 /* Copied during fork1(), thread_create(), or kthread_add(). */
diff --git a/sys/sys/ucred.h b/sys/sys/ucred.h
index 5ec54be70c2e..73b5fb7ab118 100644
--- a/sys/sys/ucred.h
+++ b/sys/sys/ucred.h
@@ -61,8 +61,9 @@ struct loginclass;
 #if defined(_KERNEL) || defined(_WANT_UCRED)
 struct ucred {
 	struct mtx cr_mtx;
-	int	cr_ref;			/* (c) reference count */
+	long	cr_ref;			/* (c) reference count */
 	u_int	cr_users;		/* (c) proc + thread using this cred */
+	u_int	cr_flags;		/* credential flags */
 	struct auditinfo_addr	cr_audit;	/* Audit properties. */
 #define	cr_startcopy cr_uid
 	uid_t	cr_uid;			/* effective user id */
@@ -75,7 +76,6 @@ struct ucred {
 	struct uidinfo	*cr_ruidinfo;	/* per ruid resource consumption */
 	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 */
 #define	cr_endcopy	cr_label
 	struct label	*cr_label;	/* MAC label */



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202303290504.32T54MaU074325>