Date: Thu, 28 Dec 2023 03:11:15 GMT From: Mark Johnston <markj@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org Subject: git: d2c1d1b2bc47 - stable/13 - kthread: Set *newtdp earlier in kthread_add1() Message-ID: <202312280311.3BS3BFhu053097@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch stable/13 has been updated by markj: URL: https://cgit.FreeBSD.org/src/commit/?id=d2c1d1b2bc47e451a4cc77ca1a2b0d1648343110 commit d2c1d1b2bc47e451a4cc77ca1a2b0d1648343110 Author: Mark Johnston <markj@FreeBSD.org> AuthorDate: 2023-12-09 15:22:06 +0000 Commit: Mark Johnston <markj@FreeBSD.org> CommitDate: 2023-12-28 03:01:26 +0000 kthread: Set *newtdp earlier in kthread_add1() syzbot reported a single boot-time crash in g_event_procbody(), a page fault when dereferencing g_event_td. g_event_td is initialized by the kproc_kthread_add() call which creates the GEOM event thread: kproc_kthread_add(g_event_procbody, NULL, &g_proc, &g_event_td, RFHIGHPID, 0, "geom", "g_event"); I believe that the caller of kproc_kthread_add() was preempted after adding the new thread to the scheduler, and before setting *newtdp, which is equal to g_event_td. Thus, since the first action of the GEOM event thread is to lock itself, it ended up dereferencing a NULL pointer. Fix the problem simply by initializing *newtdp earlier. I see no harm in that, and it matches kproc_create1(). The scheduler provides sufficient synchronization to ensure that the store is visible to the new thread, wherever it happens to run. Reported by: syzbot+5397f4d39219b85a9409@syzkaller.appspotmail.com Reviewed by: kib MFC after: 1 week Differential Revision: https://reviews.freebsd.org/D42986 (cherry picked from commit ae77041e0714627f9ec8045ca9ee2b6ea563138e) --- sys/kern/kern_kthread.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/sys/kern/kern_kthread.c b/sys/kern/kern_kthread.c index 6f7fd8b3d555..9cbc74658432 100644 --- a/sys/kern/kern_kthread.c +++ b/sys/kern/kern_kthread.c @@ -287,6 +287,13 @@ kthread_add1(void (*func)(void *), void *arg, struct proc *p, } oldtd = FIRST_THREAD_IN_PROC(p); + /* + * Set the new thread pointer before the thread starts running: *newtdp + * could be a pointer that is referenced by "func". + */ + if (newtdp != NULL) + *newtdp = newtd; + bzero(&newtd->td_startzero, __rangeof(struct thread, td_startzero, td_endzero)); bcopy(&oldtd->td_startcopy, &newtd->td_startcopy, @@ -331,8 +338,6 @@ kthread_add1(void (*func)(void *), void *arg, struct proc *p, thread_lock(newtd); sched_add(newtd, SRQ_BORING); } - if (newtdp) - *newtdp = newtd; return (0); }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202312280311.3BS3BFhu053097>