Date: Mon, 29 Jan 2024 14:28:08 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: 11cf350c3893 - stable/14 - condvar: Fix a user-after-free in _cv_wait() when ktrace is enabled Message-ID: <202401291428.40TES8Oc086714@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch stable/14 has been updated by markj: URL: https://cgit.FreeBSD.org/src/commit/?id=11cf350c3893d7077dbb28b9177585e9c903007c commit 11cf350c3893d7077dbb28b9177585e9c903007c Author: Mark Johnston <markj@FreeBSD.org> AuthorDate: 2024-01-15 17:29:02 +0000 Commit: Mark Johnston <markj@FreeBSD.org> CommitDate: 2024-01-29 14:25:52 +0000 condvar: Fix a user-after-free in _cv_wait() when ktrace is enabled When a thread wakes up after sleeping on a CV, it must not dereference the CV structure, as it may already have been freed. At least ZFS relies on this invariant, see commit c636f94bd2ff15be5b904939872b4bce31456c18 for example. Thus, when logging context-switch events, copy the wmesg into a stack buffer while it is still safe to do so, and log that after waking up. While here, move the initial ktrcsw() call later, after assertions and the SCHEDULER_STOPPED_TD() condition are checked. Reported by: syzkaller Reviewed by: kib MFC after: 2 weeks Differential Revision: https://reviews.freebsd.org/D43450 (cherry picked from commit a5ef95cd228e43bcc459a5c8a9911e57888ba5fd) --- sys/kern/kern_condvar.c | 109 +++++++++++++++++++++++++++++++++--------------- 1 file changed, 75 insertions(+), 34 deletions(-) diff --git a/sys/kern/kern_condvar.c b/sys/kern/kern_condvar.c index 6b5f2933e041..2731f581a29f 100644 --- a/sys/kern/kern_condvar.c +++ b/sys/kern/kern_condvar.c @@ -43,8 +43,9 @@ #include <sys/sleepqueue.h> #include <sys/resourcevar.h> #ifdef KTRACE -#include <sys/uio.h> #include <sys/ktrace.h> +#include <sys/uio.h> +#include <sys/user.h> #endif /* @@ -107,24 +108,32 @@ void _cv_wait(struct cv *cvp, struct lock_object *lock) { WITNESS_SAVE_DECL(lock_witness); +#ifdef KTRACE + char wmesg[WMESGLEN + 1]; +#endif struct lock_class *class; struct thread *td; uintptr_t lock_state; td = curthread; - lock_state = 0; -#ifdef KTRACE - if (KTRPOINT(td, KTR_CSW)) - ktrcsw(1, 0, cv_wmesg(cvp)); -#endif CV_ASSERT(cvp, lock, td); WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK, lock, "Waiting on \"%s\"", cvp->cv_description); - class = LOCK_CLASS(lock); if (SCHEDULER_STOPPED_TD(td)) return; +#ifdef KTRACE + if (KTRPOINT(td, KTR_CSW)) { + strlcpy(wmesg, cv_wmesg(cvp), sizeof(wmesg)); + ktrcsw(1, 0, wmesg); + } else { + wmesg[0] = '\0'; + } +#endif + + class = LOCK_CLASS(lock); + lock_state = 0; sleepq_lock(cvp); CV_WAITERS_INC(cvp); @@ -145,7 +154,7 @@ _cv_wait(struct cv *cvp, struct lock_object *lock) #ifdef KTRACE if (KTRPOINT(td, KTR_CSW)) - ktrcsw(0, 0, cv_wmesg(cvp)); + ktrcsw(0, 0, wmesg); #endif PICKUP_GIANT(); if (lock != &Giant.lock_object) { @@ -161,14 +170,13 @@ _cv_wait(struct cv *cvp, struct lock_object *lock) void _cv_wait_unlock(struct cv *cvp, struct lock_object *lock) { +#ifdef KTRACE + char wmesg[WMESGLEN + 1]; +#endif struct lock_class *class; struct thread *td; td = curthread; -#ifdef KTRACE - if (KTRPOINT(td, KTR_CSW)) - ktrcsw(1, 0, cv_wmesg(cvp)); -#endif CV_ASSERT(cvp, lock, td); WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK, lock, "Waiting on \"%s\"", cvp->cv_description); @@ -181,6 +189,15 @@ _cv_wait_unlock(struct cv *cvp, struct lock_object *lock) return; } +#ifdef KTRACE + if (KTRPOINT(td, KTR_CSW)) { + strlcpy(wmesg, cv_wmesg(cvp), sizeof(wmesg)); + ktrcsw(1, 0, wmesg); + } else { + wmesg[0] = '\0'; + } +#endif + sleepq_lock(cvp); CV_WAITERS_INC(cvp); @@ -196,7 +213,7 @@ _cv_wait_unlock(struct cv *cvp, struct lock_object *lock) #ifdef KTRACE if (KTRPOINT(td, KTR_CSW)) - ktrcsw(0, 0, cv_wmesg(cvp)); + ktrcsw(0, 0, wmesg); #endif PICKUP_GIANT(); } @@ -211,25 +228,33 @@ int _cv_wait_sig(struct cv *cvp, struct lock_object *lock) { WITNESS_SAVE_DECL(lock_witness); +#ifdef KTRACE + char wmesg[WMESGLEN + 1]; +#endif struct lock_class *class; struct thread *td; uintptr_t lock_state; int rval; td = curthread; - lock_state = 0; -#ifdef KTRACE - if (KTRPOINT(td, KTR_CSW)) - ktrcsw(1, 0, cv_wmesg(cvp)); -#endif CV_ASSERT(cvp, lock, td); WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK, lock, "Waiting on \"%s\"", cvp->cv_description); - class = LOCK_CLASS(lock); if (SCHEDULER_STOPPED_TD(td)) return (0); +#ifdef KTRACE + if (KTRPOINT(td, KTR_CSW)) { + strlcpy(wmesg, cv_wmesg(cvp), sizeof(wmesg)); + ktrcsw(1, 0, wmesg); + } else { + wmesg[0] = '\0'; + } +#endif + + class = LOCK_CLASS(lock); + lock_state = 0; sleepq_lock(cvp); CV_WAITERS_INC(cvp); @@ -251,7 +276,7 @@ _cv_wait_sig(struct cv *cvp, struct lock_object *lock) #ifdef KTRACE if (KTRPOINT(td, KTR_CSW)) - ktrcsw(0, 0, cv_wmesg(cvp)); + ktrcsw(0, 0, wmesg); #endif PICKUP_GIANT(); if (lock != &Giant.lock_object) { @@ -272,24 +297,32 @@ _cv_timedwait_sbt(struct cv *cvp, struct lock_object *lock, sbintime_t sbt, sbintime_t pr, int flags) { WITNESS_SAVE_DECL(lock_witness); +#ifdef KTRACE + char wmesg[WMESGLEN + 1]; +#endif struct lock_class *class; struct thread *td; int lock_state, rval; td = curthread; - lock_state = 0; -#ifdef KTRACE - if (KTRPOINT(td, KTR_CSW)) - ktrcsw(1, 0, cv_wmesg(cvp)); -#endif CV_ASSERT(cvp, lock, td); WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK, lock, "Waiting on \"%s\"", cvp->cv_description); - class = LOCK_CLASS(lock); if (SCHEDULER_STOPPED_TD(td)) return (0); +#ifdef KTRACE + if (KTRPOINT(td, KTR_CSW)) { + strlcpy(wmesg, cv_wmesg(cvp), sizeof(wmesg)); + ktrcsw(1, 0, wmesg); + } else { + wmesg[0] = '\0'; + } +#endif + + class = LOCK_CLASS(lock); + lock_state = 0; sleepq_lock(cvp); CV_WAITERS_INC(cvp); @@ -311,7 +344,7 @@ _cv_timedwait_sbt(struct cv *cvp, struct lock_object *lock, sbintime_t sbt, #ifdef KTRACE if (KTRPOINT(td, KTR_CSW)) - ktrcsw(0, 0, cv_wmesg(cvp)); + ktrcsw(0, 0, wmesg); #endif PICKUP_GIANT(); if (lock != &Giant.lock_object) { @@ -334,24 +367,32 @@ _cv_timedwait_sig_sbt(struct cv *cvp, struct lock_object *lock, sbintime_t sbt, sbintime_t pr, int flags) { WITNESS_SAVE_DECL(lock_witness); +#ifdef KTRACE + char wmesg[WMESGLEN + 1]; +#endif struct lock_class *class; struct thread *td; int lock_state, rval; td = curthread; - lock_state = 0; -#ifdef KTRACE - if (KTRPOINT(td, KTR_CSW)) - ktrcsw(1, 0, cv_wmesg(cvp)); -#endif CV_ASSERT(cvp, lock, td); WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK, lock, "Waiting on \"%s\"", cvp->cv_description); - class = LOCK_CLASS(lock); if (SCHEDULER_STOPPED_TD(td)) return (0); +#ifdef KTRACE + if (KTRPOINT(td, KTR_CSW)) { + strlcpy(wmesg, cv_wmesg(cvp), sizeof(wmesg)); + ktrcsw(1, 0, wmesg); + } else { + wmesg[0] = '\0'; + } +#endif + + class = LOCK_CLASS(lock); + lock_state = 0; sleepq_lock(cvp); CV_WAITERS_INC(cvp); @@ -374,7 +415,7 @@ _cv_timedwait_sig_sbt(struct cv *cvp, struct lock_object *lock, #ifdef KTRACE if (KTRPOINT(td, KTR_CSW)) - ktrcsw(0, 0, cv_wmesg(cvp)); + ktrcsw(0, 0, wmesg); #endif PICKUP_GIANT(); if (lock != &Giant.lock_object) {
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202401291428.40TES8Oc086714>