Skip to contentSkip to conversation

psi: Fix uaf issue when psi trigger is destroyed while being polled

PATCH In review Suren Baghdasaryan 2 participants 2 messages Feb 1, 2022 – Feb 3, 2022

Subsystems (5)
Stable branch Documentation Scheduler cgroup +1 more subsystems
lore
Share & tools
lore

Suren Baghdasaryan

author maintainer
#0 lore raw

PATCH 1/1 psi: Fix uaf issue when psi trigger is destroyed while being polled

commit a06247c6804f1a7c86a2e5398a4c1f1db1471848 upstream.

With write operation on psi files replacing old trigger with a new one, the lifetime of its waitqueue is totally arbitrary. Overwriting an existing trigger causes its waitqueue to be freed and pending poll() will stumble on trigger->event_wait which was destroyed.
Fix this by disallowing to redefine an existing psi trigger. If a write operation is used on a file descriptor with an already existing psi trigger, the operation will fail with EBUSY error.
Also bypass a check for psi_disabled in the psi_trigger_destroy as the flag can be flipped after the trigger is created, leading to a memory leak.

Fixes 0e94682b73bf ("psi: introduce psi monitor")
Reported-by [email protected]
Suggested-by Linus Torvalds <[email protected]>
Analyzed-by Eric Biggers <[email protected]>
Signed-off-by Suren Baghdasaryan <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Reviewed-by Eric Biggers <[email protected]>
Acked-by Johannes Weiner <[email protected]>
Cc [email protected]
Link https://lore.kernel.org/r/[email protected] open here
[surenb: backported to 5.10 kernel]
CC [email protected] # 5.10
Signed-off-by Suren Baghdasaryan <[email protected]>
---
 Documentation/accounting/psi.rst |  3 +-
 include/linux/psi.h              |  2 +-
 include/linux/psi_types.h        |  3 --
 kernel/cgroup/cgroup.c           | 11 ++++--
 kernel/sched/psi.c               | 66 ++++++++++++++------------------
 5 files changed, 40 insertions(+), 45 deletions(-)
Documentation/accounting/psi.rst +2 -1
index f2b3439edcc2..860fe651d645 100644
@@ -92,7 +92,8 @@ Triggers can be set on more than one psi metric and more than one trigger
 for the same psi metric can be specified. However for each trigger a separate
 file descriptor is required to be able to poll it separately from others,
 therefore for each trigger a separate open() syscall should be made even
-when opening the same psi interface file.
+when opening the same psi interface file. Write operations to a file descriptor
+with an already existing psi trigger will fail with EBUSY.
 
 Monitors activate only when system enters stall state for the monitored
 psi metric and deactivates upon exit from the stall state. While system is
include/linux/psi.h +1 -1
index 7361023f3fdd..db4ecfaab879 100644
@@ -33,7 +33,7 @@ void cgroup_move_task(struct task_struct *p, struct css_set *to);
 
 struct psi_trigger *psi_trigger_create(struct psi_group *group,
 			char *buf, size_t nbytes, enum psi_res res);
-void psi_trigger_replace(void **trigger_ptr, struct psi_trigger *t);
+void psi_trigger_destroy(struct psi_trigger *t);
 
 __poll_t psi_trigger_poll(void **trigger_ptr, struct file *file,
 			poll_table *wait);
include/linux/psi_types.h +0 -3
index b95f3211566a..17d74f62c181 100644
@@ -128,9 +128,6 @@ struct psi_trigger {
 	 * events to one per window
 	 */
 	u64 last_event_time;
-
-	/* Refcounting to prevent premature destruction */
-	struct kref refcount;
 };
 
 struct psi_group {
kernel/cgroup/cgroup.c +8 -3
index a86857edaa57..4927289a91a9 100644
@@ -3601,6 +3601,12 @@ static ssize_t cgroup_pressure_write(struct kernfs_open_file *of, char *buf,
 	cgroup_get(cgrp);
 	cgroup_kn_unlock(of->kn);
 
+	/* Allow only one trigger per file descriptor */
+	if (of->priv) {
+		cgroup_put(cgrp);
+		return -EBUSY;
+	}
+
 	psi = cgroup_ino(cgrp) == 1 ? &psi_system : &cgrp->psi;
 	new = psi_trigger_create(psi, buf, nbytes, res);
 	if (IS_ERR(new)) {

@@ -3608,8 +3614,7 @@ static ssize_t cgroup_pressure_write(struct kernfs_open_file *of, char *buf,
 		return PTR_ERR(new);
 	}
 
-	psi_trigger_replace(&of->priv, new);
-
+	smp_store_release(&of->priv, new);
 	cgroup_put(cgrp);
 
 	return nbytes;

@@ -3644,7 +3649,7 @@ static __poll_t cgroup_pressure_poll(struct kernfs_open_file *of,
 
 static void cgroup_pressure_release(struct kernfs_open_file *of)
 {
-	psi_trigger_replace(&of->priv, NULL);
+	psi_trigger_destroy(of->priv);
 }
 #endif /* CONFIG_PSI */
kernel/sched/psi.c +29 -37

123 lines not on this page · raw message

Signature · 1 line
2.35.0.rc2.247.g8bbb082509-goog
maintainer
#1 lore raw
Quoted 26 lines · Suren Baghdasaryan

commit a06247c6804f1a7c86a2e5398a4c1f1db1471848 upstream.

With write operation on psi files replacing old trigger with a new one, the lifetime of its waitqueue is totally arbitrary. Overwriting an existing trigger causes its waitqueue to be freed and pending poll() will stumble on trigger->event_wait which was destroyed.
Fix this by disallowing to redefine an existing psi trigger. If a write operation is used on a file descriptor with an already existing psi trigger, the operation will fail with EBUSY error.
Also bypass a check for psi_disabled in the psi_trigger_destroy as the flag can be flipped after the trigger is created, leading to a memory leak.

Fixes 0e94682b73bf ("psi: introduce psi monitor")
Reported-by [email protected]
Suggested-by Linus Torvalds <[email protected]>
Analyzed-by Eric Biggers <[email protected]>
Signed-off-by Suren Baghdasaryan <[email protected]>
Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Reviewed-by Eric Biggers <[email protected]>
Acked-by Johannes Weiner <[email protected]>
Cc [email protected]
Link https://lore.kernel.org/r/[email protected] open here
[surenb: backported to 5.10 kernel]
CC [email protected] # 5.10
Signed-off-by Suren Baghdasaryan <[email protected]>
Quoted 7 lines
---
 Documentation/accounting/psi.rst |  3 +-
 include/linux/psi.h              |  2 +-
 include/linux/psi_types.h        |  3 --
 kernel/cgroup/cgroup.c           | 11 ++++--
 kernel/sched/psi.c               | 66 ++++++++++++++------------------
 5 files changed, 40 insertions(+), 45 deletions(-)

Both backports now queued up, thanks.

greg k-h