Re: [PATCH v10 12/22] kasan, arm64: fix up fault handling logic

[Mark Rutland]

On Tue, Nov 13, 2018 at 04:01:27PM +0100, Andrey Konovalov wrote:
> On Thu, Nov 8, 2018 at 1:22 PM, Mark Rutland <mark.rutland@xxxxxxx> wrote:
> > On Tue, Nov 06, 2018 at 06:30:27PM +0100, Andrey Konovalov wrote:
> >> show_pte in arm64 fault handling relies on the fact that the top byte of
> >> a kernel pointer is 0xff, which isn't always the case with tag-based
> >> KASAN.
> >
> > That's for the TTBR1 check, right?
> >
> > i.e. for the following to work:
> >
> >         if (addr >= VA_START)
> >
> > ... we need the tag bits to be an extension of bit 55...
> >
> >>
> >> This patch resets the top byte in show_pte.
> >>
> >> Reviewed-by: Andrey Ryabinin <aryabinin@xxxxxxxxxxxxx>
> >> Reviewed-by: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
> >> Signed-off-by: Andrey Konovalov <andreyknvl@xxxxxxxxxx>
> >> ---
> >>  arch/arm64/mm/fault.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> >> index 7d9571f4ae3d..d9a84d6f3343 100644
> >> --- a/arch/arm64/mm/fault.c
> >> +++ b/arch/arm64/mm/fault.c
> >> @@ -32,6 +32,7 @@
> >>  #include <linux/perf_event.h>
> >>  #include <linux/preempt.h>
> >>  #include <linux/hugetlb.h>
> >> +#include <linux/kasan.h>
> >>
> >>  #include <asm/bug.h>
> >>  #include <asm/cmpxchg.h>
> >> @@ -141,6 +142,8 @@ void show_pte(unsigned long addr)
> >>       pgd_t *pgdp;
> >>       pgd_t pgd;
> >>
> >> +     addr = (unsigned long)kasan_reset_tag((void *)addr);
> >
> > ... but this ORs in (0xffUL << 56), which is not correct for addresses
> > which aren't TTBR1 addresses to begin with, where bit 55 is clear, and
> > throws away useful information.
> >
> > We could use untagged_addr() here, but that wouldn't be right for
> > kernels which don't use TBI1, and we'd erroneously report addresses
> > under the TTBR1 range as being in the TTBR1 range.
> >
> > I also see that the entry assembly for el{1,0}_{da,ia} clears the tag
> > for EL0 addresses.
> >
> > So we could have:
> >
> > static inline bool is_ttbr0_addr(unsigned long addr)
> > {
> >         /* entry assembly clears tags for TTBR0 addrs */
> >         return addr < TASK_SIZE_64;
> > }
> >
> > static inline bool is_ttbr1_addr(unsigned long addr)
> > {
> >         /* TTBR1 addresses may have a tag if HWKASAN is in use */
> >         return arch_kasan_reset_tag(addr) >= VA_START;
> > }
> >
> > ... and use those in the conditionals, leaving the addr as-is for
> > reporting purposes.
> 
> Actually it looks like 276e9327 ("arm64: entry: improve data abort
> handling of tagged pointers") already takes care of both user and
> kernel fault addresses and correctly removes tags from them. So I
> think we need to drop this patch.

The clear_address_tag macro added in that commit only removes tags from TTBR0
addresses, so that's not sufficient if the kernel is used tagged addresses
(which will be in the TTBR1 range).

Thanks,
Mark.
That commit only removes tags from TTBR0 addresses,