From ba9ea6e3d6bf294ccb87299f3744070b96c10a67 Mon Sep 17 00:00:00 2001
From: Suren Baghdasaryan <surenb@google.com>
Date: Tue, 27 Sep 2022 14:30:34 -0700
Subject: [PATCH] lmkd: Fix UAF caused by calling pid_remove() from the
 watchdog thread

pid_remove() is not a thread-safe function and can be called only from
the main thread. Calling it from the watchdog_callback() executed in the
context of the watchdog thread can cause a use-after-free failure if the
same record is being used by the main thread. Fix this issue by marking
the process record as invalid instead of destroying it. Later on invalid
records will be cleared in kill_one_process() called from the context of
the main thread.

Fixes: f8727745f917 ("lmkd: Remove process record after it is killed by lmkd watchdog")
Bug: 248448498
Signed-off-by: Suren Baghdasaryan <surenb@google.com>
Change-Id: I0c7776aea1518c17f0a29904a44b7fe8f33980ca
---
 lmkd.cpp | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/lmkd.cpp b/lmkd.cpp
index e723572..2ee4253 100644
--- a/lmkd.cpp
+++ b/lmkd.cpp
@@ -510,6 +510,7 @@ struct proc {
     uid_t uid;
     int oomadj;
     pid_t reg_pid; /* PID of the process that registered this record */
+    bool valid;
     struct proc *pidhash_next;
 };
 
@@ -941,6 +942,7 @@ static void proc_insert(struct proc *procp) {
     proc_slot(procp);
 }
 
+// Can be called only from the main thread.
 static int pid_remove(int pid) {
     int hval = pid_hashfn(pid);
     struct proc *procp;
@@ -970,6 +972,15 @@ static int pid_remove(int pid) {
     return 0;
 }
 
+static void pid_invalidate(int pid) {
+    std::shared_lock lock(adjslot_list_lock);
+    struct proc *procp = pid_lookup(pid);
+
+    if (procp) {
+        procp->valid = false;
+    }
+}
+
 /*
  * Write a string to a file.
  * Returns false if the file does not exist.
@@ -1220,6 +1231,7 @@ static void cmd_procprio(LMKD_CTRL_PACKET packet, int field_count, struct ucred
         procp->uid = params.uid;
         procp->reg_pid = cred->pid;
         procp->oomadj = params.oomadj;
+        procp->valid = true;
         proc_insert(procp);
     } else {
         if (!claim_record(procp, cred->pid)) {
@@ -2092,7 +2104,7 @@ static struct proc *proc_adj_prev(int oomadj, int pid) {
     return NULL;
 }
 
-// When called from a non-main thread, adjslot_list_lock read lock should be taken.
+// Can be called only from the main thread.
 static struct proc *proc_get_heaviest(int oomadj) {
     struct adjslot_list *head = &procadjslot_list[ADJTOSLOT(oomadj)];
     struct adjslot_list *curr = head->next;
@@ -2154,11 +2166,11 @@ static void watchdog_callback() {
             continue;
         }
 
-        if (reaper.kill({ target.pidfd, target.pid, target.uid }, true) == 0) {
+        if (target.valid && reaper.kill({ target.pidfd, target.pid, target.uid }, true) == 0) {
             ALOGW("lmkd watchdog killed process %d, oom_score_adj %d", target.pid, oom_score);
             killinfo_log(&target, 0, 0, 0, NULL, NULL, NULL, NULL, NULL);
-            // WARNING: do not use target after pid_remove()
-            pid_remove(target.pid);
+            // Can't call pid_remove() from non-main thread, therefore just invalidate the record
+            pid_invalidate(target.pid);
             break;
         }
         prev_pid = target.pid;
@@ -2296,7 +2308,7 @@ static int kill_one_process(struct proc* procp, int min_oom_score, struct kill_i
     char buf[PAGE_SIZE];
     char desc[LINE_MAX];
 
-    if (!read_proc_status(pid, buf, sizeof(buf))) {
+    if (!procp->valid || !read_proc_status(pid, buf, sizeof(buf))) {
         goto out;
     }
     if (!parse_status_tag(buf, PROC_STATUS_TGID_FIELD, &tgid)) {