Re: [PATCH v2 08/14] arm64: mask PAC bits of __builtin_return_address

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Richard Henderson <richard.henderson@linaro.org>
Cc: Mark Rutland <mark.rutland@arm.com>,
	Kees Cook <keescook@chromium.org>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will.deacon@arm.com>,
	Kristina Martsenko <kristina.martsenko@arm.com>,
	James Morse <james.morse@arm.com>,
	Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>,
	Amit Daniel Kachhap <amit.kachhap@arm.com>,
	Vincenzo Frascino <Vincenzo.Frascino@arm.com>,
	Dave Martin <Dave.Martin@arm.com>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v2 08/14] arm64: mask PAC bits of __builtin_return_address
Date: Fri, 22 Nov 2019 14:27:02 +0100	[thread overview]
Message-ID: <CAKv+Gu982YqEw4feaAw+W1kiDKYLM+DTPs_w9RU+tjTh3ZkAXQ@mail.gmail.com> (raw)
In-Reply-To: <cdc2fdcf-7a58-a551-253e-adb92180e749@linaro.org>

On Fri, 22 Nov 2019 at 09:48, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 11/21/19 5:42 PM, Ard Biesheuvel wrote:
> > On Tue, 19 Nov 2019 at 13:33, Amit Daniel Kachhap <amit.kachhap@arm.com> wrote:
> >>
> >> This patch redefines __builtin_return_address to mask pac bits
> >> when Pointer Authentication is enabled. As __builtin_return_address
> >> is used mostly used to refer to the caller function symbol address
> >> so masking runtime generated pac bits will help to find the match.
> >>
> >> This change fixes the utilities like cat /proc/vmallocinfo to now
> >> show the correct logs.
> >>
> >> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
> >> ---
> >> Change since last version:
> >>  * Comment modified.
> >>
> >>  arch/arm64/Kconfig                |  1 +
> >>  arch/arm64/include/asm/compiler.h | 17 +++++++++++++++++
> >>  2 files changed, 18 insertions(+)
> >>  create mode 100644 arch/arm64/include/asm/compiler.h
> >>
> >> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> >> index 998248e..c1844de 100644
> >> --- a/arch/arm64/Kconfig
> >> +++ b/arch/arm64/Kconfig
> >> @@ -117,6 +117,7 @@ config ARM64
> >>         select HAVE_ALIGNED_STRUCT_PAGE if SLUB
> >>         select HAVE_ARCH_AUDITSYSCALL
> >>         select HAVE_ARCH_BITREVERSE
> >> +       select HAVE_ARCH_COMPILER_H
> >>         select HAVE_ARCH_HUGE_VMAP
> >>         select HAVE_ARCH_JUMP_LABEL
> >>         select HAVE_ARCH_JUMP_LABEL_RELATIVE
> >> diff --git a/arch/arm64/include/asm/compiler.h b/arch/arm64/include/asm/compiler.h
> >> new file mode 100644
> >> index 0000000..5efe310
> >> --- /dev/null
> >> +++ b/arch/arm64/include/asm/compiler.h
> >> @@ -0,0 +1,17 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +#ifndef __ASM_ARM_COMPILER_H
> >> +#define __ASM_ARM_COMPILER_H
> >> +
> >> +#ifndef __ASSEMBLY__
> >> +
> >> +#if defined(CONFIG_ARM64_PTR_AUTH)
> >> +
> >> +/* As TBI1 is disabled currently, so bits 63:56 also has PAC */
> >> +#define __builtin_return_address(val)                          \
> >> +       (void *)((unsigned long)__builtin_return_address(val) | \
> >> +       (GENMASK_ULL(63, 56) | GENMASK_ULL(54, VA_BITS)))
> >> +#endif
> >> +
> >> +#endif
> >> +
> >> +#endif /* __ASM_ARM_COMPILER_H */
> >
> > It seems to me like we are accumulating a lot of cruft for khwasan as
> > well as PAC to convert address into their untagged format.
> >
> > Are there are untagging helpers we can already reuse? If not, can we
> > introduce something that can be shared between all these use cases?
>
> xpaci will strip the pac from an instruction pointer, but requires the
> instruction set to be enabled, so you'd have to fiddle with alternatives.  You
> *could* force the use of lr as input/output and use xpaclri, which is a nop if
> the instruction set is not enabled.
>
> Also, this definition of is not correct, because bit 55 needs to be propagated
> to all of the bits being masked out here, so that you get a large negative
> number for kernel space addresses.
>

Indeed. Even though bit 55 is generally guaranteed to be set, it would
be better to simply reuse ptrauth_strip_insn_pac() that you introduce
in the next patch.

Also, please use __ASM_COMPILER_H as the header guard (which is more
idiomatic), and drop the unnecessary 'ifndef __ASSEMBLY__'.

Finally, could you add a comment that this header is transitively
included (via include/compiler_types.h) on the compiler command line,
so it is guaranteed to be loaded by users of this macro, and so there
is no risk of the wrong version being used.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel