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

[Peter Collingbourne <pcc@google.com>]

On Mon, Feb 13, 2023 at 10:47 AM Catalin Marinas
<catalin.marinas@arm.com> wrote:
>
> On Fri, Feb 10, 2023 at 11:03:45AM -0800, Peter Collingbourne wrote:
> > On Fri, Feb 10, 2023 at 10:28 AM Catalin Marinas
> > <catalin.marinas@arm.com> wrote:
> > > 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.
> >
> > 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.
>
> 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.

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/20230214015214.747873-1-pcc@google.com/

Peter