Re: [PATCH v10 00/22] kasan: add software tag-based mode for arm64

Re: [PATCH v10 00/22] kasan: add software tag-based mode for arm64

[Andrey Konovalov]

On Tue, Nov 6, 2018 at 6:30 PM, Andrey Konovalov <andreyknvl@google.com> wrote:
> This patchset adds a new software tag-based mode to KASAN [1].
> (Initially this mode was called KHWASAN, but it got renamed,
>  see the naming rationale at the end of this section).

[...]

> Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>

Hi Andrew,

This patchset has now been reviewed-by KASAN maintainers. Could you
take a look and consider taking this into the -mm tree?

Thanks!

Re: [PATCH v10 00/22] kasan: add software tag-based mode for arm64

[Will Deacon]

On Wed, Nov 07, 2018 at 03:56:03PM +0100, Andrey Konovalov wrote:
> On Tue, Nov 6, 2018 at 6:30 PM, Andrey Konovalov <andreyknvl@google.com> wrote:
> > This patchset adds a new software tag-based mode to KASAN [1].
> > (Initially this mode was called KHWASAN, but it got renamed,
> >  see the naming rationale at the end of this section).
> 
> [...]
> 
> > Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> > Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
> > Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> 
> Hi Andrew,
> 
> This patchset has now been reviewed-by KASAN maintainers. Could you
> take a look and consider taking this into the -mm tree?

I would much prefer to take the arm64 parts (which still need to be reviewed
by Catalin afaict) via the arm64 tree, so please can you split those out
separately?

Will

Re: [PATCH v10 00/22] kasan: add software tag-based mode for arm64

[Andrey Konovalov]

On Wed, Nov 7, 2018 at 3:59 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Wed, Nov 07, 2018 at 03:56:03PM +0100, Andrey Konovalov wrote:
>> On Tue, Nov 6, 2018 at 6:30 PM, Andrey Konovalov <andreyknvl@google.com> wrote:
>> > This patchset adds a new software tag-based mode to KASAN [1].
>> > (Initially this mode was called KHWASAN, but it got renamed,
>> >  see the naming rationale at the end of this section).
>>
>> [...]
>>
>> > Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
>> > Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
>> > Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
>>
>> Hi Andrew,
>>
>> This patchset has now been reviewed-by KASAN maintainers. Could you
>> take a look and consider taking this into the -mm tree?
>
> I would much prefer to take the arm64 parts (which still need to be reviewed
> by Catalin afaict) via the arm64 tree, so please can you split those out
> separately?

Which parts do you mean exactly, which patches? I don't think it makes
sense to split this patchset, as one part won't function without the
other.

Re: [PATCH v10 00/22] kasan: add software tag-based mode for arm64

[Will Deacon]

On Wed, Nov 07, 2018 at 04:11:35PM +0100, Andrey Konovalov wrote:
> On Wed, Nov 7, 2018 at 3:59 PM, Will Deacon <will.deacon@arm.com> wrote:
> > On Wed, Nov 07, 2018 at 03:56:03PM +0100, Andrey Konovalov wrote:
> >> On Tue, Nov 6, 2018 at 6:30 PM, Andrey Konovalov <andreyknvl@google.com> wrote:
> >> > This patchset adds a new software tag-based mode to KASAN [1].
> >> > (Initially this mode was called KHWASAN, but it got renamed,
> >> >  see the naming rationale at the end of this section).
> >>
> >> [...]
> >>
> >> > Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> >> > Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
> >> > Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> >>
> >> Hi Andrew,
> >>
> >> This patchset has now been reviewed-by KASAN maintainers. Could you
> >> take a look and consider taking this into the -mm tree?
> >
> > I would much prefer to take the arm64 parts (which still need to be reviewed
> > by Catalin afaict) via the arm64 tree, so please can you split those out
> > separately?
> 
> Which parts do you mean exactly, which patches? I don't think it makes
> sense to split this patchset, as one part won't function without the
> other.

I would like the patches that touch code under arch/arm64/ to be reviewed by
somebody from the arm64 community. Since the core parts have already been
reviewed, I was suggesting that you could split them out so that they are
not blocked by the architecture code. Is it not possible to preserve the
existing KASAN behaviour for arm64 with the core parts merged? I figured it
must be, since you're not touching any other architectures here and they
assumedly continue to function correctly.

However, if you'd rather keep everything together, please can we give it a
couple of weeks so we can at least get the architecture bits reviewed? Most
people are out at LPC next week (and I'm at another conference this week).

Will

Re: [PATCH v10 00/22] kasan: add software tag-based mode for arm64

[Andrey Konovalov]

On Wed, Nov 7, 2018 at 4:34 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Wed, Nov 07, 2018 at 04:11:35PM +0100, Andrey Konovalov wrote:
>> On Wed, Nov 7, 2018 at 3:59 PM, Will Deacon <will.deacon@arm.com> wrote:
>> > On Wed, Nov 07, 2018 at 03:56:03PM +0100, Andrey Konovalov wrote:
>> >> On Tue, Nov 6, 2018 at 6:30 PM, Andrey Konovalov <andreyknvl@google.com> wrote:
>> >> > This patchset adds a new software tag-based mode to KASAN [1].
>> >> > (Initially this mode was called KHWASAN, but it got renamed,
>> >> >  see the naming rationale at the end of this section).
>> >>
>> >> [...]
>> >>
>> >> > Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
>> >> > Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
>> >> > Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
>> >>
>> >> Hi Andrew,
>> >>
>> >> This patchset has now been reviewed-by KASAN maintainers. Could you
>> >> take a look and consider taking this into the -mm tree?
>> >
>> > I would much prefer to take the arm64 parts (which still need to be reviewed
>> > by Catalin afaict) via the arm64 tree, so please can you split those out
>> > separately?
>>
>> Which parts do you mean exactly, which patches? I don't think it makes
>> sense to split this patchset, as one part won't function without the
>> other.
>
> I would like the patches that touch code under arch/arm64/ to be reviewed by
> somebody from the arm64 community. Since the core parts have already been
> reviewed, I was suggesting that you could split them out so that they are
> not blocked by the architecture code. Is it not possible to preserve the
> existing KASAN behaviour for arm64 with the core parts merged? I figured it
> must be, since you're not touching any other architectures here and they
> assumedly continue to function correctly.
>
> However, if you'd rather keep everything together, please can we give it a
> couple of weeks so we can at least get the architecture bits reviewed? Most
> people are out at LPC next week (and I'm at another conference this week).
>
> Will

Re: [PATCH v10 00/22] kasan: add software tag-based mode for arm64

[Andrey Konovalov]

On Wed, Nov 7, 2018 at 4:34 PM, Will Deacon <will.deacon@arm.com> wrote:
>
> I would like the patches that touch code under arch/arm64/ to be reviewed by
> somebody from the arm64 community. Since the core parts have already been
> reviewed, I was suggesting that you could split them out so that they are
> not blocked by the architecture code. Is it not possible to preserve the
> existing KASAN behaviour for arm64 with the core parts merged? I figured it
> must be, since you're not touching any other architectures here and they
> assumedly continue to function correctly.

It's possible to split out the core mm part, but it doesn't make much
sense to merge it separately from the arm64 changes.

> However, if you'd rather keep everything together, please can we give it a
> couple of weeks so we can at least get the architecture bits reviewed? Most
> people are out at LPC next week (and I'm at another conference this week).

OK, sounds good!

Catalin, could you take a look at the arm64 specific changes?

Re: [PATCH v10 08/22] kasan, arm64: untag address in __kimg_to_phys and _virt_addr_is_linear

[Mark Rutland]

Hi Andrey,

On Tue, Nov 06, 2018 at 06:30:23PM +0100, Andrey Konovalov wrote:
> __kimg_to_phys (which is used by virt_to_phys) and _virt_addr_is_linear
> (which is used by virt_addr_valid) assume that the top byte of the address
> is 0xff, which isn't always the case with tag-based KASAN.

I'm confused by this. Why/when do kimg address have a non-default tag?

Any kimg address is part of the static kernel image, so it's not obvious
to me how a kimg address would gain a tag. Could you please explain how
this happens in the commit message?

> This patch resets the tag in those macros.
> 
> Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> ---
>  arch/arm64/include/asm/memory.h | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> index 0f1e024a951f..3226a0218b0b 100644
> --- a/arch/arm64/include/asm/memory.h
> +++ b/arch/arm64/include/asm/memory.h
> @@ -92,6 +92,15 @@
>  #define KASAN_THREAD_SHIFT	0
>  #endif
>  
> +#ifdef CONFIG_KASAN_SW_TAGS
> +#define KASAN_TAG_SHIFTED(tag)		((unsigned long)(tag) << 56)
> +#define KASAN_SET_TAG(addr, tag)	(((addr) & ~KASAN_TAG_SHIFTED(0xff)) | \
> +						KASAN_TAG_SHIFTED(tag))
> +#define KASAN_RESET_TAG(addr)		KASAN_SET_TAG(addr, 0xff)
> +#else
> +#define KASAN_RESET_TAG(addr)		addr
> +#endif

Nit: the rest of the helper macros in this file are lower-case, with
specialised helpers prefixed with several underscores. Could we please
stick with that convention?

e.g. have __tag_set() and __tag_reset() helpers.

> +
>  #define MIN_THREAD_SHIFT	(14 + KASAN_THREAD_SHIFT)
>  
>  /*
> @@ -232,7 +241,7 @@ static inline unsigned long kaslr_offset(void)
>  #define __is_lm_address(addr)	(!!((addr) & BIT(VA_BITS - 1)))
>  
>  #define __lm_to_phys(addr)	(((addr) & ~PAGE_OFFSET) + PHYS_OFFSET)
> -#define __kimg_to_phys(addr)	((addr) - kimage_voffset)
> +#define __kimg_to_phys(addr)	(KASAN_RESET_TAG(addr) - kimage_voffset)

IIUC You need to adjust __lm_to_phys() too, since that could be passed
an address from SLAB.

Maybe that's done in a later patch, but if so it's confusing to split it
out that way. It would be nicer to fix all the *_to_*() helpers in one
go.

>  
>  #define __virt_to_phys_nodebug(x) ({					\
>  	phys_addr_t __x = (phys_addr_t)(x);				\
> @@ -308,7 +317,8 @@ static inline void *phys_to_virt(phys_addr_t x)
>  #endif
>  #endif
>  
> -#define _virt_addr_is_linear(kaddr)	(((u64)(kaddr)) >= PAGE_OFFSET)
> +#define _virt_addr_is_linear(kaddr)	(KASAN_RESET_TAG((u64)(kaddr)) >= \
> +						PAGE_OFFSET)

This is painful to read. Could you please split this like:

#define _virt_addr_is_linear(kaddr) \
	(__tag_reset((u64)(kaddr)) >= PAGE_OFFSET)

... and we could reformat virt_addr_valid() in the same way while we're
at it.

Thanks,
Mark.

Re: [PATCH v10 06/22] kasan, arm64: adjust shadow size for tag-based mode

[Mark Rutland]

Hi Andrey,

On Tue, Nov 06, 2018 at 06:30:21PM +0100, Andrey Konovalov wrote:
> Tag-based KASAN uses 1 shadow byte for 16 bytes of kernel memory, so it
> requires 1/16th of the kernel virtual address space for the shadow memory.
> 
> This commit sets KASAN_SHADOW_SCALE_SHIFT to 4 when the tag-based KASAN
> mode is enabled.
> 
> Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> ---
>  arch/arm64/Makefile             |  2 +-
>  arch/arm64/include/asm/memory.h | 13 +++++++++----
>  2 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
> index 6cb9fc7e9382..9887492381d9 100644
> --- a/arch/arm64/Makefile
> +++ b/arch/arm64/Makefile
> @@ -94,7 +94,7 @@ endif
>  # KASAN_SHADOW_OFFSET = VA_START + (1 << (VA_BITS - KASAN_SHADOW_SCALE_SHIFT))
>  #				 - (1 << (64 - KASAN_SHADOW_SCALE_SHIFT))
>  # in 32-bit arithmetic
> -KASAN_SHADOW_SCALE_SHIFT := 3
> +KASAN_SHADOW_SCALE_SHIFT := $(if $(CONFIG_KASAN_SW_TAGS), 4, 3)


We could make this something like:

ifeq ($(CONFIG_KASAN_SW_TAGS), y)
KASAN_SHADOW_SCALE_SHIFT := 4
else
KASAN_SHADOW_SCALE_SHIFT := 3
endif

KBUILD_CFLAGS += -DKASAN_SHADOW_SCALE_SHIFT=$(KASAN_SHADOW_SCALE_SHIFT)

>  KASAN_SHADOW_OFFSET := $(shell printf "0x%08x00000000\n" $$(( \
>  	(0xffffffff & (-1 << ($(CONFIG_ARM64_VA_BITS) - 32))) \
>  	+ (1 << ($(CONFIG_ARM64_VA_BITS) - 32 - $(KASAN_SHADOW_SCALE_SHIFT))) \
> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> index b96442960aea..0f1e024a951f 100644
> --- a/arch/arm64/include/asm/memory.h
> +++ b/arch/arm64/include/asm/memory.h
> @@ -74,12 +74,17 @@
>  #define KERNEL_END        _end
>  
>  /*
> - * KASAN requires 1/8th of the kernel virtual address space for the shadow
> - * region. KASAN can bloat the stack significantly, so double the (minimum)
> - * stack size when KASAN is in use.
> + * Generic and tag-based KASAN require 1/8th and 1/16th of the kernel virtual
> + * address space for the shadow region respectively. They can bloat the stack
> + * significantly, so double the (minimum) stack size when they are in use.
>   */
> -#ifdef CONFIG_KASAN
> +#ifdef CONFIG_KASAN_GENERIC
>  #define KASAN_SHADOW_SCALE_SHIFT 3
> +#endif
> +#ifdef CONFIG_KASAN_SW_TAGS
> +#define KASAN_SHADOW_SCALE_SHIFT 4
> +#endif
> +#ifdef CONFIG_KASAN

... and remove the constant entirely here, avoiding duplication.

Maybe factor that into a Makefile.kasan if things are going to get much
more complicated.

Thanks,
Mark.

>  #define KASAN_SHADOW_SIZE	(UL(1) << (VA_BITS - KASAN_SHADOW_SCALE_SHIFT))
>  #define KASAN_THREAD_SHIFT	1
>  #else
> -- 
> 2.19.1.930.g4563a0d9d0-goog
> 

Re: [PATCH v10 05/22] kasan: add CONFIG_KASAN_GENERIC and CONFIG_KASAN_SW_TAGS

[Mark Rutland]

On Tue, Nov 06, 2018 at 06:30:20PM +0100, Andrey Konovalov wrote:
> This commit splits the current CONFIG_KASAN config option into two:
> 1. CONFIG_KASAN_GENERIC, that enables the generic KASAN mode (the one
>    that exists now);
> 2. CONFIG_KASAN_SW_TAGS, that enables the software tag-based KASAN mode.
> 
> The name CONFIG_KASAN_SW_TAGS is chosen as in the future we will have
> another hardware tag-based KASAN mode, that will rely on hardware memory
> tagging support in arm64.
> 
> With CONFIG_KASAN_SW_TAGS enabled, compiler options are changed to
> instrument kernel files with -fsantize=kernel-hwaddress (except the ones
> for which KASAN_SANITIZE := n is set).
> 
> Both CONFIG_KASAN_GENERIC and CONFIG_KASAN_SW_TAGS support both
> CONFIG_KASAN_INLINE and CONFIG_KASAN_OUTLINE instrumentation modes.
> 
> This commit also adds empty placeholder (for now) implementation of
> tag-based KASAN specific hooks inserted by the compiler and adjusts
> common hooks implementation to compile correctly with each of the
> config options.
> 
> Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> ---
>  arch/arm64/Kconfig                  |  1 +
>  include/linux/compiler-clang.h      |  5 +-
>  include/linux/compiler-gcc.h        |  6 ++
>  include/linux/compiler_attributes.h | 13 -----
>  include/linux/kasan.h               | 16 ++++--
>  lib/Kconfig.kasan                   | 87 +++++++++++++++++++++++------
>  mm/kasan/Makefile                   |  6 +-
>  mm/kasan/generic.c                  |  2 +-
>  mm/kasan/kasan.h                    |  3 +-
>  mm/kasan/tags.c                     | 75 +++++++++++++++++++++++++
>  mm/slub.c                           |  2 +-
>  scripts/Makefile.kasan              | 27 ++++++++-
>  12 files changed, 201 insertions(+), 42 deletions(-)
>  create mode 100644 mm/kasan/tags.c
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 787d7850e064..8b331dcfb48e 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -111,6 +111,7 @@ config ARM64
>  	select HAVE_ARCH_JUMP_LABEL
>  	select HAVE_ARCH_JUMP_LABEL_RELATIVE
>  	select HAVE_ARCH_KASAN if !(ARM64_16K_PAGES && ARM64_VA_BITS_48)
> +	select HAVE_ARCH_KASAN_SW_TAGS if !(ARM64_16K_PAGES && ARM64_VA_BITS_48)

Given this relies on a compiler feature, can we please gate this on
compiler feature detection? e.g. in some common Kconfig have:

	select CC_HAS_ASAN_HWADDRESS if $(cc-option -fsanitize=kernel-hwaddress)

... and on arm64 we can do:

	select HAVE_ARCH_KASAN_SW_TAGS if !(ARM64_16K_PAGES && ARM64_VA_BITS_48)

... and core KASAN Kconfig can have:

config KASAN_SW_TAGS
	depends on HAVE_ARCH_KASAN_SW_TAGS
	depends on CC_HAS_ASAN_HWADDRESS

[...]

> +ifeq ($(call cc-option, $(CFLAGS_KASAN) -Werror),)
> +    ifneq ($(CONFIG_COMPILE_TEST),y)
> +        $(warning Cannot use CONFIG_KASAN_SW_TAGS: \
> +            -fsanitize=hwaddress is not supported by compiler)
> +    endif
> +endif

... and then this warning shouldn't be possible, and can go.

Thanks,
Mark.

Re: [PATCH v10 07/22] kasan: initialize shadow to 0xff for tag-based mode

[Mark Rutland]

On Tue, Nov 06, 2018 at 06:30:22PM +0100, Andrey Konovalov wrote:
> A tag-based KASAN shadow memory cell contains a memory tag, that
> corresponds to the tag in the top byte of the pointer, that points to that
> memory. The native top byte value of kernel pointers is 0xff, so with
> tag-based KASAN we need to initialize shadow memory to 0xff.
> 
> Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> ---
>  arch/arm64/mm/kasan_init.c | 16 ++++++++++++++--
>  include/linux/kasan.h      |  8 ++++++++
>  mm/kasan/common.c          |  3 ++-
>  3 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/mm/kasan_init.c b/arch/arm64/mm/kasan_init.c
> index 63527e585aac..18ebc8994a7b 100644
> --- a/arch/arm64/mm/kasan_init.c
> +++ b/arch/arm64/mm/kasan_init.c
> @@ -43,6 +43,15 @@ static phys_addr_t __init kasan_alloc_zeroed_page(int node)
>  	return __pa(p);
>  }
>  
> +static phys_addr_t __init kasan_alloc_raw_page(int node)
> +{
> +	void *p = memblock_alloc_try_nid_raw(PAGE_SIZE, PAGE_SIZE,
> +						__pa(MAX_DMA_ADDRESS),
> +						MEMBLOCK_ALLOC_ACCESSIBLE,
> +						node);
> +	return __pa(p);
> +}
> +
>  static pte_t *__init kasan_pte_offset(pmd_t *pmdp, unsigned long addr, int node,
>  				      bool early)
>  {
> @@ -88,7 +97,9 @@ static void __init kasan_pte_populate(pmd_t *pmdp, unsigned long addr,
>  
>  	do {
>  		phys_addr_t page_phys = early ? __pa_symbol(kasan_zero_page)
> -					      : kasan_alloc_zeroed_page(node);
> +					      : kasan_alloc_raw_page(node);
> +		if (!early)
> +			memset(__va(page_phys), KASAN_SHADOW_INIT, PAGE_SIZE);
>  		next = addr + PAGE_SIZE;
>  		set_pte(ptep, pfn_pte(__phys_to_pfn(page_phys), PAGE_KERNEL));
>  	} while (ptep++, addr = next, addr != end && pte_none(READ_ONCE(*ptep)));
> @@ -138,6 +149,7 @@ asmlinkage void __init kasan_early_init(void)
>  		KASAN_SHADOW_END - (1UL << (64 - KASAN_SHADOW_SCALE_SHIFT)));
>  	BUILD_BUG_ON(!IS_ALIGNED(KASAN_SHADOW_START, PGDIR_SIZE));
>  	BUILD_BUG_ON(!IS_ALIGNED(KASAN_SHADOW_END, PGDIR_SIZE));
> +
>  	kasan_pgd_populate(KASAN_SHADOW_START, KASAN_SHADOW_END, NUMA_NO_NODE,
>  			   true);
>  }
> @@ -234,7 +246,7 @@ void __init kasan_init(void)
>  		set_pte(&kasan_zero_pte[i],
>  			pfn_pte(sym_to_pfn(kasan_zero_page), PAGE_KERNEL_RO));
>  
> -	memset(kasan_zero_page, 0, PAGE_SIZE);
> +	memset(kasan_zero_page, KASAN_SHADOW_INIT, PAGE_SIZE);

If this isn't going to contain zero, can we please have a preparatory
patch renaming this to something which isn't misleading?

Perhaps kasan_common_shadow_page?

Thanks,
Mark.

Re: [PATCH v10 09/22] kasan: add tag related helper functions

[Mark Rutland]

On Tue, Nov 06, 2018 at 06:30:24PM +0100, Andrey Konovalov wrote:
> This commit adds a few helper functions, that are meant to be used to
> work with tags embedded in the top byte of kernel pointers: to set, to
> get or to reset (set to 0xff) the top byte.
> 
> Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> ---
>  arch/arm64/mm/kasan_init.c |  2 ++
>  include/linux/kasan.h      | 13 +++++++++
>  mm/kasan/kasan.h           | 55 ++++++++++++++++++++++++++++++++++++++
>  mm/kasan/tags.c            | 37 +++++++++++++++++++++++++
>  4 files changed, 107 insertions(+)
> 
> diff --git a/arch/arm64/mm/kasan_init.c b/arch/arm64/mm/kasan_init.c
> index 18ebc8994a7b..370b19d0e2fb 100644
> --- a/arch/arm64/mm/kasan_init.c
> +++ b/arch/arm64/mm/kasan_init.c
> @@ -249,6 +249,8 @@ void __init kasan_init(void)
>  	memset(kasan_zero_page, KASAN_SHADOW_INIT, PAGE_SIZE);
>  	cpu_replace_ttbr1(lm_alias(swapper_pg_dir));
>  
> +	kasan_init_tags();
> +
>  	/* At this point kasan is fully initialized. Enable error messages */
>  	init_task.kasan_depth = 0;
>  	pr_info("KernelAddressSanitizer initialized\n");
> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> index 7f6574c35c62..4c9d6f9029f2 100644
> --- a/include/linux/kasan.h
> +++ b/include/linux/kasan.h
> @@ -169,6 +169,19 @@ static inline void kasan_cache_shutdown(struct kmem_cache *cache) {}
>  
>  #define KASAN_SHADOW_INIT 0xFF
>  
> +void kasan_init_tags(void);
> +
> +void *kasan_reset_tag(const void *addr);
> +
> +#else /* CONFIG_KASAN_SW_TAGS */
> +
> +static inline void kasan_init_tags(void) { }
> +
> +static inline void *kasan_reset_tag(const void *addr)
> +{
> +	return (void *)addr;
> +}
> +

> +#ifdef CONFIG_KASAN_SW_TAGS
> +
> +#define KASAN_PTR_TAG_SHIFT 56
> +#define KASAN_PTR_TAG_MASK (0xFFUL << KASAN_PTR_TAG_SHIFT)
> +
> +u8 random_tag(void);
> +
> +static inline void *set_tag(const void *addr, u8 tag)
> +{
> +	u64 a = (u64)addr;
> +
> +	a &= ~KASAN_PTR_TAG_MASK;
> +	a |= ((u64)tag << KASAN_PTR_TAG_SHIFT);
> +
> +	return (void *)a;
> +}
> +
> +static inline u8 get_tag(const void *addr)
> +{
> +	return (u8)((u64)addr >> KASAN_PTR_TAG_SHIFT);
> +}
> +
> +static inline void *reset_tag(const void *addr)
> +{
> +	return set_tag(addr, KASAN_TAG_KERNEL);
> +}

We seem to be duplicating this functionality in several places.

Could we please make it so that the arch code defines macros:

arch_kasan_set_tag(addr, tag)
arch_kasan_get_tag(addr)
arch_kasan_reset_tag(addr)

... and use thoses consistently rather than open-coding them?

> +
> +#else /* CONFIG_KASAN_SW_TAGS */
> +
> +static inline u8 random_tag(void)
> +{
> +	return 0;
> +}
> +
> +static inline void *set_tag(const void *addr, u8 tag)
> +{
> +	return (void *)addr;
> +}
> +
> +static inline u8 get_tag(const void *addr)
> +{
> +	return 0;
> +}
> +
> +static inline void *reset_tag(const void *addr)
> +{
> +	return (void *)addr;
> +}

... these can be defined in linux/kasan.h as:

#define arch_kasan_set_tag(addr, tag)	(addr)
#define arch_kasan_get_tag(addr)	0
#define arch_kasan_reset_tag(addr)	(addr)

Thanks,
Mark.

Re: [PATCH v10 08/22] kasan, arm64: untag address in __kimg_to_phys and _virt_addr_is_linear

[Catalin Marinas]

On Tue, Nov 06, 2018 at 06:30:23PM +0100, Andrey Konovalov wrote:
> --- a/arch/arm64/include/asm/memory.h
> +++ b/arch/arm64/include/asm/memory.h
> @@ -92,6 +92,15 @@
>  #define KASAN_THREAD_SHIFT	0
>  #endif
>  
> +#ifdef CONFIG_KASAN_SW_TAGS
> +#define KASAN_TAG_SHIFTED(tag)		((unsigned long)(tag) << 56)
> +#define KASAN_SET_TAG(addr, tag)	(((addr) & ~KASAN_TAG_SHIFTED(0xff)) | \
> +						KASAN_TAG_SHIFTED(tag))
> +#define KASAN_RESET_TAG(addr)		KASAN_SET_TAG(addr, 0xff)
> +#else
> +#define KASAN_RESET_TAG(addr)		addr
> +#endif

I think we should reuse the untagged_addr() macro we have in uaccess.h
(make it more general and move to another header file).

-- 
Catalin

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

[Catalin Marinas]

On Tue, Nov 06, 2018 at 06:30:27PM +0100, Andrey Konovalov wrote:
> 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);
> +
>  	if (addr < TASK_SIZE) {
>  		/* TTBR0 */
>  		mm = current->active_mm;

I think we should clear the tag earlier on in the fault handling code,
before reaching show_pte().

-- 
Catalin

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

[Mark Rutland]

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@virtuozzo.com>
> Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> ---
>  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.

Thanks,
Mark.

Re: [PATCH v10 06/22] kasan, arm64: adjust shadow size for tag-based mode

[Andrey Konovalov]

On Wed, Nov 7, 2018 at 5:54 PM, Mark Rutland <mark.rutland@arm.com> wrote:

[...]

>> --- a/arch/arm64/Makefile
>> +++ b/arch/arm64/Makefile
>> @@ -94,7 +94,7 @@ endif
>>  # KASAN_SHADOW_OFFSET = VA_START + (1 << (VA_BITS - KASAN_SHADOW_SCALE_SHIFT))
>>  #                             - (1 << (64 - KASAN_SHADOW_SCALE_SHIFT))
>>  # in 32-bit arithmetic
>> -KASAN_SHADOW_SCALE_SHIFT := 3
>> +KASAN_SHADOW_SCALE_SHIFT := $(if $(CONFIG_KASAN_SW_TAGS), 4, 3)
>
>
> We could make this something like:
>
> ifeq ($(CONFIG_KASAN_SW_TAGS), y)
> KASAN_SHADOW_SCALE_SHIFT := 4
> else
> KASAN_SHADOW_SCALE_SHIFT := 3
> endif
>
> KBUILD_CFLAGS += -DKASAN_SHADOW_SCALE_SHIFT=$(KASAN_SHADOW_SCALE_SHIFT)

Seems that we need the same for KBUILD_CPPFLAGS and KBUILD_AFLAGS.


>> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
>> index b96442960aea..0f1e024a951f 100644
>> --- a/arch/arm64/include/asm/memory.h
>> +++ b/arch/arm64/include/asm/memory.h
>> @@ -74,12 +74,17 @@
>>  #define KERNEL_END        _end
>>
>>  /*
>> - * KASAN requires 1/8th of the kernel virtual address space for the shadow
>> - * region. KASAN can bloat the stack significantly, so double the (minimum)
>> - * stack size when KASAN is in use.
>> + * Generic and tag-based KASAN require 1/8th and 1/16th of the kernel virtual
>> + * address space for the shadow region respectively. They can bloat the stack
>> + * significantly, so double the (minimum) stack size when they are in use.
>>   */
>> -#ifdef CONFIG_KASAN
>> +#ifdef CONFIG_KASAN_GENERIC
>>  #define KASAN_SHADOW_SCALE_SHIFT 3
>> +#endif
>> +#ifdef CONFIG_KASAN_SW_TAGS
>> +#define KASAN_SHADOW_SCALE_SHIFT 4
>> +#endif
>> +#ifdef CONFIG_KASAN
>
> ... and remove the constant entirely here, avoiding duplication.
>
> Maybe factor that into a Makefile.kasan if things are going to get much
> more complicated.

Will do in v11, thanks!

Re: [PATCH v10 05/22] kasan: add CONFIG_KASAN_GENERIC and CONFIG_KASAN_SW_TAGS

[Andrey Konovalov]

On Wed, Nov 7, 2018 at 6:04 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Nov 06, 2018 at 06:30:20PM +0100, Andrey Konovalov wrote:
>> This commit splits the current CONFIG_KASAN config option into two:
>> 1. CONFIG_KASAN_GENERIC, that enables the generic KASAN mode (the one
>>    that exists now);
>> 2. CONFIG_KASAN_SW_TAGS, that enables the software tag-based KASAN mode.
>>
>> The name CONFIG_KASAN_SW_TAGS is chosen as in the future we will have
>> another hardware tag-based KASAN mode, that will rely on hardware memory
>> tagging support in arm64.
>>
>> With CONFIG_KASAN_SW_TAGS enabled, compiler options are changed to
>> instrument kernel files with -fsantize=kernel-hwaddress (except the ones
>> for which KASAN_SANITIZE := n is set).
>>
>> Both CONFIG_KASAN_GENERIC and CONFIG_KASAN_SW_TAGS support both
>> CONFIG_KASAN_INLINE and CONFIG_KASAN_OUTLINE instrumentation modes.
>>
>> This commit also adds empty placeholder (for now) implementation of
>> tag-based KASAN specific hooks inserted by the compiler and adjusts
>> common hooks implementation to compile correctly with each of the
>> config options.
>>
>> Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
>> Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
>> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
>> ---
>>  arch/arm64/Kconfig                  |  1 +
>>  include/linux/compiler-clang.h      |  5 +-
>>  include/linux/compiler-gcc.h        |  6 ++
>>  include/linux/compiler_attributes.h | 13 -----
>>  include/linux/kasan.h               | 16 ++++--
>>  lib/Kconfig.kasan                   | 87 +++++++++++++++++++++++------
>>  mm/kasan/Makefile                   |  6 +-
>>  mm/kasan/generic.c                  |  2 +-
>>  mm/kasan/kasan.h                    |  3 +-
>>  mm/kasan/tags.c                     | 75 +++++++++++++++++++++++++
>>  mm/slub.c                           |  2 +-
>>  scripts/Makefile.kasan              | 27 ++++++++-
>>  12 files changed, 201 insertions(+), 42 deletions(-)
>>  create mode 100644 mm/kasan/tags.c
>>
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 787d7850e064..8b331dcfb48e 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -111,6 +111,7 @@ config ARM64
>>       select HAVE_ARCH_JUMP_LABEL
>>       select HAVE_ARCH_JUMP_LABEL_RELATIVE
>>       select HAVE_ARCH_KASAN if !(ARM64_16K_PAGES && ARM64_VA_BITS_48)
>> +     select HAVE_ARCH_KASAN_SW_TAGS if !(ARM64_16K_PAGES && ARM64_VA_BITS_48)
>
> Given this relies on a compiler feature, can we please gate this on
> compiler feature detection? e.g. in some common Kconfig have:
>
>         select CC_HAS_ASAN_HWADDRESS if $(cc-option -fsanitize=kernel-hwaddress)
>
> ... and on arm64 we can do:
>
>         select HAVE_ARCH_KASAN_SW_TAGS if !(ARM64_16K_PAGES && ARM64_VA_BITS_48)
>
> ... and core KASAN Kconfig can have:
>
> config KASAN_SW_TAGS
>         depends on HAVE_ARCH_KASAN_SW_TAGS
>         depends on CC_HAS_ASAN_HWADDRESS
>
> [...]
>
>> +ifeq ($(call cc-option, $(CFLAGS_KASAN) -Werror),)
>> +    ifneq ($(CONFIG_COMPILE_TEST),y)
>> +        $(warning Cannot use CONFIG_KASAN_SW_TAGS: \
>> +            -fsanitize=hwaddress is not supported by compiler)
>> +    endif
>> +endif
>
> ... and then this warning shouldn't be possible, and can go.

Will do in v11, thanks!

Re: [PATCH v10 07/22] kasan: initialize shadow to 0xff for tag-based mode

[Andrey Konovalov]

On Wed, Nov 7, 2018 at 6:08 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Nov 06, 2018 at 06:30:22PM +0100, Andrey Konovalov wrote:
>> A tag-based KASAN shadow memory cell contains a memory tag, that
>> corresponds to the tag in the top byte of the pointer, that points to that
>> memory. The native top byte value of kernel pointers is 0xff, so with
>> tag-based KASAN we need to initialize shadow memory to 0xff.
>>
>> Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
>> Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
>> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
>> ---
>>  arch/arm64/mm/kasan_init.c | 16 ++++++++++++++--
>>  include/linux/kasan.h      |  8 ++++++++
>>  mm/kasan/common.c          |  3 ++-
>>  3 files changed, 24 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/mm/kasan_init.c b/arch/arm64/mm/kasan_init.c
>> index 63527e585aac..18ebc8994a7b 100644
>> --- a/arch/arm64/mm/kasan_init.c
>> +++ b/arch/arm64/mm/kasan_init.c
>> @@ -43,6 +43,15 @@ static phys_addr_t __init kasan_alloc_zeroed_page(int node)
>>       return __pa(p);
>>  }
>>
>> +static phys_addr_t __init kasan_alloc_raw_page(int node)
>> +{
>> +     void *p = memblock_alloc_try_nid_raw(PAGE_SIZE, PAGE_SIZE,
>> +                                             __pa(MAX_DMA_ADDRESS),
>> +                                             MEMBLOCK_ALLOC_ACCESSIBLE,
>> +                                             node);
>> +     return __pa(p);
>> +}
>> +
>>  static pte_t *__init kasan_pte_offset(pmd_t *pmdp, unsigned long addr, int node,
>>                                     bool early)
>>  {
>> @@ -88,7 +97,9 @@ static void __init kasan_pte_populate(pmd_t *pmdp, unsigned long addr,
>>
>>       do {
>>               phys_addr_t page_phys = early ? __pa_symbol(kasan_zero_page)
>> -                                           : kasan_alloc_zeroed_page(node);
>> +                                           : kasan_alloc_raw_page(node);
>> +             if (!early)
>> +                     memset(__va(page_phys), KASAN_SHADOW_INIT, PAGE_SIZE);
>>               next = addr + PAGE_SIZE;
>>               set_pte(ptep, pfn_pte(__phys_to_pfn(page_phys), PAGE_KERNEL));
>>       } while (ptep++, addr = next, addr != end && pte_none(READ_ONCE(*ptep)));
>> @@ -138,6 +149,7 @@ asmlinkage void __init kasan_early_init(void)
>>               KASAN_SHADOW_END - (1UL << (64 - KASAN_SHADOW_SCALE_SHIFT)));
>>       BUILD_BUG_ON(!IS_ALIGNED(KASAN_SHADOW_START, PGDIR_SIZE));
>>       BUILD_BUG_ON(!IS_ALIGNED(KASAN_SHADOW_END, PGDIR_SIZE));
>> +
>>       kasan_pgd_populate(KASAN_SHADOW_START, KASAN_SHADOW_END, NUMA_NO_NODE,
>>                          true);
>>  }
>> @@ -234,7 +246,7 @@ void __init kasan_init(void)
>>               set_pte(&kasan_zero_pte[i],
>>                       pfn_pte(sym_to_pfn(kasan_zero_page), PAGE_KERNEL_RO));
>>
>> -     memset(kasan_zero_page, 0, PAGE_SIZE);
>> +     memset(kasan_zero_page, KASAN_SHADOW_INIT, PAGE_SIZE);
>
> If this isn't going to contain zero, can we please have a preparatory
> patch renaming this to something which isn't misleading?
>
> Perhaps kasan_common_shadow_page?

Will rename to kasan_early_shadow_page in v11, thanks!

>
> Thanks,
> Mark.

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

[Andrey Konovalov]

On Thu, Nov 8, 2018 at 1:22 PM, Mark Rutland <mark.rutland@arm.com> 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@virtuozzo.com>
>> Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
>> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
>> ---
>>  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.

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@arm.com> 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@virtuozzo.com>
> >> Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
> >> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> >> ---
> >>  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, 

Re: [PATCH v10 09/22] kasan: add tag related helper functions

[Andrey Konovalov]

On Wed, Nov 7, 2018 at 6:23 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Nov 06, 2018 at 06:30:24PM +0100, Andrey Konovalov wrote:
>> This commit adds a few helper functions, that are meant to be used to
>> work with tags embedded in the top byte of kernel pointers: to set, to
>> get or to reset (set to 0xff) the top byte.
>>
>> Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
>> Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
>> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
>> ---
>>  arch/arm64/mm/kasan_init.c |  2 ++
>>  include/linux/kasan.h      | 13 +++++++++
>>  mm/kasan/kasan.h           | 55 ++++++++++++++++++++++++++++++++++++++
>>  mm/kasan/tags.c            | 37 +++++++++++++++++++++++++
>>  4 files changed, 107 insertions(+)
>>
>> diff --git a/arch/arm64/mm/kasan_init.c b/arch/arm64/mm/kasan_init.c
>> index 18ebc8994a7b..370b19d0e2fb 100644
>> --- a/arch/arm64/mm/kasan_init.c
>> +++ b/arch/arm64/mm/kasan_init.c
>> @@ -249,6 +249,8 @@ void __init kasan_init(void)
>>       memset(kasan_zero_page, KASAN_SHADOW_INIT, PAGE_SIZE);
>>       cpu_replace_ttbr1(lm_alias(swapper_pg_dir));
>>
>> +     kasan_init_tags();
>> +
>>       /* At this point kasan is fully initialized. Enable error messages */
>>       init_task.kasan_depth = 0;
>>       pr_info("KernelAddressSanitizer initialized\n");
>> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
>> index 7f6574c35c62..4c9d6f9029f2 100644
>> --- a/include/linux/kasan.h
>> +++ b/include/linux/kasan.h
>> @@ -169,6 +169,19 @@ static inline void kasan_cache_shutdown(struct kmem_cache *cache) {}
>>
>>  #define KASAN_SHADOW_INIT 0xFF
>>
>> +void kasan_init_tags(void);
>> +
>> +void *kasan_reset_tag(const void *addr);
>> +
>> +#else /* CONFIG_KASAN_SW_TAGS */
>> +
>> +static inline void kasan_init_tags(void) { }
>> +
>> +static inline void *kasan_reset_tag(const void *addr)
>> +{
>> +     return (void *)addr;
>> +}
>> +
>
>> +#ifdef CONFIG_KASAN_SW_TAGS
>> +
>> +#define KASAN_PTR_TAG_SHIFT 56
>> +#define KASAN_PTR_TAG_MASK (0xFFUL << KASAN_PTR_TAG_SHIFT)
>> +
>> +u8 random_tag(void);
>> +
>> +static inline void *set_tag(const void *addr, u8 tag)
>> +{
>> +     u64 a = (u64)addr;
>> +
>> +     a &= ~KASAN_PTR_TAG_MASK;
>> +     a |= ((u64)tag << KASAN_PTR_TAG_SHIFT);
>> +
>> +     return (void *)a;
>> +}
>> +
>> +static inline u8 get_tag(const void *addr)
>> +{
>> +     return (u8)((u64)addr >> KASAN_PTR_TAG_SHIFT);
>> +}
>> +
>> +static inline void *reset_tag(const void *addr)
>> +{
>> +     return set_tag(addr, KASAN_TAG_KERNEL);
>> +}
>
> We seem to be duplicating this functionality in several places.
>
> Could we please make it so that the arch code defines macros:
>
> arch_kasan_set_tag(addr, tag)
> arch_kasan_get_tag(addr)
> arch_kasan_reset_tag(addr)
>
> ... and use thoses consistently rather than open-coding them?
>
>> +
>> +#else /* CONFIG_KASAN_SW_TAGS */
>> +
>> +static inline u8 random_tag(void)
>> +{
>> +     return 0;
>> +}
>> +
>> +static inline void *set_tag(const void *addr, u8 tag)
>> +{
>> +     return (void *)addr;
>> +}
>> +
>> +static inline u8 get_tag(const void *addr)
>> +{
>> +     return 0;
>> +}
>> +
>> +static inline void *reset_tag(const void *addr)
>> +{
>> +     return (void *)addr;
>> +}
>
> ... these can be defined in linux/kasan.h as:
>
> #define arch_kasan_set_tag(addr, tag)   (addr)
> #define arch_kasan_get_tag(addr)        0
> #define arch_kasan_reset_tag(addr)      (addr)

Will do in v11, thanks!

Re: [PATCH v10 08/22] kasan, arm64: untag address in __kimg_to_phys and _virt_addr_is_linear

[Andrey Konovalov]

On Wed, Nov 7, 2018 at 5:52 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi Andrey,
>
> On Tue, Nov 06, 2018 at 06:30:23PM +0100, Andrey Konovalov wrote:
>> __kimg_to_phys (which is used by virt_to_phys) and _virt_addr_is_linear
>> (which is used by virt_addr_valid) assume that the top byte of the address
>> is 0xff, which isn't always the case with tag-based KASAN.
>
> I'm confused by this. Why/when do kimg address have a non-default tag?
>
> Any kimg address is part of the static kernel image, so it's not obvious
> to me how a kimg address would gain a tag. Could you please explain how
> this happens in the commit message?

If kimg address always points into the kernel image, then it shouldn't
be tagged, and this can be removed.

>
>> This patch resets the tag in those macros.
>>
>> Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
>> Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
>> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
>> ---
>>  arch/arm64/include/asm/memory.h | 14 ++++++++++++--
>>  1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
>> index 0f1e024a951f..3226a0218b0b 100644
>> --- a/arch/arm64/include/asm/memory.h
>> +++ b/arch/arm64/include/asm/memory.h
>> @@ -92,6 +92,15 @@
>>  #define KASAN_THREAD_SHIFT   0
>>  #endif
>>
>> +#ifdef CONFIG_KASAN_SW_TAGS
>> +#define KASAN_TAG_SHIFTED(tag)               ((unsigned long)(tag) << 56)
>> +#define KASAN_SET_TAG(addr, tag)     (((addr) & ~KASAN_TAG_SHIFTED(0xff)) | \
>> +                                             KASAN_TAG_SHIFTED(tag))
>> +#define KASAN_RESET_TAG(addr)                KASAN_SET_TAG(addr, 0xff)
>> +#else
>> +#define KASAN_RESET_TAG(addr)                addr
>> +#endif
>
> Nit: the rest of the helper macros in this file are lower-case, with
> specialised helpers prefixed with several underscores. Could we please
> stick with that convention?
>
> e.g. have __tag_set() and __tag_reset() helpers.

Will do in v11.

>
>> +
>>  #define MIN_THREAD_SHIFT     (14 + KASAN_THREAD_SHIFT)
>>
>>  /*
>> @@ -232,7 +241,7 @@ static inline unsigned long kaslr_offset(void)
>>  #define __is_lm_address(addr)        (!!((addr) & BIT(VA_BITS - 1)))
>>
>>  #define __lm_to_phys(addr)   (((addr) & ~PAGE_OFFSET) + PHYS_OFFSET)
>> -#define __kimg_to_phys(addr) ((addr) - kimage_voffset)
>> +#define __kimg_to_phys(addr) (KASAN_RESET_TAG(addr) - kimage_voffset)
>
> IIUC You need to adjust __lm_to_phys() too, since that could be passed
> an address from SLAB.
>
> Maybe that's done in a later patch, but if so it's confusing to split it
> out that way. It would be nicer to fix all the *_to_*() helpers in one
> go.

__lm_to_phys() does & ~PAGE_OFFSET, so it resets the tag by itself. I
can add an explicit __tag_reset() if you think it makes sense.

>
>>
>>  #define __virt_to_phys_nodebug(x) ({                                 \
>>       phys_addr_t __x = (phys_addr_t)(x);                             \
>> @@ -308,7 +317,8 @@ static inline void *phys_to_virt(phys_addr_t x)
>>  #endif
>>  #endif
>>
>> -#define _virt_addr_is_linear(kaddr)  (((u64)(kaddr)) >= PAGE_OFFSET)
>> +#define _virt_addr_is_linear(kaddr)  (KASAN_RESET_TAG((u64)(kaddr)) >= \
>> +                                             PAGE_OFFSET)
>
> This is painful to read. Could you please split this like:
>
> #define _virt_addr_is_linear(kaddr) \
>         (__tag_reset((u64)(kaddr)) >= PAGE_OFFSET)
>
> ... and we could reformat virt_addr_valid() in the same way while we're
> at it.

Will do in v11.

Thanks!

Re: [PATCH v10 08/22] kasan, arm64: untag address in __kimg_to_phys and _virt_addr_is_linear

[Andrey Konovalov]

On Wed, Nov 7, 2018 at 7:10 PM, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Tue, Nov 06, 2018 at 06:30:23PM +0100, Andrey Konovalov wrote:
>> --- a/arch/arm64/include/asm/memory.h
>> +++ b/arch/arm64/include/asm/memory.h
>> @@ -92,6 +92,15 @@
>>  #define KASAN_THREAD_SHIFT   0
>>  #endif
>>
>> +#ifdef CONFIG_KASAN_SW_TAGS
>> +#define KASAN_TAG_SHIFTED(tag)               ((unsigned long)(tag) << 56)
>> +#define KASAN_SET_TAG(addr, tag)     (((addr) & ~KASAN_TAG_SHIFTED(0xff)) | \
>> +                                             KASAN_TAG_SHIFTED(tag))
>> +#define KASAN_RESET_TAG(addr)                KASAN_SET_TAG(addr, 0xff)
>> +#else
>> +#define KASAN_RESET_TAG(addr)                addr
>> +#endif
>
> I think we should reuse the untagged_addr() macro we have in uaccess.h
> (make it more general and move to another header file).

Will do in v11, thanks!

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

[Andrey Konovalov]

On Tue, Nov 13, 2018 at 11:07 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> 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@arm.com> 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@virtuozzo.com>
>> >> Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
>> >> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
>> >> ---
>> >>  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).

Do I understand correctly that TTBR0 means user space addresses and
TTBR1 means kernel addresses? In that commit I see that the
clear_address_tag() macro is used in el0_da and in el1_da, which means
that it untags both user and kernel addresses (on data aborts). Do I
misunderstand something?

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

[Mark Rutland]

On Wed, Nov 14, 2018 at 09:06:23PM +0100, Andrey Konovalov wrote:
> On Tue, Nov 13, 2018 at 11:07 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > 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@arm.com> 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@virtuozzo.com>
> >> >> Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
> >> >> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> >> >> ---
> >> >>  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).
> 
> Do I understand correctly that TTBR0 means user space addresses and
> TTBR1 means kernel addresses? 

Effectively, yes. The address space is split into two halves (with a gap in the
middle). The high half (where we map the kernel) is covered by TTBR1, and the
low half (where we map userspace) is covered by TTBR0.

The TTBRs are the Translation Table Base Registers -- the two halves have
separate page tables.

> In that commit I see that the clear_address_tag() macro is used in el0_da and
> in el1_da, which means that it untags both user and kernel addresses (on data
> aborts). Do I misunderstand something?

It's called for faults taken from EL0 and EL1, but it only removes the tags
from addresses covered by TTBR0. The logic is:

	.macro  clear_address_tag, dst, addr
	tst     \addr, #(1 << 55)
	bic     \dst, \addr, #(0xff << 56)
	csel    \dst, \dst, \addr, eq
	.endm

... which in C would be:

	if (!(addr & (1UL << 55))) {
		addr &= ~(0xffUL << 56);
	}

... and therefore does not affect TTBR1 addresses.

Thanks,
Mark.

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

[Andrey Konovalov]

On Wed, Nov 14, 2018 at 9:17 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Nov 14, 2018 at 09:06:23PM +0100, Andrey Konovalov wrote:
>> On Tue, Nov 13, 2018 at 11:07 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > 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@arm.com> 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@virtuozzo.com>
>> >> >> Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
>> >> >> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
>> >> >> ---
>> >> >>  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).
>>
>> Do I understand correctly that TTBR0 means user space addresses and
>> TTBR1 means kernel addresses?
>
> Effectively, yes. The address space is split into two halves (with a gap in the
> middle). The high half (where we map the kernel) is covered by TTBR1, and the
> low half (where we map userspace) is covered by TTBR0.
>
> The TTBRs are the Translation Table Base Registers -- the two halves have
> separate page tables.
>
>> In that commit I see that the clear_address_tag() macro is used in el0_da and
>> in el1_da, which means that it untags both user and kernel addresses (on data
>> aborts). Do I misunderstand something?
>
> It's called for faults taken from EL0 and EL1, but it only removes the tags
> from addresses covered by TTBR0. The logic is:
>
>         .macro  clear_address_tag, dst, addr
>         tst     \addr, #(1 << 55)
>         bic     \dst, \addr, #(0xff << 56)
>         csel    \dst, \dst, \addr, eq
>         .endm
>
> ... which in C would be:
>
>         if (!(addr & (1UL << 55))) {
>                 addr &= ~(0xffUL << 56);
>         }
>
> ... and therefore does not affect TTBR1 addresses.

Got it, will fix in v11, thanks!

Re: [PATCH v10 08/22] kasan, arm64: untag address in __kimg_to_phys and _virt_addr_is_linear

[Andrey Konovalov]

On Wed, Nov 14, 2018 at 8:23 PM, Andrey Konovalov <andreyknvl@google.com> wrote:
> On Wed, Nov 7, 2018 at 5:52 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>>>  /*
>>> @@ -232,7 +241,7 @@ static inline unsigned long kaslr_offset(void)
>>>  #define __is_lm_address(addr)        (!!((addr) & BIT(VA_BITS - 1)))
>>>
>>>  #define __lm_to_phys(addr)   (((addr) & ~PAGE_OFFSET) + PHYS_OFFSET)
>>> -#define __kimg_to_phys(addr) ((addr) - kimage_voffset)
>>> +#define __kimg_to_phys(addr) (KASAN_RESET_TAG(addr) - kimage_voffset)
>>
>> IIUC You need to adjust __lm_to_phys() too, since that could be passed
>> an address from SLAB.
>>
>> Maybe that's done in a later patch, but if so it's confusing to split it
>> out that way. It would be nicer to fix all the *_to_*() helpers in one
>> go.
>
> __lm_to_phys() does & ~PAGE_OFFSET, so it resets the tag by itself. I
> can add an explicit __tag_reset() if you think it makes sense.

Hi Mark,

I think I've addressed all of your comments except for this one. Do
you think it makes sense to add explicit __tag_reset() calls to
__lm_to_phys() and a few other macros, that already set the tag to 0
by doing & ~PAGE_OFFSET?

Thanks!