From: Andrey Konovalov <[email protected]>
HW_TAGS KASAN skips zeroing page_alloc allocations backing vmalloc
mappings via __GFP_SKIP_ZERO. Instead, these pages are zeroed via
kasan_unpoison_vmalloc() by passing the KASAN_VMALLOC_INIT flag.
The problem is that __kasan_unpoison_vmalloc() does not zero pages
when either kasan_vmalloc_enabled() or is_vmalloc_or_module_addr() fail.
Thus:
1. Change __vmalloc_node_range() to only set KASAN_VMALLOC_INIT when
__GFP_SKIP_ZERO is set.
2. Change __kasan_unpoison_vmalloc() to always zero pages when the
KASAN_VMALLOC_INIT flag is set.
3. Add WARN_ON() asserts to check that KASAN_VMALLOC_INIT cannot be set
in other early return paths of __kasan_unpoison_vmalloc().
Also clean up the comment in __kasan_unpoison_vmalloc.
Fixes: 23689e91fb22 ("kasan, vmalloc: add vmalloc tagging for HW_TAGS")
Signed-off-by: Andrey Konovalov <[email protected]>
---
mm/kasan/hw_tags.c | 30 ++++++++++++++++++++++--------
mm/vmalloc.c | 10 +++++-----
2 files changed, 27 insertions(+), 13 deletions(-)
diff --git a/mm/kasan/hw_tags.c b/mm/kasan/hw_tags.c
index 9e1b6544bfa8..c0ec01eadf20 100644
--- a/mm/kasan/hw_tags.c
+++ b/mm/kasan/hw_tags.c
@@ -263,21 +263,31 @@ void *__kasan_unpoison_vmalloc(const void *start, unsigned long size,
u8 tag;
unsigned long redzone_start, redzone_size;
- if (!kasan_vmalloc_enabled())
- return (void *)start;
+ if (!kasan_vmalloc_enabled() || !is_vmalloc_or_module_addr(start)) {
+ struct page *page;
+ const void *addr;
+
+ /* Initialize memory if required. */
+
+ if (!(flags & KASAN_VMALLOC_INIT))
+ return (void *)start;
+
+ for (addr = start; addr < start + size; addr += PAGE_SIZE) {
+ page = virt_to_page(addr);
+ clear_highpage_tagged(page);
+ }
- if (!is_vmalloc_or_module_addr(start))
return (void *)start;
+ }
/*
- * Skip unpoisoning and assigning a pointer tag for non-VM_ALLOC
- * mappings as:
+ * Don't tag non-VM_ALLOC mappings, as:
*
* 1. Unlike the software KASAN modes, hardware tag-based KASAN only
* supports tagging physical memory. Therefore, it can only tag a
* single mapping of normal physical pages.
* 2. Hardware tag-based KASAN can only tag memory mapped with special
- * mapping protection bits, see arch_vmalloc_pgprot_modify().
+ * mapping protection bits, see arch_vmap_pgprot_tagged().
* As non-VM_ALLOC mappings can be mapped outside of vmalloc code,
* providing these bits would require tracking all non-VM_ALLOC
* mappers.
@@ -289,15 +299,19 @@ void *__kasan_unpoison_vmalloc(const void *start, unsigned long size,
*
* For non-VM_ALLOC allocations, page_alloc memory is tagged as usual.
*/
- if (!(flags & KASAN_VMALLOC_VM_ALLOC))
+ if (!(flags & KASAN_VMALLOC_VM_ALLOC)) {
+ WARN_ON(flags & KASAN_VMALLOC_INIT);
return (void *)start;
+ }
/*
* Don't tag executable memory.
* The kernel doesn't tolerate having the PC register tagged.
*/
- if (!(flags & KASAN_VMALLOC_PROT_NORMAL))
+ if (!(flags & KASAN_VMALLOC_PROT_NORMAL)) {
+ WARN_ON(flags & KASAN_VMALLOC_INIT);
return (void *)start;
+ }
tag = kasan_random_tag();
start = set_tag(start, tag);
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 07db42455dd4..0adf4aa1514d 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -3168,15 +3168,15 @@ void *__vmalloc_node_range(unsigned long size, unsigned long align,
/*
* Mark the pages as accessible, now that they are mapped.
- * The init condition should match the one in post_alloc_hook()
- * (except for the should_skip_init() check) to make sure that memory
- * is initialized under the same conditions regardless of the enabled
- * KASAN mode.
+ * The condition for setting KASAN_VMALLOC_INIT should complement the
+ * one in post_alloc_hook() with regards to the __GFP_SKIP_ZERO check
+ * to make sure that memory is initialized under the same conditions.
* Tag-based KASAN modes only assign tags to normal non-executable
* allocations, see __kasan_unpoison_vmalloc().
*/
kasan_flags |= KASAN_VMALLOC_VM_ALLOC;
- if (!want_init_on_free() && want_init_on_alloc(gfp_mask))
+ if (!want_init_on_free() && want_init_on_alloc(gfp_mask) &&
+ (gfp_mask & __GFP_SKIP_ZERO))
kasan_flags |= KASAN_VMALLOC_INIT;
/* KASAN_VMALLOC_PROT_NORMAL already set if required. */
area->addr = kasan_unpoison_vmalloc(area->addr, real_size, kasan_flags);
--
2.25.1
On Tue, May 31, 2022 at 05:43PM +0200, [email protected] wrote:
> From: Andrey Konovalov <[email protected]>
>
> HW_TAGS KASAN skips zeroing page_alloc allocations backing vmalloc
> mappings via __GFP_SKIP_ZERO. Instead, these pages are zeroed via
> kasan_unpoison_vmalloc() by passing the KASAN_VMALLOC_INIT flag.
>
> The problem is that __kasan_unpoison_vmalloc() does not zero pages
> when either kasan_vmalloc_enabled() or is_vmalloc_or_module_addr() fail.
>
> Thus:
>
> 1. Change __vmalloc_node_range() to only set KASAN_VMALLOC_INIT when
> __GFP_SKIP_ZERO is set.
>
> 2. Change __kasan_unpoison_vmalloc() to always zero pages when the
> KASAN_VMALLOC_INIT flag is set.
>
> 3. Add WARN_ON() asserts to check that KASAN_VMALLOC_INIT cannot be set
> in other early return paths of __kasan_unpoison_vmalloc().
>
> Also clean up the comment in __kasan_unpoison_vmalloc.
>
> Fixes: 23689e91fb22 ("kasan, vmalloc: add vmalloc tagging for HW_TAGS")
> Signed-off-by: Andrey Konovalov <[email protected]>
> ---
> mm/kasan/hw_tags.c | 30 ++++++++++++++++++++++--------
> mm/vmalloc.c | 10 +++++-----
> 2 files changed, 27 insertions(+), 13 deletions(-)
>
> diff --git a/mm/kasan/hw_tags.c b/mm/kasan/hw_tags.c
> index 9e1b6544bfa8..c0ec01eadf20 100644
> --- a/mm/kasan/hw_tags.c
> +++ b/mm/kasan/hw_tags.c
> @@ -263,21 +263,31 @@ void *__kasan_unpoison_vmalloc(const void *start, unsigned long size,
> u8 tag;
> unsigned long redzone_start, redzone_size;
>
> - if (!kasan_vmalloc_enabled())
> - return (void *)start;
> + if (!kasan_vmalloc_enabled() || !is_vmalloc_or_module_addr(start)) {
> + struct page *page;
> + const void *addr;
> +
> + /* Initialize memory if required. */
> +
This whole block of code looks out-of-place in this function, since it's
not at all related to unpoisoning but a fallback if KASAN-vmalloc is off
but we still want to initialize the memory.
Maybe to ease readability here I'd change it to look like:
diff --git a/mm/kasan/hw_tags.c b/mm/kasan/hw_tags.c
index 11f661a2494b..227c20d09258 100644
--- a/mm/kasan/hw_tags.c
+++ b/mm/kasan/hw_tags.c
@@ -257,6 +257,21 @@ static void unpoison_vmalloc_pages(const void *addr, u8 tag)
}
}
+/*
+ * Explicit initialization of pages if KASAN does not handle VM_ALLOC
+ * allocations.
+ */
+static void init_vmalloc_pages_explicit(const void *start, unsigned long size)
+{
+ const void *addr;
+
+ for (addr = start; addr < start + size; addr += PAGE_SIZE) {
+ struct page *page = virt_to_page(addr);
+
+ clear_highpage_kasan_tagged(page);
+ }
+}
+
void *__kasan_unpoison_vmalloc(const void *start, unsigned long size,
kasan_vmalloc_flags_t flags)
{
@@ -264,19 +279,8 @@ void *__kasan_unpoison_vmalloc(const void *start, unsigned long size,
unsigned long redzone_start, redzone_size;
if (!kasan_vmalloc_enabled() || !is_vmalloc_or_module_addr(start)) {
- struct page *page;
- const void *addr;
-
- /* Initialize memory if required. */
-
- if (!(flags & KASAN_VMALLOC_INIT))
- return (void *)start;
-
- for (addr = start; addr < start + size; addr += PAGE_SIZE) {
- page = virt_to_page(addr);
- clear_highpage_kasan_tagged(page);
- }
-
+ if (flags & KASAN_VMALLOC_INIT)
+ init_vmalloc_pages_explicit(start, size);
return (void *)start;
}
On Wed, Jun 1, 2022 at 2:28 PM Marco Elver <[email protected]> wrote:
>
> On Tue, May 31, 2022 at 05:43PM +0200, [email protected] wrote:
> > From: Andrey Konovalov <[email protected]>
> >
> > HW_TAGS KASAN skips zeroing page_alloc allocations backing vmalloc
> > mappings via __GFP_SKIP_ZERO. Instead, these pages are zeroed via
> > kasan_unpoison_vmalloc() by passing the KASAN_VMALLOC_INIT flag.
> >
> > The problem is that __kasan_unpoison_vmalloc() does not zero pages
> > when either kasan_vmalloc_enabled() or is_vmalloc_or_module_addr() fail.
> >
> > Thus:
> >
> > 1. Change __vmalloc_node_range() to only set KASAN_VMALLOC_INIT when
> > __GFP_SKIP_ZERO is set.
> >
> > 2. Change __kasan_unpoison_vmalloc() to always zero pages when the
> > KASAN_VMALLOC_INIT flag is set.
> >
> > 3. Add WARN_ON() asserts to check that KASAN_VMALLOC_INIT cannot be set
> > in other early return paths of __kasan_unpoison_vmalloc().
> >
> > Also clean up the comment in __kasan_unpoison_vmalloc.
> >
> > Fixes: 23689e91fb22 ("kasan, vmalloc: add vmalloc tagging for HW_TAGS")
> > Signed-off-by: Andrey Konovalov <[email protected]>
> > ---
> > mm/kasan/hw_tags.c | 30 ++++++++++++++++++++++--------
> > mm/vmalloc.c | 10 +++++-----
> > 2 files changed, 27 insertions(+), 13 deletions(-)
> >
> > diff --git a/mm/kasan/hw_tags.c b/mm/kasan/hw_tags.c
> > index 9e1b6544bfa8..c0ec01eadf20 100644
> > --- a/mm/kasan/hw_tags.c
> > +++ b/mm/kasan/hw_tags.c
> > @@ -263,21 +263,31 @@ void *__kasan_unpoison_vmalloc(const void *start, unsigned long size,
> > u8 tag;
> > unsigned long redzone_start, redzone_size;
> >
> > - if (!kasan_vmalloc_enabled())
> > - return (void *)start;
> > + if (!kasan_vmalloc_enabled() || !is_vmalloc_or_module_addr(start)) {
> > + struct page *page;
> > + const void *addr;
> > +
> > + /* Initialize memory if required. */
> > +
>
> This whole block of code looks out-of-place in this function, since it's
> not at all related to unpoisoning but a fallback if KASAN-vmalloc is off
> but we still want to initialize the memory.
>
> Maybe to ease readability here I'd change it to look like:
Sounds good, will do in v2! Thanks!