[PATCH 0/3] kasan: Fix ordering between MTE tag colouring and page->flags

[PATCH 0/3] kasan: Fix ordering between MTE tag colouring and page->flags

[Catalin Marinas]

Hi,

That's more of an RFC to get a discussion started. I plan to eventually
apply the third patch reverting the page_kasan_tag_reset() calls under
arch/arm64 since they don't cover all cases (the race is rare and we
haven't hit anything yet but it's possible).

On a system with MTE and KASAN_HW_TAGS enabled, when a page is allocated
kasan_unpoison_pages() sets a random tag and saves it in page->flags so
that page_to_virt() re-creates the correct tagged pointer. We need to
ensure that the in-memory tags are visible before setting the
page->flags:

P0 (__kasan_unpoison_range):	P1 (access via virt_to_page):
  Wtags=x			  Rflags=x
    |				    |
    | DMB			    | address dependency
    V				    V
  Wflags=x			  Rtags=x

The first patch changes the order of page unpoisoning with the tag
storing in page->flags. page_kasan_tag_set() has the right barriers
through try_cmpxchg().

If such page is mapped in user-space with PROT_MTE, the architecture
code will set the tag to 0 and a subsequent page_to_virt() dereference
will fault. We currently try to fix this by resetting the tag in
page->flags so that it is 0xff (match-all, not faulting). However,
setting the tags and flags can race with another CPU reading the flags
(page_to_virt()) and barriers can't help, e.g.:

P0 (mte_sync_page_tags):        P1 (memcpy from virt_to_page):
                                  Rflags!=0xff
  Wflags=0xff
  DMB (doesn't help)
  Wtags=0
                                  Rtags=0   // fault

Since clearing the flags in the arch code doesn't work, try to do this
at page allocation time by a new flag added to GFP_USER. Could we
instead add __GFP_SKIP_KASAN_UNPOISON rather than a new flag?

Thanks.

Catalin Marinas (3):
  mm: kasan: Ensure the tags are visible before the tag in page->flags
  mm: kasan: Reset the tag on pages intended for user
  arm64: kasan: Revert "arm64: mte: reset the page tag in page->flags"

 arch/arm64/kernel/hibernate.c |  5 -----
 arch/arm64/kernel/mte.c       |  9 ---------
 arch/arm64/mm/copypage.c      |  9 ---------
 arch/arm64/mm/fault.c         |  1 -
 arch/arm64/mm/mteswap.c       |  9 ---------
 include/linux/gfp.h           | 10 +++++++---
 mm/kasan/common.c             |  3 ++-
 mm/page_alloc.c               |  9 ++++++---
 8 files changed, 15 insertions(+), 40 deletions(-)

[PATCH 0/3] kasan: Fix ordering between MTE tag colouring and page->flags

[Catalin Marinas]

Hi,

That's more of an RFC to get a discussion started. I plan to eventually
apply the third patch reverting the page_kasan_tag_reset() calls under
arch/arm64 since they don't cover all cases (the race is rare and we
haven't hit anything yet but it's possible).

On a system with MTE and KASAN_HW_TAGS enabled, when a page is allocated
kasan_unpoison_pages() sets a random tag and saves it in page->flags so
that page_to_virt() re-creates the correct tagged pointer. We need to
ensure that the in-memory tags are visible before setting the
page->flags:

P0 (__kasan_unpoison_range):	P1 (access via virt_to_page):
  Wtags=x			  Rflags=x
    |				    |
    | DMB			    | address dependency
    V				    V
  Wflags=x			  Rtags=x

The first patch changes the order of page unpoisoning with the tag
storing in page->flags. page_kasan_tag_set() has the right barriers
through try_cmpxchg().

If such page is mapped in user-space with PROT_MTE, the architecture
code will set the tag to 0 and a subsequent page_to_virt() dereference
will fault. We currently try to fix this by resetting the tag in
page->flags so that it is 0xff (match-all, not faulting). However,
setting the tags and flags can race with another CPU reading the flags
(page_to_virt()) and barriers can't help, e.g.:

P0 (mte_sync_page_tags):        P1 (memcpy from virt_to_page):
                                  Rflags!=0xff
  Wflags=0xff
  DMB (doesn't help)
  Wtags=0
                                  Rtags=0   // fault

Since clearing the flags in the arch code doesn't work, try to do this
at page allocation time by a new flag added to GFP_USER. Could we
instead add __GFP_SKIP_KASAN_UNPOISON rather than a new flag?

Thanks.

Catalin Marinas (3):
  mm: kasan: Ensure the tags are visible before the tag in page->flags
  mm: kasan: Reset the tag on pages intended for user
  arm64: kasan: Revert "arm64: mte: reset the page tag in page->flags"

 arch/arm64/kernel/hibernate.c |  5 -----
 arch/arm64/kernel/mte.c       |  9 ---------
 arch/arm64/mm/copypage.c      |  9 ---------
 arch/arm64/mm/fault.c         |  1 -
 arch/arm64/mm/mteswap.c       |  9 ---------
 include/linux/gfp.h           | 10 +++++++---
 mm/kasan/common.c             |  3 ++-
 mm/page_alloc.c               |  9 ++++++---
 8 files changed, 15 insertions(+), 40 deletions(-)


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/3] mm: kasan: Ensure the tags are visible before the tag in page->flags

[Catalin Marinas]

__kasan_unpoison_pages() colours the memory with a random tag and stores
it in page->flags in order to re-create the tagged pointer via
page_to_virt() later. When the tag from the page->flags is read, ensure
that the in-memory tags are already visible by re-ordering the
page_kasan_tag_set() after kasan_unpoison(). The former already has
barriers in place through try_cmpxchg(). On the reader side, the order
is ensured by the address dependency between page->flags and the memory
access.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>
Cc: Andrey Konovalov <andreyknvl@gmail.com>
Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
---
 mm/kasan/common.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/kasan/common.c b/mm/kasan/common.c
index d9079ec11f31..f6b8dc4f354b 100644
--- a/mm/kasan/common.c
+++ b/mm/kasan/common.c
@@ -108,9 +108,10 @@ void __kasan_unpoison_pages(struct page *page, unsigned int order, bool init)
 		return;
 
 	tag = kasan_random_tag();
+	kasan_unpoison(set_tag(page_address(page), tag),
+		       PAGE_SIZE << order, init);
 	for (i = 0; i < (1 << order); i++)
 		page_kasan_tag_set(page + i, tag);
-	kasan_unpoison(page_address(page), PAGE_SIZE << order, init);
 }
 
 void __kasan_poison_pages(struct page *page, unsigned int order, bool init)

[PATCH 1/3] mm: kasan: Ensure the tags are visible before the tag in page->flags

[Catalin Marinas]

__kasan_unpoison_pages() colours the memory with a random tag and stores
it in page->flags in order to re-create the tagged pointer via
page_to_virt() later. When the tag from the page->flags is read, ensure
that the in-memory tags are already visible by re-ordering the
page_kasan_tag_set() after kasan_unpoison(). The former already has
barriers in place through try_cmpxchg(). On the reader side, the order
is ensured by the address dependency between page->flags and the memory
access.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>
Cc: Andrey Konovalov <andreyknvl@gmail.com>
Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
---
 mm/kasan/common.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/kasan/common.c b/mm/kasan/common.c
index d9079ec11f31..f6b8dc4f354b 100644
--- a/mm/kasan/common.c
+++ b/mm/kasan/common.c
@@ -108,9 +108,10 @@ void __kasan_unpoison_pages(struct page *page, unsigned int order, bool init)
 		return;
 
 	tag = kasan_random_tag();
+	kasan_unpoison(set_tag(page_address(page), tag),
+		       PAGE_SIZE << order, init);
 	for (i = 0; i < (1 << order); i++)
 		page_kasan_tag_set(page + i, tag);
-	kasan_unpoison(page_address(page), PAGE_SIZE << order, init);
 }
 
 void __kasan_poison_pages(struct page *page, unsigned int order, bool init)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] mm: kasan: Ensure the tags are visible before the tag in page->flags

[Andrey Konovalov]

On Tue, May 17, 2022 at 8:09 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> __kasan_unpoison_pages() colours the memory with a random tag and stores
> it in page->flags in order to re-create the tagged pointer via
> page_to_virt() later. When the tag from the page->flags is read, ensure
> that the in-memory tags are already visible by re-ordering the
> page_kasan_tag_set() after kasan_unpoison(). The former already has
> barriers in place through try_cmpxchg(). On the reader side, the order
> is ensured by the address dependency between page->flags and the memory
> access.
>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>
> Cc: Andrey Konovalov <andreyknvl@gmail.com>
> Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
> ---
>  mm/kasan/common.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> index d9079ec11f31..f6b8dc4f354b 100644
> --- a/mm/kasan/common.c
> +++ b/mm/kasan/common.c
> @@ -108,9 +108,10 @@ void __kasan_unpoison_pages(struct page *page, unsigned int order, bool init)
>                 return;
>
>         tag = kasan_random_tag();
> +       kasan_unpoison(set_tag(page_address(page), tag),
> +                      PAGE_SIZE << order, init);
>         for (i = 0; i < (1 << order); i++)
>                 page_kasan_tag_set(page + i, tag);
> -       kasan_unpoison(page_address(page), PAGE_SIZE << order, init);
>  }
>
>  void __kasan_poison_pages(struct page *page, unsigned int order, bool init)

Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com>

Re: [PATCH 1/3] mm: kasan: Ensure the tags are visible before the tag in page->flags

[Andrey Konovalov]

On Tue, May 17, 2022 at 8:09 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> __kasan_unpoison_pages() colours the memory with a random tag and stores
> it in page->flags in order to re-create the tagged pointer via
> page_to_virt() later. When the tag from the page->flags is read, ensure
> that the in-memory tags are already visible by re-ordering the
> page_kasan_tag_set() after kasan_unpoison(). The former already has
> barriers in place through try_cmpxchg(). On the reader side, the order
> is ensured by the address dependency between page->flags and the memory
> access.
>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>
> Cc: Andrey Konovalov <andreyknvl@gmail.com>
> Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
> ---
>  mm/kasan/common.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> index d9079ec11f31..f6b8dc4f354b 100644
> --- a/mm/kasan/common.c
> +++ b/mm/kasan/common.c
> @@ -108,9 +108,10 @@ void __kasan_unpoison_pages(struct page *page, unsigned int order, bool init)
>                 return;
>
>         tag = kasan_random_tag();
> +       kasan_unpoison(set_tag(page_address(page), tag),
> +                      PAGE_SIZE << order, init);
>         for (i = 0; i < (1 << order); i++)
>                 page_kasan_tag_set(page + i, tag);
> -       kasan_unpoison(page_address(page), PAGE_SIZE << order, init);
>  }
>
>  void __kasan_poison_pages(struct page *page, unsigned int order, bool init)

Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/3] mm: kasan: Reset the tag on pages intended for user

[Catalin Marinas]

On allocation kasan colours a page with a random tag and stores such tag
in page->flags so that a subsequent page_to_virt() reconstructs the
correct tagged pointer. However, when such page is mapped in user-space
with PROT_MTE, the kernel's initial tag is overridden. Ensure that such
pages have the tag reset (match-all) at allocation time since any late
clearing of the tag is racy with other page_to_virt() dereferencing.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>
Cc: Andrey Konovalov <andreyknvl@gmail.com>
Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
---
 include/linux/gfp.h | 10 +++++++---
 mm/page_alloc.c     |  9 ++++++---
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index 3e3d36fc2109..88b1d4fe4dcb 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -58,13 +58,15 @@ struct vm_area_struct;
 #define ___GFP_SKIP_ZERO		0x1000000u
 #define ___GFP_SKIP_KASAN_UNPOISON	0x2000000u
 #define ___GFP_SKIP_KASAN_POISON	0x4000000u
+#define ___GFP_PAGE_KASAN_TAG_RESET	0x8000000u
 #else
 #define ___GFP_SKIP_ZERO		0
 #define ___GFP_SKIP_KASAN_UNPOISON	0
 #define ___GFP_SKIP_KASAN_POISON	0
+#define ___GFP_PAGE_KASAN_TAG_RESET	0
 #endif
 #ifdef CONFIG_LOCKDEP
-#define ___GFP_NOLOCKDEP	0x8000000u
+#define ___GFP_NOLOCKDEP	0x10000000u
 #else
 #define ___GFP_NOLOCKDEP	0
 #endif
@@ -259,12 +261,13 @@ struct vm_area_struct;
 #define __GFP_SKIP_ZERO ((__force gfp_t)___GFP_SKIP_ZERO)
 #define __GFP_SKIP_KASAN_UNPOISON ((__force gfp_t)___GFP_SKIP_KASAN_UNPOISON)
 #define __GFP_SKIP_KASAN_POISON   ((__force gfp_t)___GFP_SKIP_KASAN_POISON)
+#define __GFP_PAGE_KASAN_TAG_RESET ((__force gfp_t)___GFP_PAGE_KASAN_TAG_RESET)
 
 /* Disable lockdep for GFP context tracking */
 #define __GFP_NOLOCKDEP ((__force gfp_t)___GFP_NOLOCKDEP)
 
 /* Room for N __GFP_FOO bits */
-#define __GFP_BITS_SHIFT (27 + IS_ENABLED(CONFIG_LOCKDEP))
+#define __GFP_BITS_SHIFT (28 + IS_ENABLED(CONFIG_LOCKDEP))
 #define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1))
 
 /**
@@ -343,7 +346,8 @@ struct vm_area_struct;
 #define GFP_NOWAIT	(__GFP_KSWAPD_RECLAIM)
 #define GFP_NOIO	(__GFP_RECLAIM)
 #define GFP_NOFS	(__GFP_RECLAIM | __GFP_IO)
-#define GFP_USER	(__GFP_RECLAIM | __GFP_IO | __GFP_FS | __GFP_HARDWALL)
+#define GFP_USER	(__GFP_RECLAIM | __GFP_IO | __GFP_FS | __GFP_HARDWALL | \
+			 __GFP_PAGE_KASAN_TAG_RESET)
 #define GFP_DMA		__GFP_DMA
 #define GFP_DMA32	__GFP_DMA32
 #define GFP_HIGHUSER	(GFP_USER | __GFP_HIGHMEM)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 0e42038382c1..f9018a84f4e3 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2382,6 +2382,7 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
 	bool init = !want_init_on_free() && want_init_on_alloc(gfp_flags) &&
 			!should_skip_init(gfp_flags);
 	bool init_tags = init && (gfp_flags & __GFP_ZEROTAGS);
+	int i;
 
 	set_page_private(page, 0);
 	set_page_refcounted(page);
@@ -2407,8 +2408,6 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
 	 * should be initialized as well).
 	 */
 	if (init_tags) {
-		int i;
-
 		/* Initialize both memory and tags. */
 		for (i = 0; i != 1 << order; ++i)
 			tag_clear_highpage(page + i);
@@ -2430,7 +2429,11 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
 	/* Propagate __GFP_SKIP_KASAN_POISON to page flags. */
 	if (kasan_hw_tags_enabled() && (gfp_flags & __GFP_SKIP_KASAN_POISON))
 		SetPageSkipKASanPoison(page);
-
+	/* if match-all page address required, reset the tag */
+	if (gfp_flags & __GFP_PAGE_KASAN_TAG_RESET) {
+		for (i = 0; i != 1 << order; ++i)
+			page_kasan_tag_reset(page + i);
+	};
 	set_page_owner(page, order, gfp_flags);
 	page_table_check_alloc(page, order);
 }

[PATCH 2/3] mm: kasan: Reset the tag on pages intended for user

[Catalin Marinas]

On allocation kasan colours a page with a random tag and stores such tag
in page->flags so that a subsequent page_to_virt() reconstructs the
correct tagged pointer. However, when such page is mapped in user-space
with PROT_MTE, the kernel's initial tag is overridden. Ensure that such
pages have the tag reset (match-all) at allocation time since any late
clearing of the tag is racy with other page_to_virt() dereferencing.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>
Cc: Andrey Konovalov <andreyknvl@gmail.com>
Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
---
 include/linux/gfp.h | 10 +++++++---
 mm/page_alloc.c     |  9 ++++++---
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index 3e3d36fc2109..88b1d4fe4dcb 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -58,13 +58,15 @@ struct vm_area_struct;
 #define ___GFP_SKIP_ZERO		0x1000000u
 #define ___GFP_SKIP_KASAN_UNPOISON	0x2000000u
 #define ___GFP_SKIP_KASAN_POISON	0x4000000u
+#define ___GFP_PAGE_KASAN_TAG_RESET	0x8000000u
 #else
 #define ___GFP_SKIP_ZERO		0
 #define ___GFP_SKIP_KASAN_UNPOISON	0
 #define ___GFP_SKIP_KASAN_POISON	0
+#define ___GFP_PAGE_KASAN_TAG_RESET	0
 #endif
 #ifdef CONFIG_LOCKDEP
-#define ___GFP_NOLOCKDEP	0x8000000u
+#define ___GFP_NOLOCKDEP	0x10000000u
 #else
 #define ___GFP_NOLOCKDEP	0
 #endif
@@ -259,12 +261,13 @@ struct vm_area_struct;
 #define __GFP_SKIP_ZERO ((__force gfp_t)___GFP_SKIP_ZERO)
 #define __GFP_SKIP_KASAN_UNPOISON ((__force gfp_t)___GFP_SKIP_KASAN_UNPOISON)
 #define __GFP_SKIP_KASAN_POISON   ((__force gfp_t)___GFP_SKIP_KASAN_POISON)
+#define __GFP_PAGE_KASAN_TAG_RESET ((__force gfp_t)___GFP_PAGE_KASAN_TAG_RESET)
 
 /* Disable lockdep for GFP context tracking */
 #define __GFP_NOLOCKDEP ((__force gfp_t)___GFP_NOLOCKDEP)
 
 /* Room for N __GFP_FOO bits */
-#define __GFP_BITS_SHIFT (27 + IS_ENABLED(CONFIG_LOCKDEP))
+#define __GFP_BITS_SHIFT (28 + IS_ENABLED(CONFIG_LOCKDEP))
 #define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1))
 
 /**
@@ -343,7 +346,8 @@ struct vm_area_struct;
 #define GFP_NOWAIT	(__GFP_KSWAPD_RECLAIM)
 #define GFP_NOIO	(__GFP_RECLAIM)
 #define GFP_NOFS	(__GFP_RECLAIM | __GFP_IO)
-#define GFP_USER	(__GFP_RECLAIM | __GFP_IO | __GFP_FS | __GFP_HARDWALL)
+#define GFP_USER	(__GFP_RECLAIM | __GFP_IO | __GFP_FS | __GFP_HARDWALL | \
+			 __GFP_PAGE_KASAN_TAG_RESET)
 #define GFP_DMA		__GFP_DMA
 #define GFP_DMA32	__GFP_DMA32
 #define GFP_HIGHUSER	(GFP_USER | __GFP_HIGHMEM)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 0e42038382c1..f9018a84f4e3 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2382,6 +2382,7 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
 	bool init = !want_init_on_free() && want_init_on_alloc(gfp_flags) &&
 			!should_skip_init(gfp_flags);
 	bool init_tags = init && (gfp_flags & __GFP_ZEROTAGS);
+	int i;
 
 	set_page_private(page, 0);
 	set_page_refcounted(page);
@@ -2407,8 +2408,6 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
 	 * should be initialized as well).
 	 */
 	if (init_tags) {
-		int i;
-
 		/* Initialize both memory and tags. */
 		for (i = 0; i != 1 << order; ++i)
 			tag_clear_highpage(page + i);
@@ -2430,7 +2429,11 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
 	/* Propagate __GFP_SKIP_KASAN_POISON to page flags. */
 	if (kasan_hw_tags_enabled() && (gfp_flags & __GFP_SKIP_KASAN_POISON))
 		SetPageSkipKASanPoison(page);
-
+	/* if match-all page address required, reset the tag */
+	if (gfp_flags & __GFP_PAGE_KASAN_TAG_RESET) {
+		for (i = 0; i != 1 << order; ++i)
+			page_kasan_tag_reset(page + i);
+	};
 	set_page_owner(page, order, gfp_flags);
 	page_table_check_alloc(page, order);
 }

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] mm: kasan: Reset the tag on pages intended for user

[Andrey Konovalov]

On Tue, May 17, 2022 at 8:09 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On allocation kasan colours a page with a random tag and stores such tag
> in page->flags so that a subsequent page_to_virt() reconstructs the
> correct tagged pointer. However, when such page is mapped in user-space
> with PROT_MTE, the kernel's initial tag is overridden. Ensure that such
> pages have the tag reset (match-all) at allocation time since any late
> clearing of the tag is racy with other page_to_virt() dereferencing.
>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>
> Cc: Andrey Konovalov <andreyknvl@gmail.com>
> Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
> ---
>  include/linux/gfp.h | 10 +++++++---
>  mm/page_alloc.c     |  9 ++++++---
>  2 files changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/gfp.h b/include/linux/gfp.h
> index 3e3d36fc2109..88b1d4fe4dcb 100644
> --- a/include/linux/gfp.h
> +++ b/include/linux/gfp.h
> @@ -58,13 +58,15 @@ struct vm_area_struct;
>  #define ___GFP_SKIP_ZERO               0x1000000u
>  #define ___GFP_SKIP_KASAN_UNPOISON     0x2000000u
>  #define ___GFP_SKIP_KASAN_POISON       0x4000000u
> +#define ___GFP_PAGE_KASAN_TAG_RESET    0x8000000u

Let's name it ___GFP_RESET_KASAN_PAGE_TAG to be consistent with the rest.

Also, please add a comment above that explains the new flag's purpose.

>  #else
>  #define ___GFP_SKIP_ZERO               0
>  #define ___GFP_SKIP_KASAN_UNPOISON     0
>  #define ___GFP_SKIP_KASAN_POISON       0
> +#define ___GFP_PAGE_KASAN_TAG_RESET    0
>  #endif
>  #ifdef CONFIG_LOCKDEP
> -#define ___GFP_NOLOCKDEP       0x8000000u
> +#define ___GFP_NOLOCKDEP       0x10000000u
>  #else
>  #define ___GFP_NOLOCKDEP       0
>  #endif
> @@ -259,12 +261,13 @@ struct vm_area_struct;
>  #define __GFP_SKIP_ZERO ((__force gfp_t)___GFP_SKIP_ZERO)
>  #define __GFP_SKIP_KASAN_UNPOISON ((__force gfp_t)___GFP_SKIP_KASAN_UNPOISON)
>  #define __GFP_SKIP_KASAN_POISON   ((__force gfp_t)___GFP_SKIP_KASAN_POISON)
> +#define __GFP_PAGE_KASAN_TAG_RESET ((__force gfp_t)___GFP_PAGE_KASAN_TAG_RESET)
>
>  /* Disable lockdep for GFP context tracking */
>  #define __GFP_NOLOCKDEP ((__force gfp_t)___GFP_NOLOCKDEP)
>
>  /* Room for N __GFP_FOO bits */
> -#define __GFP_BITS_SHIFT (27 + IS_ENABLED(CONFIG_LOCKDEP))
> +#define __GFP_BITS_SHIFT (28 + IS_ENABLED(CONFIG_LOCKDEP))
>  #define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1))
>
>  /**
> @@ -343,7 +346,8 @@ struct vm_area_struct;
>  #define GFP_NOWAIT     (__GFP_KSWAPD_RECLAIM)
>  #define GFP_NOIO       (__GFP_RECLAIM)
>  #define GFP_NOFS       (__GFP_RECLAIM | __GFP_IO)
> -#define GFP_USER       (__GFP_RECLAIM | __GFP_IO | __GFP_FS | __GFP_HARDWALL)
> +#define GFP_USER       (__GFP_RECLAIM | __GFP_IO | __GFP_FS | __GFP_HARDWALL | \
> +                        __GFP_PAGE_KASAN_TAG_RESET)

I guess we can also add both ___GFP_SKIP_KASAN_UNPOISON and
___GFP_SKIP_KASAN_POISON here then? Since we don't care about tags.

Or maybe we can add all three flags to GFP_HIGHUSER_MOVABLE instead?

>  #define GFP_DMA                __GFP_DMA
>  #define GFP_DMA32      __GFP_DMA32
>  #define GFP_HIGHUSER   (GFP_USER | __GFP_HIGHMEM)

In case we add __GFP_SKIP_KASAN_POISON to GFP_USER, we should drop it
from GFP_HIGHUSER_MOVABLE.

> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 0e42038382c1..f9018a84f4e3 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2382,6 +2382,7 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
>         bool init = !want_init_on_free() && want_init_on_alloc(gfp_flags) &&
>                         !should_skip_init(gfp_flags);
>         bool init_tags = init && (gfp_flags & __GFP_ZEROTAGS);
> +       int i;
>
>         set_page_private(page, 0);
>         set_page_refcounted(page);
> @@ -2407,8 +2408,6 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
>          * should be initialized as well).
>          */
>         if (init_tags) {
> -               int i;
> -
>                 /* Initialize both memory and tags. */
>                 for (i = 0; i != 1 << order; ++i)
>                         tag_clear_highpage(page + i);
> @@ -2430,7 +2429,11 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
>         /* Propagate __GFP_SKIP_KASAN_POISON to page flags. */
>         if (kasan_hw_tags_enabled() && (gfp_flags & __GFP_SKIP_KASAN_POISON))
>                 SetPageSkipKASanPoison(page);
> -
> +       /* if match-all page address required, reset the tag */

Please match the style of other comments: capitalize the first letter
and add a dot at the end.

I would also simply say: "Reset page tags if required."

> +       if (gfp_flags & __GFP_PAGE_KASAN_TAG_RESET) {
> +               for (i = 0; i != 1 << order; ++i)
> +                       page_kasan_tag_reset(page + i);
> +       };

I would add an empty line here.



>         set_page_owner(page, order, gfp_flags);
>         page_table_check_alloc(page, order);
>  }

Re: [PATCH 2/3] mm: kasan: Reset the tag on pages intended for user

[Andrey Konovalov]

On Tue, May 17, 2022 at 8:09 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On allocation kasan colours a page with a random tag and stores such tag
> in page->flags so that a subsequent page_to_virt() reconstructs the
> correct tagged pointer. However, when such page is mapped in user-space
> with PROT_MTE, the kernel's initial tag is overridden. Ensure that such
> pages have the tag reset (match-all) at allocation time since any late
> clearing of the tag is racy with other page_to_virt() dereferencing.
>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com>
> Cc: Andrey Konovalov <andreyknvl@gmail.com>
> Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
> ---
>  include/linux/gfp.h | 10 +++++++---
>  mm/page_alloc.c     |  9 ++++++---
>  2 files changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/gfp.h b/include/linux/gfp.h
> index 3e3d36fc2109..88b1d4fe4dcb 100644
> --- a/include/linux/gfp.h
> +++ b/include/linux/gfp.h
> @@ -58,13 +58,15 @@ struct vm_area_struct;
>  #define ___GFP_SKIP_ZERO               0x1000000u
>  #define ___GFP_SKIP_KASAN_UNPOISON     0x2000000u
>  #define ___GFP_SKIP_KASAN_POISON       0x4000000u
> +#define ___GFP_PAGE_KASAN_TAG_RESET    0x8000000u

Let's name it ___GFP_RESET_KASAN_PAGE_TAG to be consistent with the rest.

Also, please add a comment above that explains the new flag's purpose.

>  #else
>  #define ___GFP_SKIP_ZERO               0
>  #define ___GFP_SKIP_KASAN_UNPOISON     0
>  #define ___GFP_SKIP_KASAN_POISON       0
> +#define ___GFP_PAGE_KASAN_TAG_RESET    0
>  #endif
>  #ifdef CONFIG_LOCKDEP
> -#define ___GFP_NOLOCKDEP       0x8000000u
> +#define ___GFP_NOLOCKDEP       0x10000000u
>  #else
>  #define ___GFP_NOLOCKDEP       0
>  #endif
> @@ -259,12 +261,13 @@ struct vm_area_struct;
>  #define __GFP_SKIP_ZERO ((__force gfp_t)___GFP_SKIP_ZERO)
>  #define __GFP_SKIP_KASAN_UNPOISON ((__force gfp_t)___GFP_SKIP_KASAN_UNPOISON)
>  #define __GFP_SKIP_KASAN_POISON   ((__force gfp_t)___GFP_SKIP_KASAN_POISON)
> +#define __GFP_PAGE_KASAN_TAG_RESET ((__force gfp_t)___GFP_PAGE_KASAN_TAG_RESET)
>
>  /* Disable lockdep for GFP context tracking */
>  #define __GFP_NOLOCKDEP ((__force gfp_t)___GFP_NOLOCKDEP)
>
>  /* Room for N __GFP_FOO bits */
> -#define __GFP_BITS_SHIFT (27 + IS_ENABLED(CONFIG_LOCKDEP))
> +#define __GFP_BITS_SHIFT (28 + IS_ENABLED(CONFIG_LOCKDEP))
>  #define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1))
>
>  /**
> @@ -343,7 +346,8 @@ struct vm_area_struct;
>  #define GFP_NOWAIT     (__GFP_KSWAPD_RECLAIM)
>  #define GFP_NOIO       (__GFP_RECLAIM)
>  #define GFP_NOFS       (__GFP_RECLAIM | __GFP_IO)
> -#define GFP_USER       (__GFP_RECLAIM | __GFP_IO | __GFP_FS | __GFP_HARDWALL)
> +#define GFP_USER       (__GFP_RECLAIM | __GFP_IO | __GFP_FS | __GFP_HARDWALL | \
> +                        __GFP_PAGE_KASAN_TAG_RESET)

I guess we can also add both ___GFP_SKIP_KASAN_UNPOISON and
___GFP_SKIP_KASAN_POISON here then? Since we don't care about tags.

Or maybe we can add all three flags to GFP_HIGHUSER_MOVABLE instead?

>  #define GFP_DMA                __GFP_DMA
>  #define GFP_DMA32      __GFP_DMA32
>  #define GFP_HIGHUSER   (GFP_USER | __GFP_HIGHMEM)

In case we add __GFP_SKIP_KASAN_POISON to GFP_USER, we should drop it
from GFP_HIGHUSER_MOVABLE.

> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 0e42038382c1..f9018a84f4e3 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2382,6 +2382,7 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
>         bool init = !want_init_on_free() && want_init_on_alloc(gfp_flags) &&
>                         !should_skip_init(gfp_flags);
>         bool init_tags = init && (gfp_flags & __GFP_ZEROTAGS);
> +       int i;
>
>         set_page_private(page, 0);
>         set_page_refcounted(page);
> @@ -2407,8 +2408,6 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
>          * should be initialized as well).
>          */
>         if (init_tags) {
> -               int i;
> -
>                 /* Initialize both memory and tags. */
>                 for (i = 0; i != 1 << order; ++i)
>                         tag_clear_highpage(page + i);
> @@ -2430,7 +2429,11 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
>         /* Propagate __GFP_SKIP_KASAN_POISON to page flags. */
>         if (kasan_hw_tags_enabled() && (gfp_flags & __GFP_SKIP_KASAN_POISON))
>                 SetPageSkipKASanPoison(page);
> -
> +       /* if match-all page address required, reset the tag */

Please match the style of other comments: capitalize the first letter
and add a dot at the end.

I would also simply say: "Reset page tags if required."

> +       if (gfp_flags & __GFP_PAGE_KASAN_TAG_RESET) {
> +               for (i = 0; i != 1 << order; ++i)
> +                       page_kasan_tag_reset(page + i);
> +       };

I would add an empty line here.



>         set_page_owner(page, order, gfp_flags);
>         page_table_check_alloc(page, order);
>  }

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/3] arm64: kasan: Revert "arm64: mte: reset the page tag in page->flags"

[Catalin Marinas]

This reverts commit e5b8d9218951e59df986f627ec93569a0d22149b.

On a system with MTE and KASAN_HW_TAGS enabled, when a page is allocated
kasan_unpoison_pages() sets a random tag and saves it in page->flags.
page_to_virt() re-creates the correct tagged pointer.

If such page is mapped in user-space with PROT_MTE, the architecture
code will set the tag to 0 and a subsequent page_to_virt() dereference
will fault. The reverted commit aimed to fix this by resetting the tag
in page->flags so that it is 0xff (match-all, not faulting). However,
setting the tags and flags can race with another CPU reading the flags
(page_to_virt()) and barriers can't help:

P0 (mte_sync_page_tags):	P1 (memcpy from virt_to_page):
				  Rflags!=0xff
  Wflags=0xff
  DMB (doesn't help)
  Wtags=0
				  Rtags=0   // fault

Since clearing the flags in the arch code doesn't help, revert the patch
altogether. In addition, remove the page_kasan_tag_reset() call in
tag_clear_highpage() since the core kasan code should take care of
resetting the page tag.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
Cc: Andrey Konovalov <andreyknvl@gmail.com>
Cc: Peter Collingbourne <pcc@google.com>
---
 arch/arm64/kernel/hibernate.c | 5 -----
 arch/arm64/kernel/mte.c       | 9 ---------
 arch/arm64/mm/copypage.c      | 9 ---------
 arch/arm64/mm/fault.c         | 1 -
 arch/arm64/mm/mteswap.c       | 9 ---------
 5 files changed, 33 deletions(-)

diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c
index 6328308be272..7754ef328657 100644
--- a/arch/arm64/kernel/hibernate.c
+++ b/arch/arm64/kernel/hibernate.c
@@ -300,11 +300,6 @@ static void swsusp_mte_restore_tags(void)
 		unsigned long pfn = xa_state.xa_index;
 		struct page *page = pfn_to_online_page(pfn);
 
-		/*
-		 * It is not required to invoke page_kasan_tag_reset(page)
-		 * at this point since the tags stored in page->flags are
-		 * already restored.
-		 */
 		mte_restore_page_tags(page_address(page), tags);
 
 		mte_free_tag_storage(tags);
diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
index 78b3e0f8e997..90994aca54f3 100644
--- a/arch/arm64/kernel/mte.c
+++ b/arch/arm64/kernel/mte.c
@@ -47,15 +47,6 @@ static void mte_sync_page_tags(struct page *page, pte_t old_pte,
 	if (!pte_is_tagged)
 		return;
 
-	page_kasan_tag_reset(page);
-	/*
-	 * We need smp_wmb() in between setting the flags and clearing the
-	 * tags because if another thread reads page->flags and builds a
-	 * tagged address out of it, there is an actual dependency to the
-	 * memory access, but on the current thread we do not guarantee that
-	 * the new page->flags are visible before the tags were updated.
-	 */
-	smp_wmb();
 	mte_clear_page_tags(page_address(page));
 }
 
diff --git a/arch/arm64/mm/copypage.c b/arch/arm64/mm/copypage.c
index b5447e53cd73..70a71f38b6a9 100644
--- a/arch/arm64/mm/copypage.c
+++ b/arch/arm64/mm/copypage.c
@@ -23,15 +23,6 @@ void copy_highpage(struct page *to, struct page *from)
 
 	if (system_supports_mte() && test_bit(PG_mte_tagged, &from->flags)) {
 		set_bit(PG_mte_tagged, &to->flags);
-		page_kasan_tag_reset(to);
-		/*
-		 * We need smp_wmb() in between setting the flags and clearing the
-		 * tags because if another thread reads page->flags and builds a
-		 * tagged address out of it, there is an actual dependency to the
-		 * memory access, but on the current thread we do not guarantee that
-		 * the new page->flags are visible before the tags were updated.
-		 */
-		smp_wmb();
 		mte_copy_page_tags(kto, kfrom);
 	}
 }
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 77341b160aca..f2f21cd6d43f 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -926,6 +926,5 @@ struct page *alloc_zeroed_user_highpage_movable(struct vm_area_struct *vma,
 void tag_clear_highpage(struct page *page)
 {
 	mte_zero_clear_page_tags(page_address(page));
-	page_kasan_tag_reset(page);
 	set_bit(PG_mte_tagged, &page->flags);
 }
diff --git a/arch/arm64/mm/mteswap.c b/arch/arm64/mm/mteswap.c
index a9e50e930484..4334dec93bd4 100644
--- a/arch/arm64/mm/mteswap.c
+++ b/arch/arm64/mm/mteswap.c
@@ -53,15 +53,6 @@ bool mte_restore_tags(swp_entry_t entry, struct page *page)
 	if (!tags)
 		return false;
 
-	page_kasan_tag_reset(page);
-	/*
-	 * We need smp_wmb() in between setting the flags and clearing the
-	 * tags because if another thread reads page->flags and builds a
-	 * tagged address out of it, there is an actual dependency to the
-	 * memory access, but on the current thread we do not guarantee that
-	 * the new page->flags are visible before the tags were updated.
-	 */
-	smp_wmb();
 	mte_restore_page_tags(page_address(page), tags);
 
 	return true;

[PATCH 3/3] arm64: kasan: Revert "arm64: mte: reset the page tag in page->flags"

[Catalin Marinas]

This reverts commit e5b8d9218951e59df986f627ec93569a0d22149b.

On a system with MTE and KASAN_HW_TAGS enabled, when a page is allocated
kasan_unpoison_pages() sets a random tag and saves it in page->flags.
page_to_virt() re-creates the correct tagged pointer.

If such page is mapped in user-space with PROT_MTE, the architecture
code will set the tag to 0 and a subsequent page_to_virt() dereference
will fault. The reverted commit aimed to fix this by resetting the tag
in page->flags so that it is 0xff (match-all, not faulting). However,
setting the tags and flags can race with another CPU reading the flags
(page_to_virt()) and barriers can't help:

P0 (mte_sync_page_tags):	P1 (memcpy from virt_to_page):
				  Rflags!=0xff
  Wflags=0xff
  DMB (doesn't help)
  Wtags=0
				  Rtags=0   // fault

Since clearing the flags in the arch code doesn't help, revert the patch
altogether. In addition, remove the page_kasan_tag_reset() call in
tag_clear_highpage() since the core kasan code should take care of
resetting the page tag.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
Cc: Andrey Konovalov <andreyknvl@gmail.com>
Cc: Peter Collingbourne <pcc@google.com>
---
 arch/arm64/kernel/hibernate.c | 5 -----
 arch/arm64/kernel/mte.c       | 9 ---------
 arch/arm64/mm/copypage.c      | 9 ---------
 arch/arm64/mm/fault.c         | 1 -
 arch/arm64/mm/mteswap.c       | 9 ---------
 5 files changed, 33 deletions(-)

diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c
index 6328308be272..7754ef328657 100644
--- a/arch/arm64/kernel/hibernate.c
+++ b/arch/arm64/kernel/hibernate.c
@@ -300,11 +300,6 @@ static void swsusp_mte_restore_tags(void)
 		unsigned long pfn = xa_state.xa_index;
 		struct page *page = pfn_to_online_page(pfn);
 
-		/*
-		 * It is not required to invoke page_kasan_tag_reset(page)
-		 * at this point since the tags stored in page->flags are
-		 * already restored.
-		 */
 		mte_restore_page_tags(page_address(page), tags);
 
 		mte_free_tag_storage(tags);
diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
index 78b3e0f8e997..90994aca54f3 100644
--- a/arch/arm64/kernel/mte.c
+++ b/arch/arm64/kernel/mte.c
@@ -47,15 +47,6 @@ static void mte_sync_page_tags(struct page *page, pte_t old_pte,
 	if (!pte_is_tagged)
 		return;
 
-	page_kasan_tag_reset(page);
-	/*
-	 * We need smp_wmb() in between setting the flags and clearing the
-	 * tags because if another thread reads page->flags and builds a
-	 * tagged address out of it, there is an actual dependency to the
-	 * memory access, but on the current thread we do not guarantee that
-	 * the new page->flags are visible before the tags were updated.
-	 */
-	smp_wmb();
 	mte_clear_page_tags(page_address(page));
 }
 
diff --git a/arch/arm64/mm/copypage.c b/arch/arm64/mm/copypage.c
index b5447e53cd73..70a71f38b6a9 100644
--- a/arch/arm64/mm/copypage.c
+++ b/arch/arm64/mm/copypage.c
@@ -23,15 +23,6 @@ void copy_highpage(struct page *to, struct page *from)
 
 	if (system_supports_mte() && test_bit(PG_mte_tagged, &from->flags)) {
 		set_bit(PG_mte_tagged, &to->flags);
-		page_kasan_tag_reset(to);
-		/*
-		 * We need smp_wmb() in between setting the flags and clearing the
-		 * tags because if another thread reads page->flags and builds a
-		 * tagged address out of it, there is an actual dependency to the
-		 * memory access, but on the current thread we do not guarantee that
-		 * the new page->flags are visible before the tags were updated.
-		 */
-		smp_wmb();
 		mte_copy_page_tags(kto, kfrom);
 	}
 }
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 77341b160aca..f2f21cd6d43f 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -926,6 +926,5 @@ struct page *alloc_zeroed_user_highpage_movable(struct vm_area_struct *vma,
 void tag_clear_highpage(struct page *page)
 {
 	mte_zero_clear_page_tags(page_address(page));
-	page_kasan_tag_reset(page);
 	set_bit(PG_mte_tagged, &page->flags);
 }
diff --git a/arch/arm64/mm/mteswap.c b/arch/arm64/mm/mteswap.c
index a9e50e930484..4334dec93bd4 100644
--- a/arch/arm64/mm/mteswap.c
+++ b/arch/arm64/mm/mteswap.c
@@ -53,15 +53,6 @@ bool mte_restore_tags(swp_entry_t entry, struct page *page)
 	if (!tags)
 		return false;
 
-	page_kasan_tag_reset(page);
-	/*
-	 * We need smp_wmb() in between setting the flags and clearing the
-	 * tags because if another thread reads page->flags and builds a
-	 * tagged address out of it, there is an actual dependency to the
-	 * memory access, but on the current thread we do not guarantee that
-	 * the new page->flags are visible before the tags were updated.
-	 */
-	smp_wmb();
 	mte_restore_page_tags(page_address(page), tags);
 
 	return true;

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] arm64: kasan: Revert "arm64: mte: reset the page tag in page->flags"

[Andrey Konovalov]

On Tue, May 17, 2022 at 8:09 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> This reverts commit e5b8d9218951e59df986f627ec93569a0d22149b.
>
> On a system with MTE and KASAN_HW_TAGS enabled, when a page is allocated
> kasan_unpoison_pages() sets a random tag and saves it in page->flags.
> page_to_virt() re-creates the correct tagged pointer.
>
> If such page is mapped in user-space with PROT_MTE, the architecture
> code will set the tag to 0 and a subsequent page_to_virt() dereference
> will fault. The reverted commit aimed to fix this by resetting the tag
> in page->flags so that it is 0xff (match-all, not faulting). However,
> setting the tags and flags can race with another CPU reading the flags
> (page_to_virt()) and barriers can't help:
>
> P0 (mte_sync_page_tags):        P1 (memcpy from virt_to_page):
>                                   Rflags!=0xff
>   Wflags=0xff
>   DMB (doesn't help)
>   Wtags=0
>                                   Rtags=0   // fault
>
> Since clearing the flags in the arch code doesn't help, revert the patch
> altogether. In addition, remove the page_kasan_tag_reset() call in
> tag_clear_highpage() since the core kasan code should take care of
> resetting the page tag.
>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
> Cc: Andrey Konovalov <andreyknvl@gmail.com>
> Cc: Peter Collingbourne <pcc@google.com>
> ---
>  arch/arm64/kernel/hibernate.c | 5 -----
>  arch/arm64/kernel/mte.c       | 9 ---------
>  arch/arm64/mm/copypage.c      | 9 ---------
>  arch/arm64/mm/fault.c         | 1 -
>  arch/arm64/mm/mteswap.c       | 9 ---------
>  5 files changed, 33 deletions(-)
>
> diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c
> index 6328308be272..7754ef328657 100644
> --- a/arch/arm64/kernel/hibernate.c
> +++ b/arch/arm64/kernel/hibernate.c
> @@ -300,11 +300,6 @@ static void swsusp_mte_restore_tags(void)
>                 unsigned long pfn = xa_state.xa_index;
>                 struct page *page = pfn_to_online_page(pfn);
>
> -               /*
> -                * It is not required to invoke page_kasan_tag_reset(page)
> -                * at this point since the tags stored in page->flags are
> -                * already restored.
> -                */
>                 mte_restore_page_tags(page_address(page), tags);
>
>                 mte_free_tag_storage(tags);
> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> index 78b3e0f8e997..90994aca54f3 100644
> --- a/arch/arm64/kernel/mte.c
> +++ b/arch/arm64/kernel/mte.c
> @@ -47,15 +47,6 @@ static void mte_sync_page_tags(struct page *page, pte_t old_pte,
>         if (!pte_is_tagged)
>                 return;
>
> -       page_kasan_tag_reset(page);
> -       /*
> -        * We need smp_wmb() in between setting the flags and clearing the
> -        * tags because if another thread reads page->flags and builds a
> -        * tagged address out of it, there is an actual dependency to the
> -        * memory access, but on the current thread we do not guarantee that
> -        * the new page->flags are visible before the tags were updated.
> -        */
> -       smp_wmb();
>         mte_clear_page_tags(page_address(page));
>  }
>
> diff --git a/arch/arm64/mm/copypage.c b/arch/arm64/mm/copypage.c
> index b5447e53cd73..70a71f38b6a9 100644
> --- a/arch/arm64/mm/copypage.c
> +++ b/arch/arm64/mm/copypage.c
> @@ -23,15 +23,6 @@ void copy_highpage(struct page *to, struct page *from)
>
>         if (system_supports_mte() && test_bit(PG_mte_tagged, &from->flags)) {
>                 set_bit(PG_mte_tagged, &to->flags);
> -               page_kasan_tag_reset(to);
> -               /*
> -                * We need smp_wmb() in between setting the flags and clearing the
> -                * tags because if another thread reads page->flags and builds a
> -                * tagged address out of it, there is an actual dependency to the
> -                * memory access, but on the current thread we do not guarantee that
> -                * the new page->flags are visible before the tags were updated.
> -                */
> -               smp_wmb();
>                 mte_copy_page_tags(kto, kfrom);
>         }
>  }
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index 77341b160aca..f2f21cd6d43f 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -926,6 +926,5 @@ struct page *alloc_zeroed_user_highpage_movable(struct vm_area_struct *vma,
>  void tag_clear_highpage(struct page *page)
>  {
>         mte_zero_clear_page_tags(page_address(page));
> -       page_kasan_tag_reset(page);

This change is not a part of e5b8d9218951e59df986f627ec93569a0d22149b
revert. I think it should go into a separate commit.


>         set_bit(PG_mte_tagged, &page->flags);
>  }
> diff --git a/arch/arm64/mm/mteswap.c b/arch/arm64/mm/mteswap.c
> index a9e50e930484..4334dec93bd4 100644
> --- a/arch/arm64/mm/mteswap.c
> +++ b/arch/arm64/mm/mteswap.c
> @@ -53,15 +53,6 @@ bool mte_restore_tags(swp_entry_t entry, struct page *page)
>         if (!tags)
>                 return false;
>
> -       page_kasan_tag_reset(page);
> -       /*
> -        * We need smp_wmb() in between setting the flags and clearing the
> -        * tags because if another thread reads page->flags and builds a
> -        * tagged address out of it, there is an actual dependency to the
> -        * memory access, but on the current thread we do not guarantee that
> -        * the new page->flags are visible before the tags were updated.
> -        */
> -       smp_wmb();
>         mte_restore_page_tags(page_address(page), tags);
>
>         return true;

Re: [PATCH 3/3] arm64: kasan: Revert "arm64: mte: reset the page tag in page->flags"

[Andrey Konovalov]

On Tue, May 17, 2022 at 8:09 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> This reverts commit e5b8d9218951e59df986f627ec93569a0d22149b.
>
> On a system with MTE and KASAN_HW_TAGS enabled, when a page is allocated
> kasan_unpoison_pages() sets a random tag and saves it in page->flags.
> page_to_virt() re-creates the correct tagged pointer.
>
> If such page is mapped in user-space with PROT_MTE, the architecture
> code will set the tag to 0 and a subsequent page_to_virt() dereference
> will fault. The reverted commit aimed to fix this by resetting the tag
> in page->flags so that it is 0xff (match-all, not faulting). However,
> setting the tags and flags can race with another CPU reading the flags
> (page_to_virt()) and barriers can't help:
>
> P0 (mte_sync_page_tags):        P1 (memcpy from virt_to_page):
>                                   Rflags!=0xff
>   Wflags=0xff
>   DMB (doesn't help)
>   Wtags=0
>                                   Rtags=0   // fault
>
> Since clearing the flags in the arch code doesn't help, revert the patch
> altogether. In addition, remove the page_kasan_tag_reset() call in
> tag_clear_highpage() since the core kasan code should take care of
> resetting the page tag.
>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Vincenzo Frascino <vincenzo.frascino@arm.com>
> Cc: Andrey Konovalov <andreyknvl@gmail.com>
> Cc: Peter Collingbourne <pcc@google.com>
> ---
>  arch/arm64/kernel/hibernate.c | 5 -----
>  arch/arm64/kernel/mte.c       | 9 ---------
>  arch/arm64/mm/copypage.c      | 9 ---------
>  arch/arm64/mm/fault.c         | 1 -
>  arch/arm64/mm/mteswap.c       | 9 ---------
>  5 files changed, 33 deletions(-)
>
> diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c
> index 6328308be272..7754ef328657 100644
> --- a/arch/arm64/kernel/hibernate.c
> +++ b/arch/arm64/kernel/hibernate.c
> @@ -300,11 +300,6 @@ static void swsusp_mte_restore_tags(void)
>                 unsigned long pfn = xa_state.xa_index;
>                 struct page *page = pfn_to_online_page(pfn);
>
> -               /*
> -                * It is not required to invoke page_kasan_tag_reset(page)
> -                * at this point since the tags stored in page->flags are
> -                * already restored.
> -                */
>                 mte_restore_page_tags(page_address(page), tags);
>
>                 mte_free_tag_storage(tags);
> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> index 78b3e0f8e997..90994aca54f3 100644
> --- a/arch/arm64/kernel/mte.c
> +++ b/arch/arm64/kernel/mte.c
> @@ -47,15 +47,6 @@ static void mte_sync_page_tags(struct page *page, pte_t old_pte,
>         if (!pte_is_tagged)
>                 return;
>
> -       page_kasan_tag_reset(page);
> -       /*
> -        * We need smp_wmb() in between setting the flags and clearing the
> -        * tags because if another thread reads page->flags and builds a
> -        * tagged address out of it, there is an actual dependency to the
> -        * memory access, but on the current thread we do not guarantee that
> -        * the new page->flags are visible before the tags were updated.
> -        */
> -       smp_wmb();
>         mte_clear_page_tags(page_address(page));
>  }
>
> diff --git a/arch/arm64/mm/copypage.c b/arch/arm64/mm/copypage.c
> index b5447e53cd73..70a71f38b6a9 100644
> --- a/arch/arm64/mm/copypage.c
> +++ b/arch/arm64/mm/copypage.c
> @@ -23,15 +23,6 @@ void copy_highpage(struct page *to, struct page *from)
>
>         if (system_supports_mte() && test_bit(PG_mte_tagged, &from->flags)) {
>                 set_bit(PG_mte_tagged, &to->flags);
> -               page_kasan_tag_reset(to);
> -               /*
> -                * We need smp_wmb() in between setting the flags and clearing the
> -                * tags because if another thread reads page->flags and builds a
> -                * tagged address out of it, there is an actual dependency to the
> -                * memory access, but on the current thread we do not guarantee that
> -                * the new page->flags are visible before the tags were updated.
> -                */
> -               smp_wmb();
>                 mte_copy_page_tags(kto, kfrom);
>         }
>  }
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index 77341b160aca..f2f21cd6d43f 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -926,6 +926,5 @@ struct page *alloc_zeroed_user_highpage_movable(struct vm_area_struct *vma,
>  void tag_clear_highpage(struct page *page)
>  {
>         mte_zero_clear_page_tags(page_address(page));
> -       page_kasan_tag_reset(page);

This change is not a part of e5b8d9218951e59df986f627ec93569a0d22149b
revert. I think it should go into a separate commit.


>         set_bit(PG_mte_tagged, &page->flags);
>  }
> diff --git a/arch/arm64/mm/mteswap.c b/arch/arm64/mm/mteswap.c
> index a9e50e930484..4334dec93bd4 100644
> --- a/arch/arm64/mm/mteswap.c
> +++ b/arch/arm64/mm/mteswap.c
> @@ -53,15 +53,6 @@ bool mte_restore_tags(swp_entry_t entry, struct page *page)
>         if (!tags)
>                 return false;
>
> -       page_kasan_tag_reset(page);
> -       /*
> -        * We need smp_wmb() in between setting the flags and clearing the
> -        * tags because if another thread reads page->flags and builds a
> -        * tagged address out of it, there is an actual dependency to the
> -        * memory access, but on the current thread we do not guarantee that
> -        * the new page->flags are visible before the tags were updated.
> -        */
> -       smp_wmb();
>         mte_restore_page_tags(page_address(page), tags);
>
>         return true;

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] kasan: Fix ordering between MTE tag colouring and page->flags

[Andrey Konovalov]

On Tue, May 17, 2022 at 8:09 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> Hi,

Hi Catalin,

> That's more of an RFC to get a discussion started. I plan to eventually
> apply the third patch reverting the page_kasan_tag_reset() calls under
> arch/arm64 since they don't cover all cases (the race is rare and we
> haven't hit anything yet but it's possible).
>
> On a system with MTE and KASAN_HW_TAGS enabled, when a page is allocated
> kasan_unpoison_pages() sets a random tag and saves it in page->flags so
> that page_to_virt() re-creates the correct tagged pointer. We need to
> ensure that the in-memory tags are visible before setting the
> page->flags:
>
> P0 (__kasan_unpoison_range):    P1 (access via virt_to_page):
>   Wtags=x                         Rflags=x
>     |                               |
>     | DMB                           | address dependency
>     V                               V
>   Wflags=x                        Rtags=x

This is confusing: the paragraph mentions page_to_virt() and the
diagram - virt_to_page(). I assume it should be page_to_virt().

alloc_pages(), which calls kasan_unpoison_pages(), has to return
before page_to_virt() can be called. So they only can race if the tags
don't get propagated to memory before alloc_pages() returns, right?
This is why you say that the race is rare?

> The first patch changes the order of page unpoisoning with the tag
> storing in page->flags. page_kasan_tag_set() has the right barriers
> through try_cmpxchg().

[...]

> If such page is mapped in user-space with PROT_MTE, the architecture
> code will set the tag to 0 and a subsequent page_to_virt() dereference
> will fault. We currently try to fix this by resetting the tag in
> page->flags so that it is 0xff (match-all, not faulting). However,
> setting the tags and flags can race with another CPU reading the flags
> (page_to_virt()) and barriers can't help, e.g.:
>
> P0 (mte_sync_page_tags):        P1 (memcpy from virt_to_page):
>                                   Rflags!=0xff
>   Wflags=0xff
>   DMB (doesn't help)
>   Wtags=0
>                                   Rtags=0   // fault

So this change, effectively, makes the tag in page->flags for GFP_USER
pages to be reset at allocation time. And the current approach of
resetting the tag when the kernel is about to access these pages is
not good because: 1. it's inconvenient to track all places where this
should be done and 2. the tag reset can race with page_to_virt() even
with patch #1 applied. Is my understanding correct?

This will reset the tags for all kinds of GFP_USER allocations, not
only for the ones intended for MAP_ANONYMOUS and RAM-based file
mappings, for which userspace can set tags, right? This will thus
weaken in-kernel MTE for pages whose tags can't even be set by
userspace. Is there a way to deal with this?

> Since clearing the flags in the arch code doesn't work, try to do this
> at page allocation time by a new flag added to GFP_USER. Could we
> instead add __GFP_SKIP_KASAN_UNPOISON rather than a new flag?

Why do we need a new flag? Can we just check & GFP_USER instead?

Thanks!

Re: [PATCH 0/3] kasan: Fix ordering between MTE tag colouring and page->flags

[Andrey Konovalov]

On Tue, May 17, 2022 at 8:09 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> Hi,

Hi Catalin,

> That's more of an RFC to get a discussion started. I plan to eventually
> apply the third patch reverting the page_kasan_tag_reset() calls under
> arch/arm64 since they don't cover all cases (the race is rare and we
> haven't hit anything yet but it's possible).
>
> On a system with MTE and KASAN_HW_TAGS enabled, when a page is allocated
> kasan_unpoison_pages() sets a random tag and saves it in page->flags so
> that page_to_virt() re-creates the correct tagged pointer. We need to
> ensure that the in-memory tags are visible before setting the
> page->flags:
>
> P0 (__kasan_unpoison_range):    P1 (access via virt_to_page):
>   Wtags=x                         Rflags=x
>     |                               |
>     | DMB                           | address dependency
>     V                               V
>   Wflags=x                        Rtags=x

This is confusing: the paragraph mentions page_to_virt() and the
diagram - virt_to_page(). I assume it should be page_to_virt().

alloc_pages(), which calls kasan_unpoison_pages(), has to return
before page_to_virt() can be called. So they only can race if the tags
don't get propagated to memory before alloc_pages() returns, right?
This is why you say that the race is rare?

> The first patch changes the order of page unpoisoning with the tag
> storing in page->flags. page_kasan_tag_set() has the right barriers
> through try_cmpxchg().

[...]

> If such page is mapped in user-space with PROT_MTE, the architecture
> code will set the tag to 0 and a subsequent page_to_virt() dereference
> will fault. We currently try to fix this by resetting the tag in
> page->flags so that it is 0xff (match-all, not faulting). However,
> setting the tags and flags can race with another CPU reading the flags
> (page_to_virt()) and barriers can't help, e.g.:
>
> P0 (mte_sync_page_tags):        P1 (memcpy from virt_to_page):
>                                   Rflags!=0xff
>   Wflags=0xff
>   DMB (doesn't help)
>   Wtags=0
>                                   Rtags=0   // fault

So this change, effectively, makes the tag in page->flags for GFP_USER
pages to be reset at allocation time. And the current approach of
resetting the tag when the kernel is about to access these pages is
not good because: 1. it's inconvenient to track all places where this
should be done and 2. the tag reset can race with page_to_virt() even
with patch #1 applied. Is my understanding correct?

This will reset the tags for all kinds of GFP_USER allocations, not
only for the ones intended for MAP_ANONYMOUS and RAM-based file
mappings, for which userspace can set tags, right? This will thus
weaken in-kernel MTE for pages whose tags can't even be set by
userspace. Is there a way to deal with this?

> Since clearing the flags in the arch code doesn't work, try to do this
> at page allocation time by a new flag added to GFP_USER. Could we
> instead add __GFP_SKIP_KASAN_UNPOISON rather than a new flag?

Why do we need a new flag? Can we just check & GFP_USER instead?

Thanks!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] kasan: Fix ordering between MTE tag colouring and page->flags

[Catalin Marinas]

On Thu, May 19, 2022 at 11:45:04PM +0200, Andrey Konovalov wrote:
> On Tue, May 17, 2022 at 8:09 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
> > That's more of an RFC to get a discussion started. I plan to eventually
> > apply the third patch reverting the page_kasan_tag_reset() calls under
> > arch/arm64 since they don't cover all cases (the race is rare and we
> > haven't hit anything yet but it's possible).
> >
> > On a system with MTE and KASAN_HW_TAGS enabled, when a page is allocated
> > kasan_unpoison_pages() sets a random tag and saves it in page->flags so
> > that page_to_virt() re-creates the correct tagged pointer. We need to
> > ensure that the in-memory tags are visible before setting the
> > page->flags:
> >
> > P0 (__kasan_unpoison_range):    P1 (access via virt_to_page):
> >   Wtags=x                         Rflags=x
> >     |                               |
> >     | DMB                           | address dependency
> >     V                               V
> >   Wflags=x                        Rtags=x
> 
> This is confusing: the paragraph mentions page_to_virt() and the
> diagram - virt_to_page(). I assume it should be page_to_virt().

Yes, it should be page_to_virt().

> alloc_pages(), which calls kasan_unpoison_pages(), has to return
> before page_to_virt() can be called. So they only can race if the tags
> don't get propagated to memory before alloc_pages() returns, right?
> This is why you say that the race is rare?

Yeah, it involves another CPU getting the pfn or page address and trying
to access it before the tags are propagated (not necessarily to DRAM, it
can be some some cache level or they are just stuck in a writebuffer).
It's unlikely but still possible.

See a somewhat related recent memory ordering fix, it was found in
actual testing:

https://git.kernel.org/arm64/c/1d0cb4c8864a

> > If such page is mapped in user-space with PROT_MTE, the architecture
> > code will set the tag to 0 and a subsequent page_to_virt() dereference
> > will fault. We currently try to fix this by resetting the tag in
> > page->flags so that it is 0xff (match-all, not faulting). However,
> > setting the tags and flags can race with another CPU reading the flags
> > (page_to_virt()) and barriers can't help, e.g.:
> >
> > P0 (mte_sync_page_tags):        P1 (memcpy from virt_to_page):
> >                                   Rflags!=0xff
> >   Wflags=0xff
> >   DMB (doesn't help)
> >   Wtags=0
> >                                   Rtags=0   // fault
> 
> So this change, effectively, makes the tag in page->flags for GFP_USER
> pages to be reset at allocation time. And the current approach of
> resetting the tag when the kernel is about to access these pages is
> not good because: 1. it's inconvenient to track all places where this
> should be done and 2. the tag reset can race with page_to_virt() even
> with patch #1 applied. Is my understanding correct?

Yes. Regarding (1), it's pretty impractical. There are some clear places
like copy_user_highpage() where we could untag the page address. In
others others it may not be as simple. We could try to reset the page
flags when we do a get_user_pages() to cover another class. But we still
have swap, page migration that may read a page with a mismatched tag.

> This will reset the tags for all kinds of GFP_USER allocations, not
> only for the ones intended for MAP_ANONYMOUS and RAM-based file
> mappings, for which userspace can set tags, right? This will thus
> weaken in-kernel MTE for pages whose tags can't even be set by
> userspace. Is there a way to deal with this?

That's correct, it will weaken some of the allocations where the user
doesn't care about MTE. And TBH, I'm not sure it covers all cases
either (can we have an anonymous or memfd page mapped in user space that
was not allocated with GFP_USER?).

Another option would be to lock the page but set_pte_at() seems to be
called for pages both locked and unlocked.

Any suggestions are welcomed.

> > Since clearing the flags in the arch code doesn't work, try to do this
> > at page allocation time by a new flag added to GFP_USER. Could we
> > instead add __GFP_SKIP_KASAN_UNPOISON rather than a new flag?
> 
> Why do we need a new flag? Can we just check & GFP_USER instead?

GFP_USER is not a flag as such but a combination of flags, none of which
says explicitly it's meant for user.

-- 
Catalin

Re: [PATCH 0/3] kasan: Fix ordering between MTE tag colouring and page->flags

[Catalin Marinas]

On Thu, May 19, 2022 at 11:45:04PM +0200, Andrey Konovalov wrote:
> On Tue, May 17, 2022 at 8:09 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
> > That's more of an RFC to get a discussion started. I plan to eventually
> > apply the third patch reverting the page_kasan_tag_reset() calls under
> > arch/arm64 since they don't cover all cases (the race is rare and we
> > haven't hit anything yet but it's possible).
> >
> > On a system with MTE and KASAN_HW_TAGS enabled, when a page is allocated
> > kasan_unpoison_pages() sets a random tag and saves it in page->flags so
> > that page_to_virt() re-creates the correct tagged pointer. We need to
> > ensure that the in-memory tags are visible before setting the
> > page->flags:
> >
> > P0 (__kasan_unpoison_range):    P1 (access via virt_to_page):
> >   Wtags=x                         Rflags=x
> >     |                               |
> >     | DMB                           | address dependency
> >     V                               V
> >   Wflags=x                        Rtags=x
> 
> This is confusing: the paragraph mentions page_to_virt() and the
> diagram - virt_to_page(). I assume it should be page_to_virt().

Yes, it should be page_to_virt().

> alloc_pages(), which calls kasan_unpoison_pages(), has to return
> before page_to_virt() can be called. So they only can race if the tags
> don't get propagated to memory before alloc_pages() returns, right?
> This is why you say that the race is rare?

Yeah, it involves another CPU getting the pfn or page address and trying
to access it before the tags are propagated (not necessarily to DRAM, it
can be some some cache level or they are just stuck in a writebuffer).
It's unlikely but still possible.

See a somewhat related recent memory ordering fix, it was found in
actual testing:

https://git.kernel.org/arm64/c/1d0cb4c8864a

> > If such page is mapped in user-space with PROT_MTE, the architecture
> > code will set the tag to 0 and a subsequent page_to_virt() dereference
> > will fault. We currently try to fix this by resetting the tag in
> > page->flags so that it is 0xff (match-all, not faulting). However,
> > setting the tags and flags can race with another CPU reading the flags
> > (page_to_virt()) and barriers can't help, e.g.:
> >
> > P0 (mte_sync_page_tags):        P1 (memcpy from virt_to_page):
> >                                   Rflags!=0xff
> >   Wflags=0xff
> >   DMB (doesn't help)
> >   Wtags=0
> >                                   Rtags=0   // fault
> 
> So this change, effectively, makes the tag in page->flags for GFP_USER
> pages to be reset at allocation time. And the current approach of
> resetting the tag when the kernel is about to access these pages is
> not good because: 1. it's inconvenient to track all places where this
> should be done and 2. the tag reset can race with page_to_virt() even
> with patch #1 applied. Is my understanding correct?

Yes. Regarding (1), it's pretty impractical. There are some clear places
like copy_user_highpage() where we could untag the page address. In
others others it may not be as simple. We could try to reset the page
flags when we do a get_user_pages() to cover another class. But we still
have swap, page migration that may read a page with a mismatched tag.

> This will reset the tags for all kinds of GFP_USER allocations, not
> only for the ones intended for MAP_ANONYMOUS and RAM-based file
> mappings, for which userspace can set tags, right? This will thus
> weaken in-kernel MTE for pages whose tags can't even be set by
> userspace. Is there a way to deal with this?

That's correct, it will weaken some of the allocations where the user
doesn't care about MTE. And TBH, I'm not sure it covers all cases
either (can we have an anonymous or memfd page mapped in user space that
was not allocated with GFP_USER?).

Another option would be to lock the page but set_pte_at() seems to be
called for pages both locked and unlocked.

Any suggestions are welcomed.

> > Since clearing the flags in the arch code doesn't work, try to do this
> > at page allocation time by a new flag added to GFP_USER. Could we
> > instead add __GFP_SKIP_KASAN_UNPOISON rather than a new flag?
> 
> Why do we need a new flag? Can we just check & GFP_USER instead?

GFP_USER is not a flag as such but a combination of flags, none of which
says explicitly it's meant for user.

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] kasan: Fix ordering between MTE tag colouring and page->flags

[Andrey Konovalov]

On Fri, May 20, 2022 at 3:01 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> > So this change, effectively, makes the tag in page->flags for GFP_USER
> > pages to be reset at allocation time. And the current approach of
> > resetting the tag when the kernel is about to access these pages is
> > not good because: 1. it's inconvenient to track all places where this
> > should be done and 2. the tag reset can race with page_to_virt() even
> > with patch #1 applied. Is my understanding correct?
>
> Yes. Regarding (1), it's pretty impractical. There are some clear places
> like copy_user_highpage() where we could untag the page address. In
> others others it may not be as simple. We could try to reset the page
> flags when we do a get_user_pages() to cover another class. But we still
> have swap, page migration that may read a page with a mismatched tag.

I see.

> > This will reset the tags for all kinds of GFP_USER allocations, not
> > only for the ones intended for MAP_ANONYMOUS and RAM-based file
> > mappings, for which userspace can set tags, right? This will thus
> > weaken in-kernel MTE for pages whose tags can't even be set by
> > userspace. Is there a way to deal with this?
>
> That's correct, it will weaken some of the allocations where the user
> doesn't care about MTE.

Well, while this is unfortunate, I don't mind the change.

I've left some comments on the patches.

> > > Since clearing the flags in the arch code doesn't work, try to do this
> > > at page allocation time by a new flag added to GFP_USER.

Does this have to be GFP_USER? Can we add new flags to
GFP_HIGHUSER_MOVABLE instead?

For instance, Peter added __GFP_SKIP_KASAN_POISON to
GFP_HIGHUSER_MOVABLE in c275c5c6d50a0.

> > > Could we
> > > instead add __GFP_SKIP_KASAN_UNPOISON rather than a new flag?

Adding __GFP_SKIP_KASAN_UNPOISON makes sense, but we still need to
reset the tag in page->flags.

Thanks!

Re: [PATCH 0/3] kasan: Fix ordering between MTE tag colouring and page->flags

[Andrey Konovalov]

On Fri, May 20, 2022 at 3:01 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> > So this change, effectively, makes the tag in page->flags for GFP_USER
> > pages to be reset at allocation time. And the current approach of
> > resetting the tag when the kernel is about to access these pages is
> > not good because: 1. it's inconvenient to track all places where this
> > should be done and 2. the tag reset can race with page_to_virt() even
> > with patch #1 applied. Is my understanding correct?
>
> Yes. Regarding (1), it's pretty impractical. There are some clear places
> like copy_user_highpage() where we could untag the page address. In
> others others it may not be as simple. We could try to reset the page
> flags when we do a get_user_pages() to cover another class. But we still
> have swap, page migration that may read a page with a mismatched tag.

I see.

> > This will reset the tags for all kinds of GFP_USER allocations, not
> > only for the ones intended for MAP_ANONYMOUS and RAM-based file
> > mappings, for which userspace can set tags, right? This will thus
> > weaken in-kernel MTE for pages whose tags can't even be set by
> > userspace. Is there a way to deal with this?
>
> That's correct, it will weaken some of the allocations where the user
> doesn't care about MTE.

Well, while this is unfortunate, I don't mind the change.

I've left some comments on the patches.

> > > Since clearing the flags in the arch code doesn't work, try to do this
> > > at page allocation time by a new flag added to GFP_USER.

Does this have to be GFP_USER? Can we add new flags to
GFP_HIGHUSER_MOVABLE instead?

For instance, Peter added __GFP_SKIP_KASAN_POISON to
GFP_HIGHUSER_MOVABLE in c275c5c6d50a0.

> > > Could we
> > > instead add __GFP_SKIP_KASAN_UNPOISON rather than a new flag?

Adding __GFP_SKIP_KASAN_UNPOISON makes sense, but we still need to
reset the tag in page->flags.

Thanks!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] kasan: Fix ordering between MTE tag colouring and page->flags

[Catalin Marinas]

On Sun, May 22, 2022 at 12:20:26AM +0200, Andrey Konovalov wrote:
> On Fri, May 20, 2022 at 3:01 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
> > > This will reset the tags for all kinds of GFP_USER allocations, not
> > > only for the ones intended for MAP_ANONYMOUS and RAM-based file
> > > mappings, for which userspace can set tags, right? This will thus
> > > weaken in-kernel MTE for pages whose tags can't even be set by
> > > userspace. Is there a way to deal with this?
> >
> > That's correct, it will weaken some of the allocations where the user
> > doesn't care about MTE.
> 
> Well, while this is unfortunate, I don't mind the change.
> 
> I've left some comments on the patches.

Thanks. I'll update and post at -rc1.

> > > > Since clearing the flags in the arch code doesn't work, try to do this
> > > > at page allocation time by a new flag added to GFP_USER.
> 
> Does this have to be GFP_USER? Can we add new flags to
> GFP_HIGHUSER_MOVABLE instead?
> 
> For instance, Peter added __GFP_SKIP_KASAN_POISON to
> GFP_HIGHUSER_MOVABLE in c275c5c6d50a0.

The above commit was a performance improvement. Here we need to address
the correctness. However, looking through the GFP_USER cases, I don't
think any of them is at risk of ending up in user space with PROT_MTE.
There are places where GFP_USER is passed to kmalloc() for in-kernel
objects that would never be mapped to user, though the new gfp flag
won't be taken into account.

I'm ok to move the new flag to the GFP_HIGHUSER_MOVABLE but probably
still keep a page_kasan_tag_reset() on the set_pte_at() path together
with a WARN_ON_ONCE() if we miss anything.

> > > > Could we
> > > > instead add __GFP_SKIP_KASAN_UNPOISON rather than a new flag?
> 
> Adding __GFP_SKIP_KASAN_UNPOISON makes sense, but we still need to
> reset the tag in page->flags.

My thought was to reset the tag in page->flags based on 'unpoison'
alone without any extra flags. We use this flag for vmalloc() pages but
it seems we don't reset the page tags (as we do via
kasan_poison_slab()).

-- 
Catalin

Re: [PATCH 0/3] kasan: Fix ordering between MTE tag colouring and page->flags

[Catalin Marinas]

On Sun, May 22, 2022 at 12:20:26AM +0200, Andrey Konovalov wrote:
> On Fri, May 20, 2022 at 3:01 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
> > > This will reset the tags for all kinds of GFP_USER allocations, not
> > > only for the ones intended for MAP_ANONYMOUS and RAM-based file
> > > mappings, for which userspace can set tags, right? This will thus
> > > weaken in-kernel MTE for pages whose tags can't even be set by
> > > userspace. Is there a way to deal with this?
> >
> > That's correct, it will weaken some of the allocations where the user
> > doesn't care about MTE.
> 
> Well, while this is unfortunate, I don't mind the change.
> 
> I've left some comments on the patches.

Thanks. I'll update and post at -rc1.

> > > > Since clearing the flags in the arch code doesn't work, try to do this
> > > > at page allocation time by a new flag added to GFP_USER.
> 
> Does this have to be GFP_USER? Can we add new flags to
> GFP_HIGHUSER_MOVABLE instead?
> 
> For instance, Peter added __GFP_SKIP_KASAN_POISON to
> GFP_HIGHUSER_MOVABLE in c275c5c6d50a0.

The above commit was a performance improvement. Here we need to address
the correctness. However, looking through the GFP_USER cases, I don't
think any of them is at risk of ending up in user space with PROT_MTE.
There are places where GFP_USER is passed to kmalloc() for in-kernel
objects that would never be mapped to user, though the new gfp flag
won't be taken into account.

I'm ok to move the new flag to the GFP_HIGHUSER_MOVABLE but probably
still keep a page_kasan_tag_reset() on the set_pte_at() path together
with a WARN_ON_ONCE() if we miss anything.

> > > > Could we
> > > > instead add __GFP_SKIP_KASAN_UNPOISON rather than a new flag?
> 
> Adding __GFP_SKIP_KASAN_UNPOISON makes sense, but we still need to
> reset the tag in page->flags.

My thought was to reset the tag in page->flags based on 'unpoison'
alone without any extra flags. We use this flag for vmalloc() pages but
it seems we don't reset the page tags (as we do via
kasan_poison_slab()).

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] kasan: Fix ordering between MTE tag colouring and page->flags

[Andrey Konovalov]

On Wed, May 25, 2022 at 5:45 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> > Does this have to be GFP_USER? Can we add new flags to
> > GFP_HIGHUSER_MOVABLE instead?
> >
> > For instance, Peter added __GFP_SKIP_KASAN_POISON to
> > GFP_HIGHUSER_MOVABLE in c275c5c6d50a0.
>
> The above commit was a performance improvement. Here we need to address
> the correctness. However, looking through the GFP_USER cases, I don't
> think any of them is at risk of ending up in user space with PROT_MTE.
> There are places where GFP_USER is passed to kmalloc() for in-kernel
> objects that would never be mapped to user, though the new gfp flag
> won't be taken into account.

Yeah, those kmalloc()'s look suspicious.

> I'm ok to move the new flag to the GFP_HIGHUSER_MOVABLE but probably
> still keep a page_kasan_tag_reset() on the set_pte_at() path together
> with a WARN_ON_ONCE() if we miss anything.

GFP_HIGHUSER_MOVABLE is used in fewer places than GFP_USER, so if it
works - great!

However, see below.

> > Adding __GFP_SKIP_KASAN_UNPOISON makes sense, but we still need to
> > reset the tag in page->flags.
>
> My thought was to reset the tag in page->flags based on 'unpoison'
> alone without any extra flags. We use this flag for vmalloc() pages but
> it seems we don't reset the page tags (as we do via
> kasan_poison_slab()).

I just realized that we already have __GFP_ZEROTAGS that initializes
both in-memory and page->flags tags. Currently only used for user
pages allocated via alloc_zeroed_user_highpage_movable(). Perhaps we
can add this flag to GFP_HIGHUSER_MOVABLE?

We'll also need to change the behavior of __GFP_ZEROTAGS to work even
when GFP_ZERO is not set, but this doesn't seem to be a problem.

And, at this point, we can probably combine __GFP_ZEROTAGS with
__GFP_SKIP_KASAN_POISON, as they both would target user pages.

Does this make sense?

Re: [PATCH 0/3] kasan: Fix ordering between MTE tag colouring and page->flags

[Andrey Konovalov]

On Wed, May 25, 2022 at 5:45 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> > Does this have to be GFP_USER? Can we add new flags to
> > GFP_HIGHUSER_MOVABLE instead?
> >
> > For instance, Peter added __GFP_SKIP_KASAN_POISON to
> > GFP_HIGHUSER_MOVABLE in c275c5c6d50a0.
>
> The above commit was a performance improvement. Here we need to address
> the correctness. However, looking through the GFP_USER cases, I don't
> think any of them is at risk of ending up in user space with PROT_MTE.
> There are places where GFP_USER is passed to kmalloc() for in-kernel
> objects that would never be mapped to user, though the new gfp flag
> won't be taken into account.

Yeah, those kmalloc()'s look suspicious.

> I'm ok to move the new flag to the GFP_HIGHUSER_MOVABLE but probably
> still keep a page_kasan_tag_reset() on the set_pte_at() path together
> with a WARN_ON_ONCE() if we miss anything.

GFP_HIGHUSER_MOVABLE is used in fewer places than GFP_USER, so if it
works - great!

However, see below.

> > Adding __GFP_SKIP_KASAN_UNPOISON makes sense, but we still need to
> > reset the tag in page->flags.
>
> My thought was to reset the tag in page->flags based on 'unpoison'
> alone without any extra flags. We use this flag for vmalloc() pages but
> it seems we don't reset the page tags (as we do via
> kasan_poison_slab()).

I just realized that we already have __GFP_ZEROTAGS that initializes
both in-memory and page->flags tags. Currently only used for user
pages allocated via alloc_zeroed_user_highpage_movable(). Perhaps we
can add this flag to GFP_HIGHUSER_MOVABLE?

We'll also need to change the behavior of __GFP_ZEROTAGS to work even
when GFP_ZERO is not set, but this doesn't seem to be a problem.

And, at this point, we can probably combine __GFP_ZEROTAGS with
__GFP_SKIP_KASAN_POISON, as they both would target user pages.

Does this make sense?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] kasan: Fix ordering between MTE tag colouring and page->flags

[Catalin Marinas]

On Wed, May 25, 2022 at 07:41:08PM +0200, Andrey Konovalov wrote:
> On Wed, May 25, 2022 at 5:45 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
> > > Adding __GFP_SKIP_KASAN_UNPOISON makes sense, but we still need to
> > > reset the tag in page->flags.
> >
> > My thought was to reset the tag in page->flags based on 'unpoison'
> > alone without any extra flags. We use this flag for vmalloc() pages but
> > it seems we don't reset the page tags (as we do via
> > kasan_poison_slab()).
> 
> I just realized that we already have __GFP_ZEROTAGS that initializes
> both in-memory and page->flags tags.

IIUC it only zeroes the tags and skips the unpoisoning but
page_kasan_tag() remains unchanged.

> Currently only used for user
> pages allocated via alloc_zeroed_user_highpage_movable(). Perhaps we
> can add this flag to GFP_HIGHUSER_MOVABLE?

I wouldn't add __GFP_ZEROTAGS to GFP_HIGHUSER_MOVABLE as we only need it
if the page is mapped with PROT_MTE. Clearing a page without tags may be
marginally faster.

> We'll also need to change the behavior of __GFP_ZEROTAGS to work even
> when GFP_ZERO is not set, but this doesn't seem to be a problem.

Why? We'd get unnecessary tag zeroing. We have these cases for
anonymous, private pages:

1. Zeroed page allocation without PROT_MTE: we need GFP_ZERO and
   page_kasan_tag_reset() in case of later mprotect(PROT_MTE).

2. Zeroed page allocation with PROT_MTE: we need GFP_ZERO,
   __GFP_ZEROTAGS and page_kasan_tag_reset().

3. CoW page allocation without PROT_MTE: copy data and we only need
   page_kasan_tag_reset() in case of later mprotect(PROT_MTE).

4. CoW page allocation with PROT_MTE: copy data and tags together with
   page_kasan_tag_reset().

So basically we always need page_kasan_tag_reset() for pages mapped in
user space even if they are not PROT_MTE, in case of a later
mprotect(PROT_MTE). For (1), (3) and (4) we don't need to zero the tags.
For (1) maybe we could do it as part of data zeroing (subject to some
benchmarks) but for (3) and (4) they'd be overridden by the copy anyway.

> And, at this point, we can probably combine __GFP_ZEROTAGS with
> __GFP_SKIP_KASAN_POISON, as they both would target user pages.

For user pages, I think we should skip unpoisoning as well. We can keep
unpoisoning around but if we end up calling page_kasan_tag_reset(),
there's not much value, at least in page_address() accesses since the
pointer would match all tags. That's unless you want to detect other
stray pointers to such pages but we already skip the poisoning on free,
so it doesn't seem to be a use-case.

If we skip unpoisoning (not just poisoning as we already do) for user
pages, we should reset the tags in page->flags. Whether __GFP_ZEROTAGS
is passed is complementary, depending on the reason for allocation.
Currently if __GFP_ZEROTAGS is passed, the unpoisoning is skipped but I
think we should have just added __GFP_SKIP_KASAN_UNPOISON instead and
not add a new argument to should_skip_kasan_unpoison(). If we decide to
always skip unpoisoning, something like below on top of the vanilla
kernel:

-------------8<-----------------
diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index 3e3d36fc2109..df0ec30524fb 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -348,7 +348,7 @@ struct vm_area_struct;
 #define GFP_DMA32	__GFP_DMA32
 #define GFP_HIGHUSER	(GFP_USER | __GFP_HIGHMEM)
 #define GFP_HIGHUSER_MOVABLE	(GFP_HIGHUSER | __GFP_MOVABLE | \
-			 __GFP_SKIP_KASAN_POISON)
+			 __GFP_SKIP_KASAN_POISON | __GFP_SKIP_KASAN_UNPOISON)
 #define GFP_TRANSHUGE_LIGHT	((GFP_HIGHUSER_MOVABLE | __GFP_COMP | \
 			 __GFP_NOMEMALLOC | __GFP_NOWARN) & ~__GFP_RECLAIM)
 #define GFP_TRANSHUGE	(GFP_TRANSHUGE_LIGHT | __GFP_DIRECT_RECLAIM)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 0e42038382c1..3173e8f0e69a 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2346,7 +2346,7 @@ static inline bool check_new_pcp(struct page *page, unsigned int order)
 }
 #endif /* CONFIG_DEBUG_VM */

-static inline bool should_skip_kasan_unpoison(gfp_t flags, bool init_tags)
+static inline bool should_skip_kasan_unpoison(gfp_t flags)
 {
 	/* Don't skip if a software KASAN mode is enabled. */
 	if (IS_ENABLED(CONFIG_KASAN_GENERIC) ||
@@ -2358,12 +2358,10 @@ static inline bool should_skip_kasan_unpoison(gfp_t flags, bool init_tags)
 		return true;

 	/*
-	 * With hardware tag-based KASAN enabled, skip if either:
-	 *
-	 * 1. Memory tags have already been cleared via tag_clear_highpage().
-	 * 2. Skipping has been requested via __GFP_SKIP_KASAN_UNPOISON.
+	 * With hardware tag-based KASAN enabled, skip if this was requested
+	 * via __GFP_SKIP_KASAN_UNPOISON.
 	 */
-	return init_tags || (flags & __GFP_SKIP_KASAN_UNPOISON);
+	return flags & __GFP_SKIP_KASAN_UNPOISON;
 }

 static inline bool should_skip_init(gfp_t flags)
@@ -2416,7 +2414,7 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
 		/* Note that memory is already initialized by the loop above. */
 		init = false;
 	}
-	if (!should_skip_kasan_unpoison(gfp_flags, init_tags)) {
+	if (!should_skip_kasan_unpoison(gfp_flags)) {
 		/* Unpoison shadow memory or set memory tags. */
 		kasan_unpoison_pages(page, order, init);
 
-------------8<-----------------

With the above, we can wire up page_kasan_tag_reset() to the
__GFP_SKIP_KASAN_UNPOISON check without any additional flags.

-- 
Catalin

Re: [PATCH 0/3] kasan: Fix ordering between MTE tag colouring and page->flags

[Catalin Marinas]

On Wed, May 25, 2022 at 07:41:08PM +0200, Andrey Konovalov wrote:
> On Wed, May 25, 2022 at 5:45 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
> > > Adding __GFP_SKIP_KASAN_UNPOISON makes sense, but we still need to
> > > reset the tag in page->flags.
> >
> > My thought was to reset the tag in page->flags based on 'unpoison'
> > alone without any extra flags. We use this flag for vmalloc() pages but
> > it seems we don't reset the page tags (as we do via
> > kasan_poison_slab()).
> 
> I just realized that we already have __GFP_ZEROTAGS that initializes
> both in-memory and page->flags tags.

IIUC it only zeroes the tags and skips the unpoisoning but
page_kasan_tag() remains unchanged.

> Currently only used for user
> pages allocated via alloc_zeroed_user_highpage_movable(). Perhaps we
> can add this flag to GFP_HIGHUSER_MOVABLE?

I wouldn't add __GFP_ZEROTAGS to GFP_HIGHUSER_MOVABLE as we only need it
if the page is mapped with PROT_MTE. Clearing a page without tags may be
marginally faster.

> We'll also need to change the behavior of __GFP_ZEROTAGS to work even
> when GFP_ZERO is not set, but this doesn't seem to be a problem.

Why? We'd get unnecessary tag zeroing. We have these cases for
anonymous, private pages:

1. Zeroed page allocation without PROT_MTE: we need GFP_ZERO and
   page_kasan_tag_reset() in case of later mprotect(PROT_MTE).

2. Zeroed page allocation with PROT_MTE: we need GFP_ZERO,
   __GFP_ZEROTAGS and page_kasan_tag_reset().

3. CoW page allocation without PROT_MTE: copy data and we only need
   page_kasan_tag_reset() in case of later mprotect(PROT_MTE).

4. CoW page allocation with PROT_MTE: copy data and tags together with
   page_kasan_tag_reset().

So basically we always need page_kasan_tag_reset() for pages mapped in
user space even if they are not PROT_MTE, in case of a later
mprotect(PROT_MTE). For (1), (3) and (4) we don't need to zero the tags.
For (1) maybe we could do it as part of data zeroing (subject to some
benchmarks) but for (3) and (4) they'd be overridden by the copy anyway.

> And, at this point, we can probably combine __GFP_ZEROTAGS with
> __GFP_SKIP_KASAN_POISON, as they both would target user pages.

For user pages, I think we should skip unpoisoning as well. We can keep
unpoisoning around but if we end up calling page_kasan_tag_reset(),
there's not much value, at least in page_address() accesses since the
pointer would match all tags. That's unless you want to detect other
stray pointers to such pages but we already skip the poisoning on free,
so it doesn't seem to be a use-case.

If we skip unpoisoning (not just poisoning as we already do) for user
pages, we should reset the tags in page->flags. Whether __GFP_ZEROTAGS
is passed is complementary, depending on the reason for allocation.
Currently if __GFP_ZEROTAGS is passed, the unpoisoning is skipped but I
think we should have just added __GFP_SKIP_KASAN_UNPOISON instead and
not add a new argument to should_skip_kasan_unpoison(). If we decide to
always skip unpoisoning, something like below on top of the vanilla
kernel:

-------------8<-----------------
diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index 3e3d36fc2109..df0ec30524fb 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -348,7 +348,7 @@ struct vm_area_struct;
 #define GFP_DMA32	__GFP_DMA32
 #define GFP_HIGHUSER	(GFP_USER | __GFP_HIGHMEM)
 #define GFP_HIGHUSER_MOVABLE	(GFP_HIGHUSER | __GFP_MOVABLE | \
-			 __GFP_SKIP_KASAN_POISON)
+			 __GFP_SKIP_KASAN_POISON | __GFP_SKIP_KASAN_UNPOISON)
 #define GFP_TRANSHUGE_LIGHT	((GFP_HIGHUSER_MOVABLE | __GFP_COMP | \
 			 __GFP_NOMEMALLOC | __GFP_NOWARN) & ~__GFP_RECLAIM)
 #define GFP_TRANSHUGE	(GFP_TRANSHUGE_LIGHT | __GFP_DIRECT_RECLAIM)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 0e42038382c1..3173e8f0e69a 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2346,7 +2346,7 @@ static inline bool check_new_pcp(struct page *page, unsigned int order)
 }
 #endif /* CONFIG_DEBUG_VM */

-static inline bool should_skip_kasan_unpoison(gfp_t flags, bool init_tags)
+static inline bool should_skip_kasan_unpoison(gfp_t flags)
 {
 	/* Don't skip if a software KASAN mode is enabled. */
 	if (IS_ENABLED(CONFIG_KASAN_GENERIC) ||
@@ -2358,12 +2358,10 @@ static inline bool should_skip_kasan_unpoison(gfp_t flags, bool init_tags)
 		return true;

 	/*
-	 * With hardware tag-based KASAN enabled, skip if either:
-	 *
-	 * 1. Memory tags have already been cleared via tag_clear_highpage().
-	 * 2. Skipping has been requested via __GFP_SKIP_KASAN_UNPOISON.
+	 * With hardware tag-based KASAN enabled, skip if this was requested
+	 * via __GFP_SKIP_KASAN_UNPOISON.
 	 */
-	return init_tags || (flags & __GFP_SKIP_KASAN_UNPOISON);
+	return flags & __GFP_SKIP_KASAN_UNPOISON;
 }

 static inline bool should_skip_init(gfp_t flags)
@@ -2416,7 +2414,7 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
 		/* Note that memory is already initialized by the loop above. */
 		init = false;
 	}
-	if (!should_skip_kasan_unpoison(gfp_flags, init_tags)) {
+	if (!should_skip_kasan_unpoison(gfp_flags)) {
 		/* Unpoison shadow memory or set memory tags. */
 		kasan_unpoison_pages(page, order, init);
 
-------------8<-----------------

With the above, we can wire up page_kasan_tag_reset() to the
__GFP_SKIP_KASAN_UNPOISON check without any additional flags.

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] kasan: Fix ordering between MTE tag colouring and page->flags

[Andrey Konovalov]

On Thu, May 26, 2022 at 2:24 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Wed, May 25, 2022 at 07:41:08PM +0200, Andrey Konovalov wrote:
> > On Wed, May 25, 2022 at 5:45 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
> > > > Adding __GFP_SKIP_KASAN_UNPOISON makes sense, but we still need to
> > > > reset the tag in page->flags.
> > >
> > > My thought was to reset the tag in page->flags based on 'unpoison'
> > > alone without any extra flags. We use this flag for vmalloc() pages but
> > > it seems we don't reset the page tags (as we do via
> > > kasan_poison_slab()).
> >
> > I just realized that we already have __GFP_ZEROTAGS that initializes
> > both in-memory and page->flags tags.
>
> IIUC it only zeroes the tags and skips the unpoisoning but
> page_kasan_tag() remains unchanged.

No, it does page_kasan_tag_reset() via tag_clear_highpage(). At least,
currently.

> > Currently only used for user
> > pages allocated via alloc_zeroed_user_highpage_movable(). Perhaps we
> > can add this flag to GFP_HIGHUSER_MOVABLE?
>
> I wouldn't add __GFP_ZEROTAGS to GFP_HIGHUSER_MOVABLE as we only need it
> if the page is mapped with PROT_MTE. Clearing a page without tags may be
> marginally faster.

Ah, right. We need a dedicated flag for PROT_MTE allocations.

> > We'll also need to change the behavior of __GFP_ZEROTAGS to work even
> > when GFP_ZERO is not set, but this doesn't seem to be a problem.
>
> Why? We'd get unnecessary tag zeroing. We have these cases for
> anonymous, private pages:
>
> 1. Zeroed page allocation without PROT_MTE: we need GFP_ZERO and
>    page_kasan_tag_reset() in case of later mprotect(PROT_MTE).
>
> 2. Zeroed page allocation with PROT_MTE: we need GFP_ZERO,
>    __GFP_ZEROTAGS and page_kasan_tag_reset().
>
> 3. CoW page allocation without PROT_MTE: copy data and we only need
>    page_kasan_tag_reset() in case of later mprotect(PROT_MTE).
>
> 4. CoW page allocation with PROT_MTE: copy data and tags together with
>    page_kasan_tag_reset().
>
> So basically we always need page_kasan_tag_reset() for pages mapped in
> user space even if they are not PROT_MTE, in case of a later
> mprotect(PROT_MTE). For (1), (3) and (4) we don't need to zero the tags.
> For (1) maybe we could do it as part of data zeroing (subject to some
> benchmarks) but for (3) and (4) they'd be overridden by the copy anyway.

Ack.

> > And, at this point, we can probably combine __GFP_ZEROTAGS with
> > __GFP_SKIP_KASAN_POISON, as they both would target user pages.
>
> For user pages, I think we should skip unpoisoning as well. We can keep
> unpoisoning around but if we end up calling page_kasan_tag_reset(),
> there's not much value, at least in page_address() accesses since the
> pointer would match all tags. That's unless you want to detect other
> stray pointers to such pages but we already skip the poisoning on free,
> so it doesn't seem to be a use-case.

Skipping unpoisoning makes sense.

> If we skip unpoisoning (not just poisoning as we already do) for user
> pages, we should reset the tags in page->flags. Whether __GFP_ZEROTAGS
> is passed is complementary, depending on the reason for allocation.

[...]

> Currently if __GFP_ZEROTAGS is passed, the unpoisoning is skipped but I
> think we should have just added __GFP_SKIP_KASAN_UNPOISON instead and
> not add a new argument to should_skip_kasan_unpoison(). If we decide to
> always skip unpoisoning, something like below on top of the vanilla
> kernel:

[...]

> With the above, we can wire up page_kasan_tag_reset() to the
> __GFP_SKIP_KASAN_UNPOISON check without any additional flags.

This would make __GFP_SKIP_KASAN_UNPOISON do two logically unrelated
things: skip setting memory tags and reset page tags. This seems
weird.

I think it makes more sense to split __GFP_ZEROTAGS into
__GFP_ZERO_MEMORY_TAGS and __GFP_ZERO_PAGE_TAGS: the first one does
tag_clear_highpage() without page_kasan_tag_reset() and the second one
does page_kasan_tag_reset() in post_alloc_hook(). Then, add
__GFP_ZERO_PAGE_TAGS to GFP_HIGHUSER_MOVABLE along with
__GFP_SKIP_KASAN_UNPOISON and __GFP_SKIP_KASAN_POISON. And replace
__GFP_ZEROTAGS with __GFP_ZERO_MEMORY_TAGS in
alloc_zeroed_user_highpage_movable().

An a alternative approach that would reduce the number of GFP flags,
we could extend your suggestion and pre-combining all standalone
MTE-related GFP flags based on their use cases:

__GFP_KASAN_VMALLOC == essentially __GFP_SKIP_KASAN_UNPOISON | __GFP_SKIP_ZERO
__GFP_MTE_USER == essentially __GFP_ZERO_PAGE_TAGS |
__GFP_SKIP_KASAN_UNPOISON | __GFP_SKIP_KASAN_POISON
__GFP_MTE_USER_ZERO == essentially __GFP_ZERO_MEMORY_TAGS

Then we would only need 3 flags instead of 5.

However, this seems to be unaligned with the idea that __GFP flags
should enable/disable a single piece of functionality. So I like the
first approach better.

What do you think?

Thanks!

Re: [PATCH 0/3] kasan: Fix ordering between MTE tag colouring and page->flags

[Andrey Konovalov]

On Thu, May 26, 2022 at 2:24 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Wed, May 25, 2022 at 07:41:08PM +0200, Andrey Konovalov wrote:
> > On Wed, May 25, 2022 at 5:45 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
> > > > Adding __GFP_SKIP_KASAN_UNPOISON makes sense, but we still need to
> > > > reset the tag in page->flags.
> > >
> > > My thought was to reset the tag in page->flags based on 'unpoison'
> > > alone without any extra flags. We use this flag for vmalloc() pages but
> > > it seems we don't reset the page tags (as we do via
> > > kasan_poison_slab()).
> >
> > I just realized that we already have __GFP_ZEROTAGS that initializes
> > both in-memory and page->flags tags.
>
> IIUC it only zeroes the tags and skips the unpoisoning but
> page_kasan_tag() remains unchanged.

No, it does page_kasan_tag_reset() via tag_clear_highpage(). At least,
currently.

> > Currently only used for user
> > pages allocated via alloc_zeroed_user_highpage_movable(). Perhaps we
> > can add this flag to GFP_HIGHUSER_MOVABLE?
>
> I wouldn't add __GFP_ZEROTAGS to GFP_HIGHUSER_MOVABLE as we only need it
> if the page is mapped with PROT_MTE. Clearing a page without tags may be
> marginally faster.

Ah, right. We need a dedicated flag for PROT_MTE allocations.

> > We'll also need to change the behavior of __GFP_ZEROTAGS to work even
> > when GFP_ZERO is not set, but this doesn't seem to be a problem.
>
> Why? We'd get unnecessary tag zeroing. We have these cases for
> anonymous, private pages:
>
> 1. Zeroed page allocation without PROT_MTE: we need GFP_ZERO and
>    page_kasan_tag_reset() in case of later mprotect(PROT_MTE).
>
> 2. Zeroed page allocation with PROT_MTE: we need GFP_ZERO,
>    __GFP_ZEROTAGS and page_kasan_tag_reset().
>
> 3. CoW page allocation without PROT_MTE: copy data and we only need
>    page_kasan_tag_reset() in case of later mprotect(PROT_MTE).
>
> 4. CoW page allocation with PROT_MTE: copy data and tags together with
>    page_kasan_tag_reset().
>
> So basically we always need page_kasan_tag_reset() for pages mapped in
> user space even if they are not PROT_MTE, in case of a later
> mprotect(PROT_MTE). For (1), (3) and (4) we don't need to zero the tags.
> For (1) maybe we could do it as part of data zeroing (subject to some
> benchmarks) but for (3) and (4) they'd be overridden by the copy anyway.

Ack.

> > And, at this point, we can probably combine __GFP_ZEROTAGS with
> > __GFP_SKIP_KASAN_POISON, as they both would target user pages.
>
> For user pages, I think we should skip unpoisoning as well. We can keep
> unpoisoning around but if we end up calling page_kasan_tag_reset(),
> there's not much value, at least in page_address() accesses since the
> pointer would match all tags. That's unless you want to detect other
> stray pointers to such pages but we already skip the poisoning on free,
> so it doesn't seem to be a use-case.

Skipping unpoisoning makes sense.

> If we skip unpoisoning (not just poisoning as we already do) for user
> pages, we should reset the tags in page->flags. Whether __GFP_ZEROTAGS
> is passed is complementary, depending on the reason for allocation.

[...]

> Currently if __GFP_ZEROTAGS is passed, the unpoisoning is skipped but I
> think we should have just added __GFP_SKIP_KASAN_UNPOISON instead and
> not add a new argument to should_skip_kasan_unpoison(). If we decide to
> always skip unpoisoning, something like below on top of the vanilla
> kernel:

[...]

> With the above, we can wire up page_kasan_tag_reset() to the
> __GFP_SKIP_KASAN_UNPOISON check without any additional flags.

This would make __GFP_SKIP_KASAN_UNPOISON do two logically unrelated
things: skip setting memory tags and reset page tags. This seems
weird.

I think it makes more sense to split __GFP_ZEROTAGS into
__GFP_ZERO_MEMORY_TAGS and __GFP_ZERO_PAGE_TAGS: the first one does
tag_clear_highpage() without page_kasan_tag_reset() and the second one
does page_kasan_tag_reset() in post_alloc_hook(). Then, add
__GFP_ZERO_PAGE_TAGS to GFP_HIGHUSER_MOVABLE along with
__GFP_SKIP_KASAN_UNPOISON and __GFP_SKIP_KASAN_POISON. And replace
__GFP_ZEROTAGS with __GFP_ZERO_MEMORY_TAGS in
alloc_zeroed_user_highpage_movable().

An a alternative approach that would reduce the number of GFP flags,
we could extend your suggestion and pre-combining all standalone
MTE-related GFP flags based on their use cases:

__GFP_KASAN_VMALLOC == essentially __GFP_SKIP_KASAN_UNPOISON | __GFP_SKIP_ZERO
__GFP_MTE_USER == essentially __GFP_ZERO_PAGE_TAGS |
__GFP_SKIP_KASAN_UNPOISON | __GFP_SKIP_KASAN_POISON
__GFP_MTE_USER_ZERO == essentially __GFP_ZERO_MEMORY_TAGS

Then we would only need 3 flags instead of 5.

However, this seems to be unaligned with the idea that __GFP flags
should enable/disable a single piece of functionality. So I like the
first approach better.

What do you think?

Thanks!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] kasan: Fix ordering between MTE tag colouring and page->flags

[Catalin Marinas]

Hi Andrey,

Sorry, I got distracted by the merging window.

On Tue, May 31, 2022 at 07:16:03PM +0200, Andrey Konovalov wrote:
> On Thu, May 26, 2022 at 2:24 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
> > If we skip unpoisoning (not just poisoning as we already do) for user
> > pages, we should reset the tags in page->flags. Whether __GFP_ZEROTAGS
> > is passed is complementary, depending on the reason for allocation.
> 
> [...]
> 
> > Currently if __GFP_ZEROTAGS is passed, the unpoisoning is skipped but I
> > think we should have just added __GFP_SKIP_KASAN_UNPOISON instead and
> > not add a new argument to should_skip_kasan_unpoison(). If we decide to
> > always skip unpoisoning, something like below on top of the vanilla
> > kernel:
> 
> [...]
> 
> > With the above, we can wire up page_kasan_tag_reset() to the
> > __GFP_SKIP_KASAN_UNPOISON check without any additional flags.
> 
> This would make __GFP_SKIP_KASAN_UNPOISON do two logically unrelated
> things: skip setting memory tags and reset page tags. This seems
> weird.

Not entirely weird, it depends on how you look at it. After allocation,
you expect the accesses to page_address() to work, irrespective of the
GFP flags. __kasan_unpoison_pages() ensures that the page->flags match
the written tag without a new GFP flag to set the page->flags. If you
skip the unpoisoning something should reset the page->flags tag to
ensure an accessible page_address(). I find it weirder that you need
another GFP flag to pretty much say 'give me an accessible page'.

> I think it makes more sense to split __GFP_ZEROTAGS into
> __GFP_ZERO_MEMORY_TAGS and __GFP_ZERO_PAGE_TAGS: the first one does
> tag_clear_highpage() without page_kasan_tag_reset() and the second one
> does page_kasan_tag_reset() in post_alloc_hook(). Then, add
> __GFP_ZERO_PAGE_TAGS to GFP_HIGHUSER_MOVABLE along with
> __GFP_SKIP_KASAN_UNPOISON and __GFP_SKIP_KASAN_POISON. And replace
> __GFP_ZEROTAGS with __GFP_ZERO_MEMORY_TAGS in
> alloc_zeroed_user_highpage_movable().

As above, my preference would be to avoid a new flag, just wire this up
to __GFP_SKIP_KASAN_UNPOISON. But if you do want fine-grained control, I
can add the above.

Thanks.

-- 
Catalin

Re: [PATCH 0/3] kasan: Fix ordering between MTE tag colouring and page->flags

[Catalin Marinas]

Hi Andrey,

Sorry, I got distracted by the merging window.

On Tue, May 31, 2022 at 07:16:03PM +0200, Andrey Konovalov wrote:
> On Thu, May 26, 2022 at 2:24 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
> > If we skip unpoisoning (not just poisoning as we already do) for user
> > pages, we should reset the tags in page->flags. Whether __GFP_ZEROTAGS
> > is passed is complementary, depending on the reason for allocation.
> 
> [...]
> 
> > Currently if __GFP_ZEROTAGS is passed, the unpoisoning is skipped but I
> > think we should have just added __GFP_SKIP_KASAN_UNPOISON instead and
> > not add a new argument to should_skip_kasan_unpoison(). If we decide to
> > always skip unpoisoning, something like below on top of the vanilla
> > kernel:
> 
> [...]
> 
> > With the above, we can wire up page_kasan_tag_reset() to the
> > __GFP_SKIP_KASAN_UNPOISON check without any additional flags.
> 
> This would make __GFP_SKIP_KASAN_UNPOISON do two logically unrelated
> things: skip setting memory tags and reset page tags. This seems
> weird.

Not entirely weird, it depends on how you look at it. After allocation,
you expect the accesses to page_address() to work, irrespective of the
GFP flags. __kasan_unpoison_pages() ensures that the page->flags match
the written tag without a new GFP flag to set the page->flags. If you
skip the unpoisoning something should reset the page->flags tag to
ensure an accessible page_address(). I find it weirder that you need
another GFP flag to pretty much say 'give me an accessible page'.

> I think it makes more sense to split __GFP_ZEROTAGS into
> __GFP_ZERO_MEMORY_TAGS and __GFP_ZERO_PAGE_TAGS: the first one does
> tag_clear_highpage() without page_kasan_tag_reset() and the second one
> does page_kasan_tag_reset() in post_alloc_hook(). Then, add
> __GFP_ZERO_PAGE_TAGS to GFP_HIGHUSER_MOVABLE along with
> __GFP_SKIP_KASAN_UNPOISON and __GFP_SKIP_KASAN_POISON. And replace
> __GFP_ZEROTAGS with __GFP_ZERO_MEMORY_TAGS in
> alloc_zeroed_user_highpage_movable().

As above, my preference would be to avoid a new flag, just wire this up
to __GFP_SKIP_KASAN_UNPOISON. But if you do want fine-grained control, I
can add the above.

Thanks.

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/3] kasan: Fix ordering between MTE tag colouring and page->flags

[Andrey Konovalov]

Hi Catalin,

On Thu, Jun 9, 2022 at 8:32 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> > This would make __GFP_SKIP_KASAN_UNPOISON do two logically unrelated
> > things: skip setting memory tags and reset page tags. This seems
> > weird.
>
> Not entirely weird, it depends on how you look at it. After allocation,
> you expect the accesses to page_address() to work, irrespective of the
> GFP flags. __kasan_unpoison_pages() ensures that the page->flags match
> the written tag without a new GFP flag to set the page->flags. If you
> skip the unpoisoning something should reset the page->flags tag to
> ensure an accessible page_address(). I find it weirder that you need
> another GFP flag to pretty much say 'give me an accessible page'.

Hm, this makes sense.

> As above, my preference would be to avoid a new flag, just wire this up
> to __GFP_SKIP_KASAN_UNPOISON. But if you do want fine-grained control, I
> can add the above.

OK, let's do as you suggest.

Thanks!

Re: [PATCH 0/3] kasan: Fix ordering between MTE tag colouring and page->flags

[Andrey Konovalov]

Hi Catalin,

On Thu, Jun 9, 2022 at 8:32 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> > This would make __GFP_SKIP_KASAN_UNPOISON do two logically unrelated
> > things: skip setting memory tags and reset page tags. This seems
> > weird.
>
> Not entirely weird, it depends on how you look at it. After allocation,
> you expect the accesses to page_address() to work, irrespective of the
> GFP flags. __kasan_unpoison_pages() ensures that the page->flags match
> the written tag without a new GFP flag to set the page->flags. If you
> skip the unpoisoning something should reset the page->flags tag to
> ensure an accessible page_address(). I find it weirder that you need
> another GFP flag to pretty much say 'give me an accessible page'.

Hm, this makes sense.

> As above, my preference would be to avoid a new flag, just wire this up
> to __GFP_SKIP_KASAN_UNPOISON. But if you do want fine-grained control, I
> can add the above.

OK, let's do as you suggest.

Thanks!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel