| Message ID | 20221006163354.3194102-1-catalin.marinas@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: mte: Avoid setting PG_mte_tagged if no tags cleared or restored | expand |
On 06/10/2022 17:33, Catalin Marinas wrote: > Prior to commit 69e3b846d8a7 ("arm64: mte: Sync tags for pages where PTE > is untagged"), mte_sync_tags() was only called for pte_tagged() entries > (those mapped with PROT_MTE). Therefore mte_sync_tags() could safely use > test_and_set_bit(PG_mte_tagged, &page->flags) without inadvertently > setting PG_mte_tagged on an untagged page. > > The above commit was required as guests may enable MTE without any > control at the stage 2 mapping, nor a PROT_MTE mapping in the VMM. > However, the side-effect was that any page with a PTE that looked like > swap (or migration) was getting PG_mte_tagged set automatically. A > subsequent page copy (e.g. migration) copied the tags to the destination > page even if the tags were owned by KASAN. > > This issue was masked by the page_kasan_tag_reset() call introduced in > commit e5b8d9218951 ("arm64: mte: reset the page tag in page->flags"). > When this commit was reverted (20794545c146), KASAN started reporting > access faults because the overriding tags in a page did not match the > original page->flags (with CONFIG_KASAN_HW_TAGS=y): > > BUG: KASAN: invalid-access in copy_page+0x10/0xd0 arch/arm64/lib/copy_page.S:26 > Read at addr f5ff000017f2e000 by task syz-executor.1/2218 > Pointer tag: [f5], memory tag: [f2] > > Move the PG_mte_tagged bit setting from mte_sync_tags() to the actual > place where tags are cleared (mte_sync_page_tags()) or restored > (mte_restore_tags()). > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > Reported-by: syzbot+c2c79c6d6eddc5262b77@syzkaller.appspotmail.com > Fixes: 69e3b846d8a7 ("arm64: mte: Sync tags for pages where PTE is untagged") > Cc: <stable@vger.kernel.org> # 5.14.x > Cc: Steven Price <steven.price@arm.com> > Cc: Andrey Konovalov <andreyknvl@gmail.com> > Cc: Vincenzo Frascino <vincenzo.frascino@arm.com> > Cc: Will Deacon <will@kernel.org> > Link: https://lore.kernel.org/r/0000000000004387dc05e5888ae5@google.com/ Reviewed-by: Steven Price <steven.price@arm.com> > --- > > This seems to work for me but reproducing the issue is not entirely > consistent. Once reviewed, we can merge it and then it will hit the > various CI systems and syzbot. As you say - it seems to work, hopefully the bots will agree! Steve > > arch/arm64/kernel/mte.c | 9 +++++++-- > arch/arm64/mm/mteswap.c | 7 ++++++- > 2 files changed, 13 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c > index aca88470fb69..7467217c1eaf 100644 > --- a/arch/arm64/kernel/mte.c > +++ b/arch/arm64/kernel/mte.c > @@ -48,7 +48,12 @@ static void mte_sync_page_tags(struct page *page, pte_t old_pte, > if (!pte_is_tagged) > return; > > - mte_clear_page_tags(page_address(page)); > + /* > + * Test PG_mte_tagged again in case it was racing with another > + * set_pte_at(). > + */ > + if (!test_and_set_bit(PG_mte_tagged, &page->flags)) > + mte_clear_page_tags(page_address(page)); > } > > void mte_sync_tags(pte_t old_pte, pte_t pte) > @@ -64,7 +69,7 @@ void mte_sync_tags(pte_t old_pte, pte_t pte) > > /* if PG_mte_tagged is set, tags have already been initialised */ > for (i = 0; i < nr_pages; i++, page++) { > - if (!test_and_set_bit(PG_mte_tagged, &page->flags)) > + if (!test_bit(PG_mte_tagged, &page->flags)) > mte_sync_page_tags(page, old_pte, check_swap, > pte_is_tagged); > } > diff --git a/arch/arm64/mm/mteswap.c b/arch/arm64/mm/mteswap.c > index 4334dec93bd4..bed803d8e158 100644 > --- a/arch/arm64/mm/mteswap.c > +++ b/arch/arm64/mm/mteswap.c > @@ -53,7 +53,12 @@ bool mte_restore_tags(swp_entry_t entry, struct page *page) > if (!tags) > return false; > > - mte_restore_page_tags(page_address(page), tags); > + /* > + * Test PG_mte_tagged again in case it was racing with another > + * set_pte_at(). > + */ > + if (!test_and_set_bit(PG_mte_tagged, &page->flags)) > + mte_restore_page_tags(page_address(page), tags); > > return true; > } > > base-commit: d2995249a2f72333a4ab4922ff3c42a76c023791
On Thu, 6 Oct 2022 17:33:54 +0100, Catalin Marinas wrote: > Prior to commit 69e3b846d8a7 ("arm64: mte: Sync tags for pages where PTE > is untagged"), mte_sync_tags() was only called for pte_tagged() entries > (those mapped with PROT_MTE). Therefore mte_sync_tags() could safely use > test_and_set_bit(PG_mte_tagged, &page->flags) without inadvertently > setting PG_mte_tagged on an untagged page. > > The above commit was required as guests may enable MTE without any > control at the stage 2 mapping, nor a PROT_MTE mapping in the VMM. > However, the side-effect was that any page with a PTE that looked like > swap (or migration) was getting PG_mte_tagged set automatically. A > subsequent page copy (e.g. migration) copied the tags to the destination > page even if the tags were owned by KASAN. > > [...] Applied to arm64 (for-next/core), thanks! [1/1] arm64: mte: Avoid setting PG_mte_tagged if no tags cleared or restored https://git.kernel.org/arm64/c/a8e5e5146ad0
diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c index aca88470fb69..7467217c1eaf 100644 --- a/arch/arm64/kernel/mte.c +++ b/arch/arm64/kernel/mte.c @@ -48,7 +48,12 @@ static void mte_sync_page_tags(struct page *page, pte_t old_pte, if (!pte_is_tagged) return; - mte_clear_page_tags(page_address(page)); + /* + * Test PG_mte_tagged again in case it was racing with another + * set_pte_at(). + */ + if (!test_and_set_bit(PG_mte_tagged, &page->flags)) + mte_clear_page_tags(page_address(page)); } void mte_sync_tags(pte_t old_pte, pte_t pte) @@ -64,7 +69,7 @@ void mte_sync_tags(pte_t old_pte, pte_t pte) /* if PG_mte_tagged is set, tags have already been initialised */ for (i = 0; i < nr_pages; i++, page++) { - if (!test_and_set_bit(PG_mte_tagged, &page->flags)) + if (!test_bit(PG_mte_tagged, &page->flags)) mte_sync_page_tags(page, old_pte, check_swap, pte_is_tagged); } diff --git a/arch/arm64/mm/mteswap.c b/arch/arm64/mm/mteswap.c index 4334dec93bd4..bed803d8e158 100644 --- a/arch/arm64/mm/mteswap.c +++ b/arch/arm64/mm/mteswap.c @@ -53,7 +53,12 @@ bool mte_restore_tags(swp_entry_t entry, struct page *page) if (!tags) return false; - mte_restore_page_tags(page_address(page), tags); + /* + * Test PG_mte_tagged again in case it was racing with another + * set_pte_at(). + */ + if (!test_and_set_bit(PG_mte_tagged, &page->flags)) + mte_restore_page_tags(page_address(page), tags); return true; }
Prior to commit 69e3b846d8a7 ("arm64: mte: Sync tags for pages where PTE is untagged"), mte_sync_tags() was only called for pte_tagged() entries (those mapped with PROT_MTE). Therefore mte_sync_tags() could safely use test_and_set_bit(PG_mte_tagged, &page->flags) without inadvertently setting PG_mte_tagged on an untagged page. The above commit was required as guests may enable MTE without any control at the stage 2 mapping, nor a PROT_MTE mapping in the VMM. However, the side-effect was that any page with a PTE that looked like swap (or migration) was getting PG_mte_tagged set automatically. A subsequent page copy (e.g. migration) copied the tags to the destination page even if the tags were owned by KASAN. This issue was masked by the page_kasan_tag_reset() call introduced in commit e5b8d9218951 ("arm64: mte: reset the page tag in page->flags"). When this commit was reverted (20794545c146), KASAN started reporting access faults because the overriding tags in a page did not match the original page->flags (with CONFIG_KASAN_HW_TAGS=y): BUG: KASAN: invalid-access in copy_page+0x10/0xd0 arch/arm64/lib/copy_page.S:26 Read at addr f5ff000017f2e000 by task syz-executor.1/2218 Pointer tag: [f5], memory tag: [f2] Move the PG_mte_tagged bit setting from mte_sync_tags() to the actual place where tags are cleared (mte_sync_page_tags()) or restored (mte_restore_tags()). Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> Reported-by: syzbot+c2c79c6d6eddc5262b77@syzkaller.appspotmail.com Fixes: 69e3b846d8a7 ("arm64: mte: Sync tags for pages where PTE is untagged") Cc: <stable@vger.kernel.org> # 5.14.x Cc: Steven Price <steven.price@arm.com> Cc: Andrey Konovalov <andreyknvl@gmail.com> Cc: Vincenzo Frascino <vincenzo.frascino@arm.com> Cc: Will Deacon <will@kernel.org> Link: https://lore.kernel.org/r/0000000000004387dc05e5888ae5@google.com/ --- This seems to work for me but reproducing the issue is not entirely consistent. Once reviewed, we can merge it and then it will hit the various CI systems and syzbot. arch/arm64/kernel/mte.c | 9 +++++++-- arch/arm64/mm/mteswap.c | 7 ++++++- 2 files changed, 13 insertions(+), 3 deletions(-) base-commit: d2995249a2f72333a4ab4922ff3c42a76c023791