Clear current before calling proc_decref()
authorBarret Rhoden <brho@cs.berkeley.edu>
Tue, 25 Sep 2018 01:19:37 +0000 (21:19 -0400)
committerBarret Rhoden <brho@cs.berkeley.edu>
Tue, 25 Sep 2018 01:19:37 +0000 (21:19 -0400)
'current,' aka pcpui->cur_proc, is a counted reference that is viewable
from code other than the code that is clearing the ref.  Even though
it's a per-core data structure, it's just like a ref in a global data
structure.

In this case, if you decref current when you are not guaranteed to have
an extra ref, you could have dropped the final reference.  Anyone who
uses 'current' after that is basically a use-after-free bug.

Here's a specific backtrace of the bug.  A process was exiting and the
final ref was dropped in __abandon_core(), but not cleared from current.
The process had a chan that was decreffed in __proc_free(), and that
chan eventually needed to block.  sem_down() saw 'current' and attempted
to save a reference to it, even though the process was basically freed.

 Stack Backtrace on Core 0:
 #01 [<0xffffffffc200a00c>] in backtrace
 #02 [<0xffffffffc20097fd>] in _panic
 #03 [<0xffffffffc2048ef6>] in kref_get
 #04 [<0xffffffffc200ba71>] in sem_down
 #05 [<0xffffffffc200bdbb>] in sem_down_bulk
 #06 [<0xffffffffc2050189>] in rcu_barrier
 #07 [<0xffffffffc2041ec6>] in tf_dfs_purge
 #08 [<0xffffffffc2041f90>] in tf_dfs_purge
 #09 [<0xffffffffc2041f90>] in tf_dfs_purge
 #10 [<0xffffffffc2041f90>] in tf_dfs_purge
 #11 [<0xffffffffc2041f90>] in tf_dfs_purge
 #12 [<0xffffffffc2079050>] in gtfs_release
 #13 [<0xffffffffc2078deb>] in kref_put
 #14 [<0xffffffffc20315d0>] in chan_release
 #15 [<0xffffffffc2030cfb>] in kref_put
 #16 [<0xffffffffc2048b1a>] in __proc_free
 #17 [<0xffffffffc2048a6b>] in kref_put
 #18 [<0xffffffffc20a6f21>] in __abandon_core
 #19 [<0xffffffffc204c8b9>] in abandon_core
 #20 [<0xffffffffc20539cd>] in __smp_idle

Signed-off-by: Barret Rhoden <brho@cs.berkeley.edu>
kern/arch/riscv/process.c
kern/arch/x86/process64.c
kern/src/kthread.c
kern/src/process.c

index 47d0f5d..70d14ad 100644 (file)
@@ -61,9 +61,12 @@ void proc_secure_ctx(struct user_context *ctx)
 void __abandon_core(void)
 {
        struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
+       struct proc *old_proc;
+
        lcr3(boot_cr3);
-       proc_decref(pcpui->cur_proc);
-       pcpui->cur_proc = 0;
+       old_proc = pcpui->cur_proc;
+       pcpui->cur_proc = NULL;
+       proc_decref(old_proc);
 }
 
 void __clear_owning_proc(uint32_t coreid)
index f6eb152..1b7c3fc 100644 (file)
@@ -354,10 +354,12 @@ void proc_secure_ctx(struct user_context *ctx)
 void __abandon_core(void)
 {
        struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
+       struct proc *old_proc;
 
        lcr3(boot_cr3);
-       proc_decref(pcpui->cur_proc);
-       pcpui->cur_proc = 0;
+       old_proc = pcpui->cur_proc;
+       pcpui->cur_proc = NULL;
+       proc_decref(old_proc);
 }
 
 void __clear_owning_proc(uint32_t coreid)
index c9d1b77..0439f99 100644 (file)
@@ -127,6 +127,8 @@ void restart_kthread(struct kthread *kthread)
        struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
        uintptr_t current_stacktop;
        struct kthread *cur_kth;
+       struct proc *old_proc;
+
        /* Avoid messy complications.  The kthread will enable_irqsave() when it
         * comes back up. */
        disable_irq();
@@ -164,11 +166,12 @@ void restart_kthread(struct kthread *kthread)
                        lcr3(kthread->proc->env_cr3);
                        /* Might have to clear out an existing current.  If they need to be
                         * set later (like in restartcore), it'll be done on demand. */
-                       if (pcpui->cur_proc)
-                               proc_decref(pcpui->cur_proc);
+                       old_proc = pcpui->cur_proc;
                        /* Transfer our counted ref from kthread->proc to cur_proc. */
                        pcpui->cur_proc = kthread->proc;
                        kthread->proc = 0;
+                       if (old_proc)
+                               proc_decref(old_proc);
                }
        }
        /* Finally, restart our thread */
index 11a936e..7197e11 100644 (file)
@@ -577,6 +577,8 @@ static void __set_proc_current(struct proc *p)
        /* We use the pcpui to access 'current' to cut down on the core_id() calls,
         * though who know how expensive/painful they are. */
        struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
+       struct proc *old_proc;
+
        /* If the process wasn't here, then we need to load its address space. */
        if (p != pcpui->cur_proc) {
                proc_incref(p, 1);
@@ -585,9 +587,10 @@ static void __set_proc_current(struct proc *p)
                 * previous lcr3 unloaded the previous proc's context.  This should
                 * rarely happen, since we usually proactively leave process context,
                 * but this is the fallback. */
-               if (pcpui->cur_proc)
-                       proc_decref(pcpui->cur_proc);
+               old_proc = pcpui->cur_proc;
                pcpui->cur_proc = p;
+               if (old_proc)
+                       proc_decref(old_proc);
        }
 }
 
@@ -869,8 +872,8 @@ void proc_destroy(struct proc *p)
                        // here's how to do it manually
                        if (current == p) {
                                lcr3(boot_cr3);
-                               proc_decref(p);         /* this decref is for the cr3 */
                                current = NULL;
+                               proc_decref(p);         /* this decref is for the cr3 */
                        }
                        #endif
                        send_kernel_message(get_pcoreid(p, 0), __death, (long)p, 0, 0,