[v2] fork: reset pointer tag of vmapped thread stack before vfree

[PATCH v2] fork: reset pointer tag of vmapped thread stack before vfree

Posted by Shaobo Huang 1 week, 4 days ago

thread_stack_free_rcu() frees the vmalloc'd thread stack via
vfree(vm_area->addr).  In RCU callback context, vfree() routes to
vfree_atomic(), which calls llist_add((struct llist_node *)addr, ...)
and writes 8 bytes to the base of the region being freed.

With KASAN_SW_TAGS, vm_area->addr carries a random tag.  If
kasan_unpoison_task_stack_below() has rewritten the shadow covering
[base, sp] to KASAN_TAG_KERNEL (0xff) -- which it does on every CPU
resume for the current task's stack -- the llist_add store checks
shadow[base] (0xff) against the pointer tag (random) and reports an
invalid-access, although writing to the base of a stack queued for
deferred free is legitimate.

Reset the pointer tag to KASAN_TAG_KERNEL before vfree() so that
kasan_check_range() short-circuits the check, the same way the task
accesses its own stack at runtime via sp.  The vmalloc lookup is safe:
__find_vmap_area() resets the tag before comparing against va_start.

Fixes: 9f7d416c3612 ("kprobes: Unpoison stack in jprobe_return() for KASAN")
Cc: stable@vger.kernel.org
Assisted-by: zhipuai:glm-5.2
Signed-off-by: Shaobo Huang <huangshaobo3@xiaomi.com>
---
Changes since v1:
- Drop the 12-line comment; keep just the one-line fix.
- Fix the Fixes: tag to point to the commit that introduced
  kasan_unpoison_task_stack_below (9f7d416c3612), which added
  both the function definition and the _cpu_resume call site, not
  the 2016 vfree_atomic commit.
- Add Assisted-by tag per Documentation/process/coding-assistants.rst.
- Use real name (Shaobo Huang) instead of "sparkhuang".
- Trim the commit message; remove the full KASAN dump.
v1:
  https://lore.kernel.org/all/20260806123020.90869-1-huangshaobo3@xiaomi.com/
---
 kernel/fork.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index 45300f59cf2c..9a66b10749de 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -238,7 +238,7 @@ static void thread_stack_free_rcu(struct rcu_head *rh)
 	if (try_release_thread_stack_to_cache(vm_stack->stack_vm_area))
 		return;
 
-	vfree(vm_area->addr);
+	vfree(kasan_reset_tag(vm_area->addr));
 }
 
 static void thread_stack_delayed_free(struct task_struct *tsk)
-- 
2.34.1

Re: [PATCH v2] fork: reset pointer tag of vmapped thread stack before vfree

Posted by Andrey Ryabinin 1 week, 1 day ago

Shaobo Huang <huangshaobo3@xiaomi.com> writes:

> thread_stack_free_rcu() frees the vmalloc'd thread stack via
> vfree(vm_area->addr).  In RCU callback context, vfree() routes to
> vfree_atomic(), which calls llist_add((struct llist_node *)addr, ...)
> and writes 8 bytes to the base of the region being freed.
>
> With KASAN_SW_TAGS, vm_area->addr carries a random tag.  If
> kasan_unpoison_task_stack_below() has rewritten the shadow covering
> [base, sp] to KASAN_TAG_KERNEL (0xff) -- which it does on every CPU
> resume for the current task's stack -- the llist_add store checks
> shadow[base] (0xff) against the pointer tag (random) and reports an
> invalid-access, although writing to the base of a stack queued for
> deferred free is legitimate.
>
> Reset the pointer tag to KASAN_TAG_KERNEL before vfree() so that
> kasan_check_range() short-circuits the check, the same way the task
> accesses its own stack at runtime via sp.  The vmalloc lookup is safe:
> __find_vmap_area() resets the tag before comparing against va_start.
>
> Fixes: 9f7d416c3612 ("kprobes: Unpoison stack in jprobe_return() for KASAN")

This commit merely renamed the resume helper without changing what it
unpoisons. Only generic KASAN existed back then, so it cannot have
introduced a tagged-mode problem.

The problem became visible with

  449e0b4ed5a1 ("fork: clean-up naming of vm_stack/vm_struct variables
in vmap stacks code")

which changed thread_stack_free_rcu() from vfree(vm_stack) to
vfree(vm_area->addr). vm_stack is derived from tsk->stack, whose tag
alloc_thread_stack_node() resets to 0xff. Since that commit, vfree_atomic()
gets the tagged vm_area->addr instead.

> Cc: stable@vger.kernel.org
> Assisted-by: zhipuai:glm-5.2
> Signed-off-by: Shaobo Huang <huangshaobo3@xiaomi.com>
> ---
> Changes since v1:
> - Drop the 12-line comment; keep just the one-line fix.
> - Fix the Fixes: tag to point to the commit that introduced
>   kasan_unpoison_task_stack_below (9f7d416c3612), which added
>   both the function definition and the _cpu_resume call site, not
>   the 2016 vfree_atomic commit.
> - Add Assisted-by tag per Documentation/process/coding-assistants.rst.
> - Use real name (Shaobo Huang) instead of "sparkhuang".
> - Trim the commit message; remove the full KASAN dump.
> v1:
>   https://lore.kernel.org/all/20260806123020.90869-1-huangshaobo3@xiaomi.com/
> ---
>  kernel/fork.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 45300f59cf2c..9a66b10749de 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -238,7 +238,7 @@ static void thread_stack_free_rcu(struct rcu_head *rh)
>  	if (try_release_thread_stack_to_cache(vm_stack->stack_vm_area))
>  		return;
>
> -	vfree(vm_area->addr);
> +	vfree(kasan_reset_tag(vm_area->addr));

Thanks for report the problem and the patch. NAK on the fix though.
It addresses the symptom rather than the cause.

The problem is not that vfree() is called with a tagged pointer. The
problem is the 0xff written into the shadow by
kasan_unpoison_task_stack_below().

That helper is only needed in generic mode. There the compiler expects
the stack shadow to be zero, so the redzones left behind by discarded
frames must be cleared before the stack is reused.

The tagged mode has no such requirement. The compiler fully initializes
the shadow of stack variables on function entry, so stale tags left
below the watermark are harmless.

kasan_unpoison_task_stack_below() should simply be a no-op in tagged
mode. I'll send a patch.

Re: [PATCH v2] fork: reset pointer tag of vmapped thread stack before vfree

Posted by Lorenzo Stoakes (ARM) 1 week, 4 days ago

+cc Ulad for vmalloc stuff.

Please don't send a v2 in-reply-to a v1 or any other email.

Send the patch entirely separately.

I really need to write a bot to say this :)...

On Mon, Sep 14, 2026 at 05:33:00PM +0800, Shaobo Huang wrote:
> thread_stack_free_rcu() frees the vmalloc'd thread stack via
> vfree(vm_area->addr).  In RCU callback context, vfree() routes to
> vfree_atomic(), which calls llist_add((struct llist_node *)addr, ...)
> and writes 8 bytes to the base of the region being freed.
>
> With KASAN_SW_TAGS, vm_area->addr carries a random tag.  If
> kasan_unpoison_task_stack_below() has rewritten the shadow covering
> [base, sp] to KASAN_TAG_KERNEL (0xff) -- which it does on every CPU
> resume for the current task's stack -- the llist_add store checks
> shadow[base] (0xff) against the pointer tag (random) and reports an
> invalid-access, although writing to the base of a stack queued for
> deferred free is legitimate.
>
> Reset the pointer tag to KASAN_TAG_KERNEL before vfree() so that
> kasan_check_range() short-circuits the check, the same way the task
> accesses its own stack at runtime via sp.  The vmalloc lookup is safe:
> __find_vmap_area() resets the tag before comparing against va_start.
>
> Fixes: 9f7d416c3612 ("kprobes: Unpoison stack in jprobe_return() for KASAN")
> Cc: stable@vger.kernel.org
> Assisted-by: zhipuai:glm-5.2

Thanks for adding this!

New convention is to say:

	Assisted-by: LLM

Rather than to list the agent.

See https://docs.kernel.org/process/coding-assistants.html

> Signed-off-by: Shaobo Huang <huangshaobo3@xiaomi.com>

Looks reasonable to me so, with nits addressed:

Acked-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>

But I would like Ulad's input on this from vmalloc side.

> ---
> Changes since v1:

Thanks for adding this!

Nit, but nicer to say 'v2:' here I think.

> - Drop the 12-line comment; keep just the one-line fix.
> - Fix the Fixes: tag to point to the commit that introduced
>   kasan_unpoison_task_stack_below (9f7d416c3612), which added
>   both the function definition and the _cpu_resume call site, not
>   the 2016 vfree_atomic commit.
> - Add Assisted-by tag per Documentation/process/coding-assistants.rst.
> - Use real name (Shaobo Huang) instead of "sparkhuang".
> - Trim the commit message; remove the full KASAN dump.

Please give credit to reviewers for each change, e.g. 'as per XXX' :)

> v1:
>   https://lore.kernel.org/all/20260806123020.90869-1-huangshaobo3@xiaomi.com/
> ---
>  kernel/fork.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 45300f59cf2c..9a66b10749de 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -238,7 +238,7 @@ static void thread_stack_free_rcu(struct rcu_head *rh)
>  	if (try_release_thread_stack_to_cache(vm_stack->stack_vm_area))
>  		return;
>
> -	vfree(vm_area->addr);
> +	vfree(kasan_reset_tag(vm_area->addr));
>  }
>
>  static void thread_stack_delayed_free(struct task_struct *tsk)
> --
> 2.34.1
>

--
Cheers, Lorenzo

Re: [PATCH v2] fork: reset pointer tag of vmapped thread stack before vfree

Posted by Uladzislau Rezki 1 week, 2 days ago

On Mon, Sep 14, 2026 at 10:44:26AM +0100, Lorenzo Stoakes (ARM) wrote:
> +cc Ulad for vmalloc stuff.
> 
> Please don't send a v2 in-reply-to a v1 or any other email.
> 
> Send the patch entirely separately.
> 
> I really need to write a bot to say this :)...
> 
> On Mon, Sep 14, 2026 at 05:33:00PM +0800, Shaobo Huang wrote:
> > thread_stack_free_rcu() frees the vmalloc'd thread stack via
> > vfree(vm_area->addr).  In RCU callback context, vfree() routes to
> > vfree_atomic(), which calls llist_add((struct llist_node *)addr, ...)
> > and writes 8 bytes to the base of the region being freed.
> >
> > With KASAN_SW_TAGS, vm_area->addr carries a random tag.  If
> > kasan_unpoison_task_stack_below() has rewritten the shadow covering
> > [base, sp] to KASAN_TAG_KERNEL (0xff) -- which it does on every CPU
> > resume for the current task's stack -- the llist_add store checks
> > shadow[base] (0xff) against the pointer tag (random) and reports an
> > invalid-access, although writing to the base of a stack queued for
> > deferred free is legitimate.
> >
> > Reset the pointer tag to KASAN_TAG_KERNEL before vfree() so that
> > kasan_check_range() short-circuits the check, the same way the task
> > accesses its own stack at runtime via sp.  The vmalloc lookup is safe:
> > __find_vmap_area() resets the tag before comparing against va_start.
> >
> > Fixes: 9f7d416c3612 ("kprobes: Unpoison stack in jprobe_return() for KASAN")
> > Cc: stable@vger.kernel.org
> > Assisted-by: zhipuai:glm-5.2
> 
> Thanks for adding this!
> 
> New convention is to say:
> 
> 	Assisted-by: LLM
> 
> Rather than to list the agent.
> 
> See https://docs.kernel.org/process/coding-assistants.html
> 
> > Signed-off-by: Shaobo Huang <huangshaobo3@xiaomi.com>
> 
> Looks reasonable to me so, with nits addressed:
> 
> Acked-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
> 
> But I would like Ulad's input on this from vmalloc side.
> 
Makes sense to me even though we do it in the __find_vmap_area()
but we also use an "addr" in other paths.

LGTM:

Reviewed-by: Uladzislau Rezki (Sony) <urezki@gmail.com>

--
Uladzislau Rezki