* Re: [PATCH] kasan: Fix tag for large allocations when using CONFIG_SLAB
2021-10-01 2:41 [PATCH] kasan: Fix tag for large allocations when using CONFIG_SLAB Matthew Wilcox (Oracle)
@ 2021-10-01 10:30 ` Marco Elver
2021-10-01 13:29 ` Andrey Konovalov
2021-10-03 16:27 ` Andrey Konovalov
2 siblings, 0 replies; 6+ messages in thread
From: Marco Elver @ 2021-10-01 10:30 UTC (permalink / raw)
To: Matthew Wilcox (Oracle)
Cc: Andrey Ryabinin, Alexander Potapenko, Andrey Konovalov,
Dmitry Vyukov, Andrew Morton, kasan-dev, linux-mm, linux-kernel
On Fri, 1 Oct 2021 at 04:42, Matthew Wilcox (Oracle)
<willy@infradead.org> wrote:
> If an object is allocated on a tail page of a multi-page slab, kasan
> will get the wrong tag because page->s_mem is NULL for tail pages.
> I'm not quite sure what the user-visible effect of this might be.
>
> Fixes: 7f94ffbc4c6a ("kasan: add hooks implementation for tag-based mode")
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Acked-by: Marco Elver <elver@google.com>
Indeed this looks wrong. I don't know how much this code is even
tested, because it depends on CONFIG_KASAN_SW_TAGS && CONFIG_SLAB, and
the cache having a constructor or SLAB_TYPESAFE_BY_RCU. HW_TAGS isn't
affected because it doesn't work with SLAB.
And to run SW_TAGS, one needs an arm64 CPU with TBI. And the instances
of KASAN_SW_TAGS I'm aware of use SLUB.
With eventual availability of Intel LAM, I expect KASAN_SW_TAGS to
become more widely used though, including its SLAB support.
> ---
> mm/kasan/common.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> index 2baf121fb8c5..41779ad109cd 100644
> --- a/mm/kasan/common.c
> +++ b/mm/kasan/common.c
> @@ -298,7 +298,7 @@ static inline u8 assign_tag(struct kmem_cache *cache,
> /* For caches that either have a constructor or SLAB_TYPESAFE_BY_RCU: */
> #ifdef CONFIG_SLAB
> /* For SLAB assign tags based on the object index in the freelist. */
> - return (u8)obj_to_index(cache, virt_to_page(object), (void *)object);
> + return (u8)obj_to_index(cache, virt_to_head_page(object), (void *)object);
> #else
> /*
> * For SLUB assign a random tag during slab creation, otherwise reuse
> --
> 2.32.0
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] kasan: Fix tag for large allocations when using CONFIG_SLAB
2021-10-01 2:41 [PATCH] kasan: Fix tag for large allocations when using CONFIG_SLAB Matthew Wilcox (Oracle)
2021-10-01 10:30 ` Marco Elver
@ 2021-10-01 13:29 ` Andrey Konovalov
2021-10-01 14:05 ` Matthew Wilcox
2021-10-03 16:27 ` Andrey Konovalov
2 siblings, 1 reply; 6+ messages in thread
From: Andrey Konovalov @ 2021-10-01 13:29 UTC (permalink / raw)
To: Matthew Wilcox (Oracle)
Cc: Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov,
Andrew Morton, kasan-dev, Linux Memory Management List, LKML
On Fri, Oct 1, 2021 at 4:42 AM Matthew Wilcox (Oracle)
<willy@infradead.org> wrote:
>
> If an object is allocated on a tail page of a multi-page slab, kasan
> will get the wrong tagbecause page->s_mem is NULL for tail pages.
Interesting. Is this a known property of tail pages? Why does this
happen? I failed to find this exception in the code.
The tag value won't really be "wrong", just unexpected. But if s_mem
is indeed NULL for tail pages, your fix makes sense.
> I'm not quite sure what the user-visible effect of this might be.
Everything should work, as long as tag values are assigned
consistently based on the object address.
>
> Fixes: 7f94ffbc4c6a ("kasan: add hooks implementation for tag-based mode")
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
> mm/kasan/common.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> index 2baf121fb8c5..41779ad109cd 100644
> --- a/mm/kasan/common.c
> +++ b/mm/kasan/common.c
> @@ -298,7 +298,7 @@ static inline u8 assign_tag(struct kmem_cache *cache,
> /* For caches that either have a constructor or SLAB_TYPESAFE_BY_RCU: */
> #ifdef CONFIG_SLAB
> /* For SLAB assign tags based on the object index in the freelist. */
> - return (u8)obj_to_index(cache, virt_to_page(object), (void *)object);
> + return (u8)obj_to_index(cache, virt_to_head_page(object), (void *)object);
> #else
> /*
> * For SLUB assign a random tag during slab creation, otherwise reuse
> --
> 2.32.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] kasan: Fix tag for large allocations when using CONFIG_SLAB
2021-10-01 13:29 ` Andrey Konovalov
@ 2021-10-01 14:05 ` Matthew Wilcox
2021-10-03 16:27 ` Andrey Konovalov
0 siblings, 1 reply; 6+ messages in thread
From: Matthew Wilcox @ 2021-10-01 14:05 UTC (permalink / raw)
To: Andrey Konovalov
Cc: Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov,
Andrew Morton, kasan-dev, Linux Memory Management List, LKML
On Fri, Oct 01, 2021 at 03:29:29PM +0200, Andrey Konovalov wrote:
> On Fri, Oct 1, 2021 at 4:42 AM Matthew Wilcox (Oracle)
> <willy@infradead.org> wrote:
> >
> > If an object is allocated on a tail page of a multi-page slab, kasan
> > will get the wrong tagbecause page->s_mem is NULL for tail pages.
>
> Interesting. Is this a known property of tail pages? Why does this
> happen? I failed to find this exception in the code.
Yes, it's a known property of tail pages. kmem_getpages() calls
__alloc_pages_node() which returns a pointer to the head page.
All the tail pages are initialised to point to the head page.
Then in alloc_slabmgmt(), we set ->s_mem of the head page, but
we never set ->s_mem of the tail pages. Instead, we rely on
people always passing in the head page. I have a patch in the works
to change the type from struct page to struct slab so you can't
make this mistake. That was how I noticed this problem.
> The tag value won't really be "wrong", just unexpected. But if s_mem
> is indeed NULL for tail pages, your fix makes sense.
>
> > I'm not quite sure what the user-visible effect of this might be.
>
> Everything should work, as long as tag values are assigned
> consistently based on the object address.
OK, maybe this doesn't need to be backported then? Actually, why
subtract s_mem in the first place? Can we just avoid that for all
tag calculations?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] kasan: Fix tag for large allocations when using CONFIG_SLAB
2021-10-01 14:05 ` Matthew Wilcox
@ 2021-10-03 16:27 ` Andrey Konovalov
0 siblings, 0 replies; 6+ messages in thread
From: Andrey Konovalov @ 2021-10-03 16:27 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov,
Andrew Morton, kasan-dev, Linux Memory Management List, LKML
On Fri, Oct 1, 2021 at 4:06 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Fri, Oct 01, 2021 at 03:29:29PM +0200, Andrey Konovalov wrote:
> > On Fri, Oct 1, 2021 at 4:42 AM Matthew Wilcox (Oracle)
> > <willy@infradead.org> wrote:
> > >
> > > If an object is allocated on a tail page of a multi-page slab, kasan
> > > will get the wrong tagbecause page->s_mem is NULL for tail pages.
> >
> > Interesting. Is this a known property of tail pages? Why does this
> > happen? I failed to find this exception in the code.
>
> Yes, it's a known property of tail pages. kmem_getpages() calls
> __alloc_pages_node() which returns a pointer to the head page.
> All the tail pages are initialised to point to the head page.
> Then in alloc_slabmgmt(), we set ->s_mem of the head page, but
> we never set ->s_mem of the tail pages. Instead, we rely on
> people always passing in the head page. I have a patch in the works
> to change the type from struct page to struct slab so you can't
> make this mistake. That was how I noticed this problem.
Ah, so it's not "the tail page", it's "a tail page". Meaning any page
but the head page. Got it.
> > The tag value won't really be "wrong", just unexpected. But if s_mem
> > is indeed NULL for tail pages, your fix makes sense.
> >
> > > I'm not quite sure what the user-visible effect of this might be.
> >
> > Everything should work, as long as tag values are assigned
> > consistently based on the object address.
>
> OK, maybe this doesn't need to be backported then? Actually, why
> subtract s_mem in the first place? Can we just avoid that for all
> tag calculations?
We could avoid it. To me, it seems cleaner to assign tags based on the
object index rather than on the absolute address. But either way
should work.
There's no security nor stability impact from this issue, so probably
not so much incentive to backport. But the patch makes sense.
Thanks!
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] kasan: Fix tag for large allocations when using CONFIG_SLAB
2021-10-01 2:41 [PATCH] kasan: Fix tag for large allocations when using CONFIG_SLAB Matthew Wilcox (Oracle)
2021-10-01 10:30 ` Marco Elver
2021-10-01 13:29 ` Andrey Konovalov
@ 2021-10-03 16:27 ` Andrey Konovalov
2 siblings, 0 replies; 6+ messages in thread
From: Andrey Konovalov @ 2021-10-03 16:27 UTC (permalink / raw)
To: Matthew Wilcox (Oracle)
Cc: Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov,
Andrew Morton, kasan-dev, Linux Memory Management List, LKML
On Fri, Oct 1, 2021 at 4:42 AM Matthew Wilcox (Oracle)
<willy@infradead.org> wrote:
>
> If an object is allocated on a tail page of a multi-page slab, kasan
> will get the wrong tag because page->s_mem is NULL for tail pages.
> I'm not quite sure what the user-visible effect of this might be.
>
> Fixes: 7f94ffbc4c6a ("kasan: add hooks implementation for tag-based mode")
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
> mm/kasan/common.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> index 2baf121fb8c5..41779ad109cd 100644
> --- a/mm/kasan/common.c
> +++ b/mm/kasan/common.c
> @@ -298,7 +298,7 @@ static inline u8 assign_tag(struct kmem_cache *cache,
> /* For caches that either have a constructor or SLAB_TYPESAFE_BY_RCU: */
> #ifdef CONFIG_SLAB
> /* For SLAB assign tags based on the object index in the freelist. */
> - return (u8)obj_to_index(cache, virt_to_page(object), (void *)object);
> + return (u8)obj_to_index(cache, virt_to_head_page(object), (void *)object);
> #else
> /*
> * For SLUB assign a random tag during slab creation, otherwise reuse
> --
> 2.32.0
>
Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com>
^ permalink raw reply [flat|nested] 6+ messages in thread