| Message ID | 4fbc6668845e699bf708aee5c11ad9fd012d4dcd.1638308023.git.andreyknvl@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | kasan, vmalloc, arm64: add vmalloc tagging support for SW/HW_TAGS | expand |
On Tue, Nov 30, 2021 at 11:07PM +0100, andrey.konovalov@linux.dev wrote: > From: Andrey Konovalov <andreyknvl@google.com> > > Once tag-based KASAN modes start tagging vmalloc() allocations, > kernel stacks will start getting tagged if CONFIG_VMAP_STACK is enabled. > > Reset the tag of kernel stack pointers after allocation. > > For SW_TAGS KASAN, when CONFIG_KASAN_STACK is enabled, the > instrumentation can't handle the sp register being tagged. > > For HW_TAGS KASAN, there's no instrumentation-related issues. However, > the impact of having a tagged SP pointer needs to be properly evaluated, > so keep it non-tagged for now. Don't VMAP_STACK stacks have guards? So some out-of-bounds would already be caught. What would be the hypothetical benefit of using a tagged stack pointer? Perhaps wildly out-of-bounds accesses derived from stack pointers? I agree that unless we understand the impact of using a tagged stack pointers, it should remain non-tagged for now. > Note, that the memory for the stack allocation still gets tagged to > catch vmalloc-into-stack out-of-bounds accesses. Will the fact it's tagged cause issues for other code? I think kmemleak already untags all addresses it scans for pointers. Anything else? > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> > --- > kernel/fork.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/kernel/fork.c b/kernel/fork.c > index 3244cc56b697..062d1484ef42 100644 > --- a/kernel/fork.c > +++ b/kernel/fork.c > @@ -253,6 +253,7 @@ static unsigned long *alloc_thread_stack_node(struct task_struct *tsk, int node) > * so cache the vm_struct. > */ > if (stack) { > + stack = kasan_reset_tag(stack); > tsk->stack_vm_area = find_vm_area(stack); > tsk->stack = stack; > } > -- > 2.25.1 >
On Thu, Dec 2, 2021 at 3:28 PM Marco Elver <elver@google.com> wrote: > > On Tue, Nov 30, 2021 at 11:07PM +0100, andrey.konovalov@linux.dev wrote: > > From: Andrey Konovalov <andreyknvl@google.com> > > > > Once tag-based KASAN modes start tagging vmalloc() allocations, > > kernel stacks will start getting tagged if CONFIG_VMAP_STACK is enabled. > > > > Reset the tag of kernel stack pointers after allocation. > > > > For SW_TAGS KASAN, when CONFIG_KASAN_STACK is enabled, the > > instrumentation can't handle the sp register being tagged. > > > > For HW_TAGS KASAN, there's no instrumentation-related issues. However, > > the impact of having a tagged SP pointer needs to be properly evaluated, > > so keep it non-tagged for now. > > Don't VMAP_STACK stacks have guards? So some out-of-bounds would already > be caught. True, linear out-of-bounds accesses are already caught. > What would be the hypothetical benefit of using a tagged stack pointer? > Perhaps wildly out-of-bounds accesses derived from stack pointers? Yes, that's the case that comes to mind. > I agree that unless we understand the impact of using a tagged stack > pointers, it should remain non-tagged for now. Ack. I'll file a KASAN bug for this when the series is merged. > > Note, that the memory for the stack allocation still gets tagged to > > catch vmalloc-into-stack out-of-bounds accesses. > > Will the fact it's tagged cause issues for other code? I think kmemleak > already untags all addresses it scans for pointers. Anything else? Tagging stack memory shouldn't cause any stability issues like conflicts with kmemleak. Tagging memory but not the pointers is not worse than leaving memory tags uninitialized/random with regards to this kind of issues. Thanks!
diff --git a/kernel/fork.c b/kernel/fork.c index 3244cc56b697..062d1484ef42 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -253,6 +253,7 @@ static unsigned long *alloc_thread_stack_node(struct task_struct *tsk, int node) * so cache the vm_struct. */ if (stack) { + stack = kasan_reset_tag(stack); tsk->stack_vm_area = find_vm_area(stack); tsk->stack = stack; }