git: ef739c7373d8 - main - pgrp: Prevent use after free.

[Konstantin Belousov]

The branch main has been updated by kib:

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

commit ef739c7373d8b3833979ad471b31cb9e215411fd
Author:     Konstantin Belousov <kib at FreeBSD.org>
AuthorDate: 2020-12-31 13:44:32 +0000
Commit:     Konstantin Belousov <kib at FreeBSD.org>
CommitDate: 2021-01-10 02:41:19 +0000

    pgrp: Prevent use after free.
    
    Often, we have a process locked and need to get locked process group.
    In this case, because progress group lock is before process lock,
    unlocking process allows the group to be freed.  See for instance
    tty_wait_background().
    
    Make pgrp structures allocated from nofree zone, and ensure type stability
    of the pgrp mutex.
    
    Reviewed by:    jilles
    Tested by:      pho
    MFC after:      2 weeks
    Sponsored by:   The FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D27871
---
 sys/kern/kern_proc.c | 20 +++++++++++++++-----
 sys/kern/kern_prot.c | 13 +++++--------
 sys/sys/proc.h       |  2 +-
 3 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/sys/kern/kern_proc.c b/sys/kern/kern_proc.c
index ea34631995b1..fab1df795799 100644
--- a/sys/kern/kern_proc.c
+++ b/sys/kern/kern_proc.c
@@ -98,7 +98,6 @@ __FBSDID("$FreeBSD$");
 
 SDT_PROVIDER_DEFINE(proc);
 
-MALLOC_DEFINE(M_PGRP, "pgrp", "process group header");
 MALLOC_DEFINE(M_SESSION, "session", "session header");
 static MALLOC_DEFINE(M_PROC, "proc", "Proc structures");
 MALLOC_DEFINE(M_SUBPROC, "subproc", "Proc sub-structures");
@@ -112,6 +111,7 @@ static void fill_kinfo_thread(struct thread *td, struct kinfo_proc *kp,
     int preferthread);
 static void pgadjustjobc(struct pgrp *pgrp, bool entering);
 static void pgdelete(struct pgrp *);
+static int pgrp_init(void *mem, int size, int flags);
 static int proc_ctor(void *mem, int size, void *arg, int flags);
 static void proc_dtor(void *mem, int size, void *arg);
 static int proc_init(void *mem, int size, int flags);
@@ -133,6 +133,7 @@ struct sx __exclusive_cache_line proctree_lock;
 struct mtx __exclusive_cache_line ppeers_lock;
 struct mtx __exclusive_cache_line procid_lock;
 uma_zone_t proc_zone;
+uma_zone_t pgrp_zone;
 
 /*
  * The offset of various fields in struct proc and struct thread.
@@ -196,6 +197,8 @@ procinit(void)
 	proc_zone = uma_zcreate("PROC", sched_sizeof_proc(),
 	    proc_ctor, proc_dtor, proc_init, proc_fini,
 	    UMA_ALIGN_PTR, UMA_ZONE_NOFREE);
+	pgrp_zone = uma_zcreate("PGRP", sizeof(struct pgrp), NULL, NULL,
+	    pgrp_init, NULL, UMA_ALIGN_PTR, UMA_ZONE_NOFREE);
 	uihashinit();
 }
 
@@ -299,6 +302,16 @@ proc_fini(void *mem, int size)
 #endif
 }
 
+static int
+pgrp_init(void *mem, int size, int flags)
+{
+	struct pgrp *pg;
+
+	pg = mem;
+	mtx_init(&pg->pg_mtx, "process group", NULL, MTX_DEF | MTX_DUPOK);
+	return (0);
+}
+
 /*
  * PID space management.
  *
@@ -570,8 +583,6 @@ enterpgrp(struct proc *p, pid_t pgid, struct pgrp *pgrp, struct session *sess)
 	KASSERT(!SESS_LEADER(p),
 	    ("enterpgrp: session leader attempted setpgrp"));
 
-	mtx_init(&pgrp->pg_mtx, "process group", NULL, MTX_DEF | MTX_DUPOK);
-
 	if (sess != NULL) {
 		/*
 		 * new session
@@ -798,8 +809,7 @@ pgdelete(struct pgrp *pgrp)
 	}
 
 	proc_id_clear(PROC_ID_GROUP, pgrp->pg_id);
-	mtx_destroy(&pgrp->pg_mtx);
-	free(pgrp, M_PGRP);
+	uma_zfree(pgrp_zone, pgrp);
 	sess_release(savesess);
 }
 
diff --git a/sys/kern/kern_prot.c b/sys/kern/kern_prot.c
index 529a6de4b2c8..170e9598835e 100644
--- a/sys/kern/kern_prot.c
+++ b/sys/kern/kern_prot.c
@@ -333,7 +333,7 @@ sys_setsid(struct thread *td, struct setsid_args *uap)
 	error = 0;
 	pgrp = NULL;
 
-	newpgrp = malloc(sizeof(struct pgrp), M_PGRP, M_WAITOK | M_ZERO);
+	newpgrp = uma_zalloc(pgrp_zone, M_WAITOK);
 	newsess = malloc(sizeof(struct session), M_SESSION, M_WAITOK | M_ZERO);
 
 	sx_xlock(&proctree_lock);
@@ -351,10 +351,8 @@ sys_setsid(struct thread *td, struct setsid_args *uap)
 
 	sx_xunlock(&proctree_lock);
 
-	if (newpgrp != NULL)
-		free(newpgrp, M_PGRP);
-	if (newsess != NULL)
-		free(newsess, M_SESSION);
+	uma_zfree(pgrp_zone, newpgrp);
+	free(newsess, M_SESSION);
 
 	return (error);
 }
@@ -393,7 +391,7 @@ sys_setpgid(struct thread *td, struct setpgid_args *uap)
 
 	error = 0;
 
-	newpgrp = malloc(sizeof(struct pgrp), M_PGRP, M_WAITOK | M_ZERO);
+	newpgrp = uma_zalloc(pgrp_zone, M_WAITOK);
 
 	sx_xlock(&proctree_lock);
 	if (uap->pid != 0 && uap->pid != curp->p_pid) {
@@ -456,8 +454,7 @@ done:
 	sx_xunlock(&proctree_lock);
 	KASSERT((error == 0) || (newpgrp != NULL),
 	    ("setpgid failed and newpgrp is NULL"));
-	if (newpgrp != NULL)
-		free(newpgrp, M_PGRP);
+	uma_zfree(pgrp_zone, newpgrp);
 	return (error);
 }
 
diff --git a/sys/sys/proc.h b/sys/sys/proc.h
index 33da4969c6ae..2a7f0740a0c3 100644
--- a/sys/sys/proc.h
+++ b/sys/sys/proc.h
@@ -864,7 +864,6 @@ struct proc {
 
 #ifdef MALLOC_DECLARE
 MALLOC_DECLARE(M_PARGS);
-MALLOC_DECLARE(M_PGRP);
 MALLOC_DECLARE(M_SESSION);
 MALLOC_DECLARE(M_SUBPROC);
 #endif
@@ -1022,6 +1021,7 @@ extern struct proclist allproc;		/* List of all processes. */
 extern struct proc *initproc, *pageproc; /* Process slots for init, pager. */
 
 extern struct uma_zone *proc_zone;
+extern struct uma_zone *pgrp_zone;
 
 struct	proc *pfind(pid_t);		/* Find process by id. */
 struct	proc *pfind_any(pid_t);		/* Find (zombie) process by id. */