From: Greg Kroah-Hartman Date: Thu, 7 Jul 2022 18:07:14 +0000 (+0200) Subject: 4.9-stable patches X-Git-Tag: v4.9.323~68 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=d28a4c012ebfe7ab45fafc55cd14447ade4f66fc;p=thirdparty%2Fkernel%2Fstable-queue.git 4.9-stable patches added patches: mm-slub-add-missing-tid-updates-on-slab-deactivation.patch --- diff --git a/queue-4.9/mm-slub-add-missing-tid-updates-on-slab-deactivation.patch b/queue-4.9/mm-slub-add-missing-tid-updates-on-slab-deactivation.patch new file mode 100644 index 00000000000..89019cf284c --- /dev/null +++ b/queue-4.9/mm-slub-add-missing-tid-updates-on-slab-deactivation.patch @@ -0,0 +1,137 @@ +From eeaa345e128515135ccb864c04482180c08e3259 Mon Sep 17 00:00:00 2001 +From: Jann Horn +Date: Wed, 8 Jun 2022 20:22:05 +0200 +Subject: mm/slub: add missing TID updates on slab deactivation + +From: Jann Horn + +commit eeaa345e128515135ccb864c04482180c08e3259 upstream. + +The fastpath in slab_alloc_node() assumes that c->slab is stable as long as +the TID stays the same. However, two places in __slab_alloc() currently +don't update the TID when deactivating the CPU slab. + +If multiple operations race the right way, this could lead to an object +getting lost; or, in an even more unlikely situation, it could even lead to +an object being freed onto the wrong slab's freelist, messing up the +`inuse` counter and eventually causing a page to be freed to the page +allocator while it still contains slab objects. + +(I haven't actually tested these cases though, this is just based on +looking at the code. Writing testcases for this stuff seems like it'd be +a pain...) + +The race leading to state inconsistency is (all operations on the same CPU +and kmem_cache): + + - task A: begin do_slab_free(): + - read TID + - read pcpu freelist (==NULL) + - check `slab == c->slab` (true) + - [PREEMPT A->B] + - task B: begin slab_alloc_node(): + - fastpath fails (`c->freelist` is NULL) + - enter __slab_alloc() + - slub_get_cpu_ptr() (disables preemption) + - enter ___slab_alloc() + - take local_lock_irqsave() + - read c->freelist as NULL + - get_freelist() returns NULL + - write `c->slab = NULL` + - drop local_unlock_irqrestore() + - goto new_slab + - slub_percpu_partial() is NULL + - get_partial() returns NULL + - slub_put_cpu_ptr() (enables preemption) + - [PREEMPT B->A] + - task A: finish do_slab_free(): + - this_cpu_cmpxchg_double() succeeds() + - [CORRUPT STATE: c->slab==NULL, c->freelist!=NULL] + +From there, the object on c->freelist will get lost if task B is allowed to +continue from here: It will proceed to the retry_load_slab label, +set c->slab, then jump to load_freelist, which clobbers c->freelist. + +But if we instead continue as follows, we get worse corruption: + + - task A: run __slab_free() on object from other struct slab: + - CPU_PARTIAL_FREE case (slab was on no list, is now on pcpu partial) + - task A: run slab_alloc_node() with NUMA node constraint: + - fastpath fails (c->slab is NULL) + - call __slab_alloc() + - slub_get_cpu_ptr() (disables preemption) + - enter ___slab_alloc() + - c->slab is NULL: goto new_slab + - slub_percpu_partial() is non-NULL + - set c->slab to slub_percpu_partial(c) + - [CORRUPT STATE: c->slab points to slab-1, c->freelist has objects + from slab-2] + - goto redo + - node_match() fails + - goto deactivate_slab + - existing c->freelist is passed into deactivate_slab() + - inuse count of slab-1 is decremented to account for object from + slab-2 + +At this point, the inuse count of slab-1 is 1 lower than it should be. +This means that if we free all allocated objects in slab-1 except for one, +SLUB will think that slab-1 is completely unused, and may free its page, +leading to use-after-free. + +Fixes: c17dda40a6a4e ("slub: Separate out kmem_cache_cpu processing from deactivate_slab") +Fixes: 03e404af26dc2 ("slub: fast release on full slab") +Cc: stable@vger.kernel.org +Signed-off-by: Jann Horn +Acked-by: Christoph Lameter +Acked-by: David Rientjes +Reviewed-by: Muchun Song +Tested-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> +Signed-off-by: Vlastimil Babka +Link: https://lore.kernel.org/r/20220608182205.2945720-1-jannh@google.com +Signed-off-by: Greg Kroah-Hartman +--- + mm/slub.c | 5 +++++ + 1 file changed, 5 insertions(+) + +--- a/mm/slub.c ++++ b/mm/slub.c +@@ -2556,6 +2556,7 @@ redo: + deactivate_slab(s, page, c->freelist); + c->page = NULL; + c->freelist = NULL; ++ c->tid = next_tid(c->tid); + goto new_slab; + } + } +@@ -2569,6 +2570,7 @@ redo: + deactivate_slab(s, page, c->freelist); + c->page = NULL; + c->freelist = NULL; ++ c->tid = next_tid(c->tid); + goto new_slab; + } + +@@ -2581,6 +2583,7 @@ redo: + + if (!freelist) { + c->page = NULL; ++ c->tid = next_tid(c->tid); + stat(s, DEACTIVATE_BYPASS); + goto new_slab; + } +@@ -2605,6 +2608,7 @@ new_slab: + c->partial = page->next; + stat(s, CPU_PARTIAL_ALLOC); + c->freelist = NULL; ++ c->tid = next_tid(c->tid); + goto redo; + } + +@@ -2627,6 +2631,7 @@ new_slab: + deactivate_slab(s, page, get_freepointer(s, freelist)); + c->page = NULL; + c->freelist = NULL; ++ c->tid = next_tid(c->tid); + return freelist; + } + diff --git a/queue-4.9/series b/queue-4.9/series new file mode 100644 index 00000000000..5855d9c10a3 --- /dev/null +++ b/queue-4.9/series @@ -0,0 +1 @@ +mm-slub-add-missing-tid-updates-on-slab-deactivation.patch