[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(-)

[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...@arm.com>
Cc: Andrey Ryabinin <ryabin...@gmail.com>
Cc: Andrey Konovalov <andre...@gmail.com>
Cc: Vincenzo Frascino <vincenzo...@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);
}

[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...@arm.com>
Cc: Will Deacon <wi...@kernel.org>
Cc: Vincenzo Frascino <vincenzo...@arm.com>
Cc: Andrey Konovalov <andre...@gmail.com>
Cc: Peter Collingbourne <p...@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;

[Andrey Konovalov]

On Tue, May 17, 2022 at 8:09 PM Catalin Marinas <catalin...@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!

[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...@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

[Andrey Konovalov]

On Tue, May 17, 2022 at 8:09 PM Catalin Marinas <catalin...@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...@arm.com>
> Cc: Andrey Ryabinin <ryabin...@gmail.com>
> Cc: Andrey Konovalov <andre...@gmail.com>
> Cc: Vincenzo Frascino <vincenzo...@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.

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.

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.

[Andrey Konovalov]

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

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

[Andrey Konovalov]

On Fri, May 20, 2022 at 3:01 PM Catalin Marinas <catalin...@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!

[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...@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

[Andrey Konovalov]

On Wed, May 25, 2022 at 5:45 PM Catalin Marinas <catalin...@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?

[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...@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

[Andrey Konovalov]

On Thu, May 26, 2022 at 2:24 PM Catalin Marinas <catalin...@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...@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!

[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...@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

[Andrey Konovalov]

Hi Catalin,

On Thu, Jun 9, 2022 at 8:32 PM Catalin Marinas <catalin...@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!

[Catalin Marinas]

Hi,

That's a second attempt on fixing the race race between setting the
allocation (in-memory) tags in a page and the corresponding logical tag
in page->flags. Initial version here:

https://lore.kernel.org/r/20220517180945.7563...@arm.com

This new series does not introduce any new GFP flags but instead always
skips unpoisoning of the user pages (we already skip the poisoning on
free). Any unpoisoned page will have the page->flags tag reset.

For the background:

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 a page is mapped in user-space with PROT_MTE, the architecture code
will set the allocation 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, to do this at
page allocation time when __GFP_SKIP_KASAN_UNPOISON is passed.

Thanks.

Catalin Marinas (4):
mm: kasan: Ensure the tags are visible before the tag in page->flags
mm: kasan: Skip unpoisoning of user pages
mm: kasan: Skip page unpoisoning only if __GFP_SKIP_KASAN_UNPOISON

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 | 2 +-
mm/kasan/common.c | 3 ++-
mm/page_alloc.c | 19 ++++++++++---------
8 files changed, 13 insertions(+), 44 deletions(-)

[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...@arm.com>
Reviewed-by: Andrey Konovalov <andre...@gmail.com>
Cc: Andrey Ryabinin <ryabin...@gmail.com>
Cc: Vincenzo Frascino <vincenzo...@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 c40c0e7b3b5f..78be2beb7453 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)

[Catalin Marinas]

Commit c275c5c6d50a ("kasan: disable freed user page poisoning with HW
tags") added __GFP_SKIP_KASAN_POISON to GFP_HIGHUSER_MOVABLE. A similar
argument can be made about unpoisoning, so also add
__GFP_SKIP_KASAN_UNPOISON to user pages. To ensure the user page is
still accessible via page_address() without a kasan fault, reset the
page->flags tag.

With the above changes, there is no need for the arm64
tag_clear_highpage() to reset the page->flags tag.

Signed-off-by: Catalin Marinas <catalin...@arm.com>
Cc: Andrey Ryabinin <ryabin...@gmail.com>

Cc: Andrey Konovalov <andre...@gmail.com>
Cc: Peter Collingbourne <p...@google.com>

Cc: Vincenzo Frascino <vincenzo...@arm.com>
---

arch/arm64/mm/fault.c | 1 -
include/linux/gfp.h | 2 +-
mm/page_alloc.c | 7 +++++--
3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index c5e11768e5c1..cdf3ffa0c223 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -927,6 +927,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/include/linux/gfp.h b/include/linux/gfp.h
index 2d2ccae933c2..0ace7759acd2 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 e008a3df0485..f6ed240870bc 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2397,6 +2397,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);

@@ -2422,8 +2423,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);

@@ -2438,6 +2437,10 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
/* Note that memory is already initialized by KASAN. */
if (kasan_has_integrated_init())
init = false;
+ } else {
+ /* Ensure page_address() dereferencing does not fault. */

+ for (i = 0; i != 1 << order; ++i)
+ page_kasan_tag_reset(page + i);
}

/* If memory is still not initialized, do it now. */
if (init)

[Catalin Marinas]

Currently post_alloc_hook() skips the kasan unpoisoning if the tags will
be zeroed (__GFP_ZEROTAGS) or __GFP_SKIP_KASAN_UNPOISON is passed. Since
__GFP_ZEROTAGS is now accompanied by __GFP_SKIP_KASAN_UNPOISON, remove
the extra check.

Signed-off-by: Catalin Marinas <catalin...@arm.com>
Cc: Andrey Ryabinin <ryabin...@gmail.com>
Cc: Andrey Konovalov <andre...@gmail.com>
Cc: Peter Collingbourne <p...@google.com>
Cc: Vincenzo Frascino <vincenzo...@arm.com>
---

mm/page_alloc.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index f6ed240870bc..bf45a6aa407a 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2361,7 +2361,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) ||

@@ -2373,12 +2373,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 has been
+ * 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)

@@ -2430,7 +2428,7 @@ inline void post_alloc_hook(struct page *page, unsigned int order,

[Catalin Marinas]

This reverts commit e5b8d9218951e59df986f627ec93569a0d22149b.

Pages mapped in user-space with PROT_MTE have the allocation tags either
zeroed or copied/restored to some user values. In order for the kernel
to access such pages via page_address(), resetting the tag in
page->flags was necessary. This tag resetting was deferred to
set_pte_at() -> mte_sync_page_tags() but it can race with another CPU
reading the flags (via page_to_virt()):

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 now the post_alloc_hook() function resets the page->flags tag when
unpoisoning is skipped for user pages (including the __GFP_ZEROTAGS
case), revert the arm64 commit calling page_kasan_tag_reset().

Signed-off-by: Catalin Marinas <catalin...@arm.com>

Cc: Will Deacon <wi...@kernel.org>
Cc: Vincenzo Frascino <vincenzo...@arm.com>

Cc: Andrey Konovalov <andre...@gmail.com>
Cc: Peter Collingbourne <p...@google.com>

---

arch/arm64/kernel/hibernate.c | 5 -----
arch/arm64/kernel/mte.c | 9 ---------
arch/arm64/mm/copypage.c | 9 ---------

arch/arm64/mm/mteswap.c | 9 ---------
4 files changed, 32 deletions(-)

diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c
index 2e248342476e..af5df48ba915 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 57b30bcf9f21..7ba4d6fd1f72 100644
--- a/arch/arm64/kernel/mte.c
+++ b/arch/arm64/kernel/mte.c
@@ -48,15 +48,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 0dea80bf6de4..24913271e898 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);
}
}

[Andrey Konovalov]

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

[Andrey Konovalov]

On Fri, Jun 10, 2022 at 5:21 PM Catalin Marinas <catalin...@arm.com> wrote:
>

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

[Andrey Konovalov]

On Fri, Jun 10, 2022 at 5:21 PM Catalin Marinas <catalin...@arm.com> wrote:
>

Acked-by: Andrey Konovalov <andre...@gmail.com>

[Vincenzo Frascino]

On 6/10/22 16:21, Catalin Marinas 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...@arm.com>
> Reviewed-by: Andrey Konovalov <andre...@gmail.com>
> Cc: Andrey Ryabinin <ryabin...@gmail.com>
> Cc: Vincenzo Frascino <vincenzo...@arm.com>

Reviewed-by: Vincenzo Frascino <vincenzo...@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 c40c0e7b3b5f..78be2beb7453 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)

--
Regards,
Vincenzo

[Vincenzo Frascino]

On 6/10/22 16:21, Catalin Marinas wrote:

Nit: Since "i" is not used outside of the for loop context we could use the
contract form "for (int i = 0; ..." which is allowed by C11.

> set_page_private(page, 0);
> set_page_refcounted(page);
> @@ -2422,8 +2423,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);
> @@ -2438,6 +2437,10 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
> /* Note that memory is already initialized by KASAN. */
> if (kasan_has_integrated_init())
> init = false;
> + } else {
> + /* Ensure page_address() dereferencing does not fault. */
> + for (i = 0; i != 1 << order; ++i)
> + page_kasan_tag_reset(page + i);
> }
> /* If memory is still not initialized, do it now. */
> if (init)

Either way:

Reviewed-by: Vincenzo Frascino <vincenzo...@arm.com>

--
Regards,
Vincenzo

[Vincenzo Frascino]

On 6/10/22 16:21, Catalin Marinas wrote:

> Currently post_alloc_hook() skips the kasan unpoisoning if the tags will
> be zeroed (__GFP_ZEROTAGS) or __GFP_SKIP_KASAN_UNPOISON is passed. Since
> __GFP_ZEROTAGS is now accompanied by __GFP_SKIP_KASAN_UNPOISON, remove
> the extra check.
>
> Signed-off-by: Catalin Marinas <catalin...@arm.com>
> Cc: Andrey Ryabinin <ryabin...@gmail.com>
> Cc: Andrey Konovalov <andre...@gmail.com>
> Cc: Peter Collingbourne <p...@google.com>
> Cc: Vincenzo Frascino <vincenzo...@arm.com>

Reviewed-by: Vincenzo Frascino <vincenzo...@arm.com>

--
Regards,
Vincenzo

[Vincenzo Frascino]

On 6/10/22 16:21, Catalin Marinas wrote:

> This reverts commit e5b8d9218951e59df986f627ec93569a0d22149b.
>
> Pages mapped in user-space with PROT_MTE have the allocation tags either
> zeroed or copied/restored to some user values. In order for the kernel
> to access such pages via page_address(), resetting the tag in
> page->flags was necessary. This tag resetting was deferred to
> set_pte_at() -> mte_sync_page_tags() but it can race with another CPU
> reading the flags (via page_to_virt()):
>
> 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 now the post_alloc_hook() function resets the page->flags tag when
> unpoisoning is skipped for user pages (including the __GFP_ZEROTAGS
> case), revert the arm64 commit calling page_kasan_tag_reset().
>
> Signed-off-by: Catalin Marinas <catalin...@arm.com>
> Cc: Will Deacon <wi...@kernel.org>
> Cc: Vincenzo Frascino <vincenzo...@arm.com>
> Cc: Andrey Konovalov <andre...@gmail.com>
> Cc: Peter Collingbourne <p...@google.com>

Reviewed-by: Vincenzo Frascino <vincenzo...@arm.com>

--
Regards,
Vincenzo

[Catalin Marinas]

On Thu, Jun 16, 2022 at 09:42:10AM +0100, Vincenzo Frascino wrote:
> On 6/10/22 16:21, Catalin Marinas wrote:
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index e008a3df0485..f6ed240870bc 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -2397,6 +2397,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;
> >
>
> Nit: Since "i" is not used outside of the for loop context we could use the
> contract form "for (int i = 0; ..." which is allowed by C11.

Oh yeah, Linux moved to C11 in 5.18. But it looks better to be
consistent with the other for loop in this file.

> Reviewed-by: Vincenzo Frascino <vincenzo...@arm.com>

Thanks.

--
Catalin

[Will Deacon]

This looks good to me, but after reading the cover letter I'm wondering
whether the try_cmpxchg() in page_kasan_tag_set() could be relaxed to
try_cmpxchg_release() as a separate optimisation?

Will

[Will Deacon]

On Fri, 10 Jun 2022 16:21:37 +0100, Catalin Marinas wrote:
> That's a second attempt on fixing the race race between setting the
> allocation (in-memory) tags in a page and the corresponding logical tag
> in page->flags. Initial version here:
>
> https://lore.kernel.org/r/20220517180945.7563...@arm.com
>
> This new series does not introduce any new GFP flags but instead always
> skips unpoisoning of the user pages (we already skip the poisoning on
> free). Any unpoisoned page will have the page->flags tag reset.
>

> [...]

Applied to arm64 (for-next/mte), thanks!

[1/4] mm: kasan: Ensure the tags are visible before the tag in page->flags
https://git.kernel.org/arm64/c/ed0a6d1d973e
[2/4] mm: kasan: Skip unpoisoning of user pages
https://git.kernel.org/arm64/c/70c248aca9e7
[3/4] mm: kasan: Skip page unpoisoning only if __GFP_SKIP_KASAN_UNPOISON
https://git.kernel.org/arm64/c/6d05141a3930
[4/4] arm64: kasan: Revert "arm64: mte: reset the page tag in page->flags"
https://git.kernel.org/arm64/c/20794545c146

Cheers,
--
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

[Catalin Marinas]

I think it can be a try_cmpxchg_release() (I did not realise we have
one). I'll post a patch later today.

--
Catalin

[Will Deacon]

I've picked this up, thanks.

An alternative solution might be to use a seqlock (if you can find somewhere
to put it) so that virt_to_page() spins briefly while the tags and flags
are being updated.

Will

[Kuan-Ying Lee (李冠穎)]

Hi kasan maintainers,

We hit the following issue on the android-6.1 devices with MTE and HW
tag kasan enabled.

I observe that the anon flag doesn't have skip_kasan_poison and
skip_kasan_unpoison flag and kasantag is weird.

AFAIK, kasantag of anon flag needs to be 0x0.

[ 71.953938] [T1403598] FramePolicy:
[name:report&]=========================================================
=========
[ 71.955305] [T1403598] FramePolicy: [name:report&]BUG: KASAN:
invalid-access in copy_page+0x10/0xd0
[ 71.956476] [T1403598] FramePolicy: [name:report&]Read at addr
f0ffff81332a8000 by task FramePolicy/3598
[ 71.957673] [T1403598] FramePolicy: [name:report_hw_tags&]Pointer
tag: [f0], memory tag: [ff]
[ 71.958746] [T1403598] FramePolicy: [name:report&]
[ 71.959354] [T1403598] FramePolicy: CPU: 4 PID: 3598 Comm:
FramePolicy Tainted: G S W OE 6.1.0-mainline-android14-0-
ga8a53f83b9e4 #1
[ 71.960978] [T1403598] FramePolicy: Hardware name: MT6985(ENG) (DT)
[ 71.961767] [T1403598] FramePolicy: Call trace:
[ 71.962338] [T1403598] FramePolicy: dump_backtrace+0x108/0x158
[ 71.963097] [T1403598] FramePolicy: show_stack+0x20/0x48
[ 71.963782] [T1403598] FramePolicy: dump_stack_lvl+0x6c/0x88
[ 71.964512] [T1403598] FramePolicy: print_report+0x2cc/0xa64
[ 71.965263] [T1403598] FramePolicy: kasan_report+0xb8/0x138
[ 71.965986] [T1403598] FramePolicy: __do_kernel_fault+0xd4/0x248
[ 71.966782] [T1403598] FramePolicy: do_bad_area+0x38/0xe8
[ 71.967484] [T1403598] FramePolicy: do_tag_check_fault+0x24/0x38
[ 71.968261] [T1403598] FramePolicy: do_mem_abort+0x48/0xb0
[ 71.968973] [T1403598] FramePolicy: el1_abort+0x44/0x68
[ 71.969646] [T1403598] FramePolicy: el1h_64_sync_handler+0x68/0xb8
[ 71.970440] [T1403598] FramePolicy: el1h_64_sync+0x68/0x6c
[ 71.971146] [T1403598] FramePolicy: copy_page+0x10/0xd0
[ 71.971824] [T1403598] FramePolicy: copy_user_highpage+0x20/0x40
[ 71.972603] [T1403598] FramePolicy: wp_page_copy+0xd0/0x9f8
[ 71.973344] [T1403598] FramePolicy: do_wp_page+0x374/0x3b0
[ 71.974056] [T1403598] FramePolicy: handle_mm_fault+0x3ec/0x119c
[ 71.974833] [T1403598] FramePolicy: do_page_fault+0x344/0x4ac
[ 71.975583] [T1403598] FramePolicy: do_mem_abort+0x48/0xb0
[ 71.976294] [T1403598] FramePolicy: el0_da+0x4c/0xe0
[ 71.976934] [T1403598] FramePolicy: el0t_64_sync_handler+0xd4/0xfc
[ 71.977725] [T1403598] FramePolicy: el0t_64_sync+0x1a0/0x1a4
[ 71.978451] [T1403598] FramePolicy: [name:report&]
[ 71.979057] [T1403598] FramePolicy: [name:report&]The buggy address
belongs to the physical page:
[ 71.980173] [T1403598] FramePolicy:
[name:debug&]page:fffffffe04ccaa00 refcount:14 mapcount:13
mapping:0000000000000000 index:0x7884c74 pfn:0x1732a8
[ 71.981849] [T1403598] FramePolicy:
[name:debug&]memcg:faffff80c0241000
[ 71.982680] [T1403598] FramePolicy: [name:debug&]anon flags:
0x43c000000048003e(referenced|uptodate|dirty|lru|active|swapbacked|arch
_2|zone=1|kasantag=0xf)
[ 71.984446] [T1403598] FramePolicy: raw: 43c000000048003e
fffffffe04b99648 fffffffe04cca308 f2ffff8103390831
[ 71.985684] [T1403598] FramePolicy: raw: 0000000007884c74
0000000000000000 0000000e0000000c faffff80c0241000
[ 71.986919] [T1403598] FramePolicy: [name:debug&]page dumped
because: kasan: bad access detected
[ 71.988022] [T1403598] FramePolicy: [name:report&]
[ 71.988624] [T1403598] FramePolicy: [name:report&]Memory state
around the buggy address:
[ 71.989641] [T1403598] FramePolicy: ffffff81332a7e00: fe fe fe fe
fe fe fe fe fe fe fe fe fe fe fe fe
[ 71.990811] [T1403598] FramePolicy: ffffff81332a7f00: fe fe fe fe
fe fe fe fe fe fe fe fe fe fe fe fe
[ 71.991982] [T1403598] FramePolicy: >ffffff81332a8000: ff ff ff ff
f0 f0 fc fc fc fc fc fc fc f0 f0 f3
[ 71.993149] [T1403598] FramePolicy:
[name:report&] ^
[ 71.993972] [T1403598] FramePolicy: ffffff81332a8100: f3 f3 f3 f3
f3 f3 f0 f0 f8 f8 f8 f8 f8 f8 f8 f0
[ 71.995141] [T1403598] FramePolicy: ffffff81332a8200: f0 fb fb fb
fb fb fb fb f0 f0 fe fe fe fe fe fe
[ 71.996332] [T1403598] FramePolicy:
[name:report&]=========================================================
=========

Originally, I suspect that some userspace pages have been migrated so
the page->flags will be lost and page->flags is re-generated by
alloc_pages().

I try the following diff, but it didn't help.

diff --git a/mm/migrate.c b/mm/migrate.c
index dff333593a8a..ed2065908418 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -51,6 +51,7 @@
#include <linux/random.h>
#include <linux/sched/sysctl.h>
#include <linux/memory-tiers.h>
+#include <linux/kasan.h>

#include <asm/tlbflush.h>

@@ -611,6 +612,14 @@ void folio_migrate_flags(struct folio *newfolio,
struct folio *folio)

if (!folio_test_hugetlb(folio))
mem_cgroup_migrate(folio, newfolio);
+
+#ifdef CONFIG_KASAN_HW_TAGS
+ if (kasan_hw_tags_enabled()) {
+ if (folio_test_skip_kasan_poison(folio))
+ folio_set_skip_kasan_poison(newfolio);
+ page_kasan_tag_set(&newfolio->page,
page_kasan_tag(&folio->page));
+ }
+#endif
}
EXPORT_SYMBOL(folio_migrate_flags);


After I revert this patchset (4 patches), this issue disappear.

>

[Andrey Konovalov]

Hi Kuan-Ying,

There recently was a similar crash due to incorrectly implemented sampling.

Do you have the following patch in your tree?

https://android.googlesource.com/kernel/common/+/9f7f5a25f335e6e1484695da9180281a728db7e2

If not, please sync your 6.1 tree with the Android common kernel.
Hopefully this will fix the issue.

Thanks!

[Kuan-Ying Lee (李冠穎)]

https://urldefense.com/v3/__https://android.googlesource.com/kernel/common/*/9f7f5a25f335e6e1484695da9180281a728db7e2__;Kw!!CTRNKA9wMg0ARbw!hUjRlXirPMSusdIWe0RIPt0PNqIHYDCJyd7GSd4o-TgLMP0CKRUkjElH-jcvtaz42-sgE2U58964rCCbuNTJE5Jx$ 

>
>
> If not, please sync your 6.1 tree with the Android common kernel.
> Hopefully this will fix the issue.
>
> Thanks!

Hi Andrey,

Thanks for your advice.

I saw this patch is to fix ("kasan: allow sampling page_alloc
allocations for HW_TAGS").

But our 6.1 tree doesn't have following two commits now.
("FROMGIT: kasan: allow sampling page_alloc allocations for HW_TAGS")
(FROMLIST: kasan: reset page tags properly with sampling)

[Andrey Konovalov]

Hi Kuan-Ying,

Just to clarify: these two patches were applied twice: once here on Jan 13:

https://android.googlesource.com/kernel/common/+/a2a9e34d164e90fc08d35fd097a164b9101d72ef
https://android.googlesource.com/kernel/common/+/435e2a6a6c8ba8d0eb55f9aaade53e7a3957322b

but then reverted here on Jan 20:

https://android.googlesource.com/kernel/common/+/5503dbe454478fe54b9cac3fc52d4477f52efdc9
https://android.googlesource.com/kernel/common/+/4573a3cf7e18735a477845426238d46d96426bb6

And then once again via the link I sent before together with a fix on Jan 25.

It might be that you still have to former two patches in your tree if
you synced it before the revert.

However, if this is not the case:

Which 6.1 commit is your tree based on?
Do you have any private MTE-related changes in the kernel?
Do you have userspace MTE enabled?

Thanks!

[Qun-wei Lin (林群崴)]

On Fri, 2023-02-03 at 18:51 +0100, Andrey Konovalov wrote:
> On Fri, Feb 3, 2023 at 4:41 AM Kuan-Ying Lee (李冠穎)
> <Kuan-Y...@mediatek.com> wrote:
> > 
> > > Hi Kuan-Ying,
> > > 
> > > There recently was a similar crash due to incorrectly implemented
> > > sampling.
> > > 
> > > Do you have the following patch in your tree?
> > > 
> > > 
> > 
> > 
https://urldefense.com/v3/__https://android.googlesource.com/kernel/common/*/9f7f5a25f335e6e1484695da9180281a728db7e2__;Kw!!CTRNKA9wMg0ARbw!hUjRlXirPMSusdIWe0RIPt0PNqIHYDCJyd7GSd4o-TgLMP0CKRUkjElH-jcvtaz42-sgE2U58964rCCbuNTJE5Jx$

> > > 
> > > 
> > > If not, please sync your 6.1 tree with the Android common kernel.
> > > Hopefully this will fix the issue.
> > > 
> > > Thanks!
> > 
> > Hi Andrey,
> > 
> > Thanks for your advice.
> > 
> > I saw this patch is to fix ("kasan: allow sampling page_alloc
> > allocations for HW_TAGS").
> > 
> > But our 6.1 tree doesn't have following two commits now.
> > ("FROMGIT: kasan: allow sampling page_alloc allocations for
> > HW_TAGS")
> > (FROMLIST: kasan: reset page tags properly with sampling)
> 
> Hi Kuan-Ying,
> 

Hi Andrey,
I'll stand in for Kuan-Ying as he's out of office.
Thanks for your help!

> Just to clarify: these two patches were applied twice: once here on
> Jan 13:
> 
> 
https://urldefense.com/v3/__https://android.googlesource.com/kernel/common/*/a2a9e34d164e90fc08d35fd097a164b9101d72ef__;Kw!!CTRNKA9wMg0ARbw!kE1XiSmunRcQb9rTpKGkFc1EFJA57qr1cj7v9EZAjUBzXcSzMl-ofCI2mdtEQsxn3J4n7Lkgxb0_G745_3oO-3k$ 
>  
> 
https://urldefense.com/v3/__https://android.googlesource.com/kernel/common/*/435e2a6a6c8ba8d0eb55f9aaade53e7a3957322b__;Kw!!CTRNKA9wMg0ARbw!kE1XiSmunRcQb9rTpKGkFc1EFJA57qr1cj7v9EZAjUBzXcSzMl-ofCI2mdtEQsxn3J4n7Lkgxb0_G745sDEOYWY$ 
>  
> 

Our codebase does not contain these two patches.

> but then reverted here on Jan 20:
> 
> 
https://urldefense.com/v3/__https://android.googlesource.com/kernel/common/*/5503dbe454478fe54b9cac3fc52d4477f52efdc9__;Kw!!CTRNKA9wMg0ARbw!kE1XiSmunRcQb9rTpKGkFc1EFJA57qr1cj7v9EZAjUBzXcSzMl-ofCI2mdtEQsxn3J4n7Lkgxb0_G745Bl77dFY$ 
>  
> 
https://urldefense.com/v3/__https://android.googlesource.com/kernel/common/*/4573a3cf7e18735a477845426238d46d96426bb6__;Kw!!CTRNKA9wMg0ARbw!kE1XiSmunRcQb9rTpKGkFc1EFJA57qr1cj7v9EZAjUBzXcSzMl-ofCI2mdtEQsxn3J4n7Lkgxb0_G745K-J8O-w$ 
>  
> 
> And then once again via the link I sent before together with a fix on
> Jan 25.
> 
> It might be that you still have to former two patches in your tree if
> you synced it before the revert.
> 
> However, if this is not the case:
> 
> Which 6.1 commit is your tree based on?


https://android.googlesource.com/kernel/common/+/53b3a7721b7aec74d8fa2ee55c2480044cc7c1b8
(53b3a77 Merge 6.1.1 into android14-6.1) is the latest commit in our
tree.

> Do you have any private MTE-related changes in the kernel?

No, all the MTE-related code is the same as Android Common Kernel.

> Do you have userspace MTE enabled?

Yes, we have enabled MTE for both EL1 and EL0.

> 
> Thanks!

************* MEDIATEK Confidentiality Notice ********************
The information contained in this e-mail message (including any 
attachments) may be confidential, proprietary, privileged, or otherwise
exempt from disclosure under applicable laws. It is intended to be 
conveyed only to the designated recipient(s). Any use, dissemination, 
distribution, printing, retaining or copying of this e-mail (including its 
attachments) by unintended recipient(s) is strictly prohibited and may 
be unlawful. If you are not an intended recipient of this e-mail, or believe 
that you have received this e-mail in error, please notify the sender 
immediately (by replying to this e-mail), delete any and all copies of 
this e-mail (including any attachments) from your system, and do not
disclose the content of this e-mail to any other person. Thank you!

[Peter Collingbourne]

Hi Qun-wei,

Thanks for the information. We encountered a similar issue internally
with the Android 5.15 common kernel. We tracked it down to an issue
with page migration, where the source page was a userspace page with
MTE tags, and the target page was allocated using KASAN (i.e. having
a non-zero KASAN tag). This caused tag check faults when the page was
subsequently accessed by the kernel as a result of the mismatching tags
from userspace. Given the number of different ways that page migration
target pages can be allocated, the simplest fix that we could think of
was to synchronize the KASAN tag in copy_highpage().

Can you try the patch below and let us know whether it fixes the issue?

diff --git a/arch/arm64/mm/copypage.c b/arch/arm64/mm/copypage.c
index 24913271e898c..87ed38e9747bd 100644
--- a/arch/arm64/mm/copypage.c
+++ b/arch/arm64/mm/copypage.c
@@ -23,6 +23,8 @@ 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);

+ if (kasan_hw_tags_enabled())
+ page_kasan_tag_set(to, page_kasan_tag(from));
mte_copy_page_tags(kto, kfrom);
}
}

Catalin, please let us know what you think of the patch above. It
effectively partially undoes commit 20794545c146 ("arm64: kasan: Revert
"arm64: mte: reset the page tag in page->flags""), but this seems okay
to me because the mentioned race condition shouldn't affect "new" pages
such as those being used as migration targets. The smp_wmb() that was
there before doesn't seem necessary for the same reason.

If the patch is okay, we should apply it to the 6.1 stable kernel. The
problem appears to be "fixed" in the mainline kernel because of
a bad merge conflict resolution on my part; when I rebased commit
e059853d14ca ("arm64: mte: Fix/clarify the PG_mte_tagged semantics")
past commit 20794545c146, it looks like I accidentally brought back the
page_kasan_tag_reset() line removed in the latter. But we should align
the mainline kernel with whatever we decide to do on 6.1.

Peter

[Catalin Marinas]

Hi Peter,

On Thu, Feb 09, 2023 at 10:19:20PM -0800, Peter Collingbourne wrote:
> Thanks for the information. We encountered a similar issue internally
> with the Android 5.15 common kernel. We tracked it down to an issue
> with page migration, where the source page was a userspace page with
> MTE tags, and the target page was allocated using KASAN (i.e. having
> a non-zero KASAN tag). This caused tag check faults when the page was
> subsequently accessed by the kernel as a result of the mismatching tags
> from userspace. Given the number of different ways that page migration
> target pages can be allocated, the simplest fix that we could think of
> was to synchronize the KASAN tag in copy_highpage().
>
> Can you try the patch below and let us know whether it fixes the issue?
>
> diff --git a/arch/arm64/mm/copypage.c b/arch/arm64/mm/copypage.c
> index 24913271e898c..87ed38e9747bd 100644
> --- a/arch/arm64/mm/copypage.c
> +++ b/arch/arm64/mm/copypage.c
> @@ -23,6 +23,8 @@ 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);
> + if (kasan_hw_tags_enabled())
> + page_kasan_tag_set(to, page_kasan_tag(from));
> mte_copy_page_tags(kto, kfrom);

Why not just page_kasan_tag_reset(to)? If PG_mte_tagged is set on the
'from' page, the tags are random anyway and page_kasan_tag(from) should
already be 0xff. It makes more sense to do the same for the 'to' page
rather than copying the tag from the 'from' page. IOW, we are copying
user-controlled tags into a page, the kernel should have a match-all tag
in page->flags.

> Catalin, please let us know what you think of the patch above. It
> effectively partially undoes commit 20794545c146 ("arm64: kasan: Revert
> "arm64: mte: reset the page tag in page->flags""), but this seems okay
> to me because the mentioned race condition shouldn't affect "new" pages
> such as those being used as migration targets. The smp_wmb() that was
> there before doesn't seem necessary for the same reason.
>
> If the patch is okay, we should apply it to the 6.1 stable kernel. The
> problem appears to be "fixed" in the mainline kernel because of
> a bad merge conflict resolution on my part; when I rebased commit
> e059853d14ca ("arm64: mte: Fix/clarify the PG_mte_tagged semantics")
> past commit 20794545c146, it looks like I accidentally brought back the
> page_kasan_tag_reset() line removed in the latter. But we should align
> the mainline kernel with whatever we decide to do on 6.1.

Happy accident ;). When I reverted such calls in commit 20794545c146, my
assumption was that we always get a page that went through
post_alloc_hook() and the tags were reset. But it seems that's not
always the case (and probably wasteful anyway if we have to zero the
tags and data on a page we know we are going to override via
copy_highpage() anyway). The barrier doesn't help, so we shouldn't add
it back.

So, I'm fine with a stable fix but I wonder whether we should backport
the whole "Fix/clarify the PG_mte_tagged semantics" series instead.

--
Catalin

[Peter Collingbourne]

Hi Catalin,

That would also work, but I was thinking that if copy_highpage() were
being used to copy a KASAN page we should keep the original tag in
order to maintain tag checks for page accesses.

> > Catalin, please let us know what you think of the patch above. It
> > effectively partially undoes commit 20794545c146 ("arm64: kasan: Revert
> > "arm64: mte: reset the page tag in page->flags""), but this seems okay
> > to me because the mentioned race condition shouldn't affect "new" pages
> > such as those being used as migration targets. The smp_wmb() that was
> > there before doesn't seem necessary for the same reason.
> >
> > If the patch is okay, we should apply it to the 6.1 stable kernel. The
> > problem appears to be "fixed" in the mainline kernel because of
> > a bad merge conflict resolution on my part; when I rebased commit
> > e059853d14ca ("arm64: mte: Fix/clarify the PG_mte_tagged semantics")
> > past commit 20794545c146, it looks like I accidentally brought back the
> > page_kasan_tag_reset() line removed in the latter. But we should align
> > the mainline kernel with whatever we decide to do on 6.1.
>
> Happy accident ;). When I reverted such calls in commit 20794545c146, my
> assumption was that we always get a page that went through
> post_alloc_hook() and the tags were reset. But it seems that's not
> always the case (and probably wasteful anyway if we have to zero the
> tags and data on a page we know we are going to override via
> copy_highpage() anyway). The barrier doesn't help, so we shouldn't add
> it back.
>
> So, I'm fine with a stable fix but I wonder whether we should backport
> the whole "Fix/clarify the PG_mte_tagged semantics" series instead.

That seems fine to me (or as well as the above patch if we decide to
copy the tag).

Peter

[Qun-wei Lin (林群崴)]

https://urldefense.com/v3/__https://android.googlesource.com/kernel/common/*/53b3a7721b7aec74d8fa2ee55c2480044cc7c1b8__;Kw!!CTRNKA9wMg0ARbw!iEzuh9LYXlwXkpcWaHjncfr6lNgTky7OEAEzQ7cIFjlTD__7lwXqAhPJwWJXEnD8THUS7jnBK7hjnHw$ 
> >  
> > (53b3a77 Merge 6.1.1 into android14-6.1) is the latest commit in
> > our
> > tree.
> > 
> > > Do you have any private MTE-related changes in the kernel?
> > 
> > No, all the MTE-related code is the same as Android Common Kernel.
> > 
> > > Do you have userspace MTE enabled?
> > 
> > Yes, we have enabled MTE for both EL1 and EL0.
> 
> Hi Qun-wei,
> 
> Thanks for the information. We encountered a similar issue internally
> with the Android 5.15 common kernel. We tracked it down to an issue
> with page migration, where the source page was a userspace page with
> MTE tags, and the target page was allocated using KASAN (i.e. having
> a non-zero KASAN tag). This caused tag check faults when the page was
> subsequently accessed by the kernel as a result of the mismatching
> tags
> from userspace. Given the number of different ways that page
> migration
> target pages can be allocated, the simplest fix that we could think
> of
> was to synchronize the KASAN tag in copy_highpage().
> 
> Can you try the patch below and let us know whether it fixes the
> issue?
> 
> diff --git a/arch/arm64/mm/copypage.c b/arch/arm64/mm/copypage.c
> index 24913271e898c..87ed38e9747bd 100644
> --- a/arch/arm64/mm/copypage.c
> +++ b/arch/arm64/mm/copypage.c
> @@ -23,6 +23,8 @@ 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);
> +if (kasan_hw_tags_enabled())
> +page_kasan_tag_set(to, page_kasan_tag(from));
>  mte_copy_page_tags(kto, kfrom);
>  }
>  }
> 

Thank you so much, this patch has solved the problem.

> Catalin, please let us know what you think of the patch above. It
> effectively partially undoes commit 20794545c146 ("arm64: kasan:
> Revert
> "arm64: mte: reset the page tag in page->flags""), but this seems
> okay
> to me because the mentioned race condition shouldn't affect "new"
> pages
> such as those being used as migration targets. The smp_wmb() that was
> there before doesn't seem necessary for the same reason.
> 
> If the patch is okay, we should apply it to the 6.1 stable kernel.
> The
> problem appears to be "fixed" in the mainline kernel because of
> a bad merge conflict resolution on my part; when I rebased commit
> e059853d14ca ("arm64: mte: Fix/clarify the PG_mte_tagged semantics")
> past commit 20794545c146, it looks like I accidentally brought back
> the
> page_kasan_tag_reset() line removed in the latter. But we should
> align
> the mainline kernel with whatever we decide to do on 6.1.
> 
> Peter

[Catalin Marinas]

If PG_mte_tagged is set on the source, it means that the tags are no
longer trusted and we should reset to match-all. Otherwise if
copy_highpage() is called on a page that was never mapped as PROT_MTE in
user space, PG_mte_tagged would not be set on the source and no tags
copied. In such case, we should keep the original KASAN tag in the
destination. Unless I misunderstood what you meant.

--
Catalin

[Peter Collingbourne]

On Mon, Feb 13, 2023 at 10:47 AM Catalin Marinas

I was thinking that it might be possible for PG_mte_tagged to be set
on a page with KASAN tags. But as far as I can tell, this isn't
actually possible (or not intended to be possible, at least). So I
agree, we can just call page_kasan_tag_reset() here.

Patch sent (with stable backport instructions) here:
https://lore.kernel.org/all/2023021401521...@google.com/

Peter