Re: [PATCH v3 1/3] arm64: unwind: add asynchronous unwind tables to kernel and modules

From: Fangrui Song <maskray@google.com>
To: Ard Biesheuvel <ardb@kernel.org>
Cc: Kees Cook <keescook@chromium.org>,
	Sami Tolvanen <samitolvanen@google.com>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Marc Zyngier <maz@kernel.org>,
	Nick Desaulniers <ndesaulniers@google.com>,
	Dan Li <ashimida@linux.alibaba.com>
Subject: Re: [PATCH v3 1/3] arm64: unwind: add asynchronous unwind tables to kernel and modules
Date: Wed, 15 Jun 2022 14:52:02 -0700	[thread overview]
Message-ID: <20220615215202.rxv42bvrfwhs6cgl@google.com> (raw)
In-Reply-To: <CAGXu5jJpOQ_2MVVPxSf9x-p-3Utd4RDnBe4hXg5_-hyN3DicfQ@mail.gmail.com>

On 2022-06-15, Kees Cook wrote:
>On Wed, Jun 15, 2022 at 9:54 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> On Wed, 15 Jun 2022 at 18:50, Sami Tolvanen <samitolvanen@google.com> wrote:
>> >
>> > On Mon, Jun 13, 2022 at 03:40:06PM +0200, Ard Biesheuvel wrote:
>> > > Enable asynchronous unwind table generation for both the core kernel as
>> > > well as modules, and emit the resulting .eh_frame sections as init code
>> > > so we can use the unwind directives for code patching at boot or module
>> > > load time.
>> > >
>> > > This will be used by dynamic shadow call stack support, which will rely
>> > > on code patching rather than compiler codegen to emit the shadow call
>> > > stack push and pop instructions.
>> > >
>> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>> > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
>> > > ---
>> > >  arch/arm64/Kconfig                    |  3 +++
>> > >  arch/arm64/Makefile                   |  5 +++++
>> > >  arch/arm64/include/asm/module.lds.h   |  8 ++++++++
>> > >  arch/arm64/kernel/vmlinux.lds.S       | 13 +++++++++++++
>> > >  arch/arm64/kvm/hyp/nvhe/Makefile      |  1 +
>> > >  drivers/firmware/efi/libstub/Makefile |  1 +
>> > >  6 files changed, 31 insertions(+)
>> > >
>> > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> > > index 1652a9800ebe..5f92344edff5 100644
>> > > --- a/arch/arm64/Kconfig
>> > > +++ b/arch/arm64/Kconfig
>> > > @@ -366,6 +366,9 @@ config KASAN_SHADOW_OFFSET
>> > >       default 0xeffffff800000000 if ARM64_VA_BITS_36 && KASAN_SW_TAGS
>> > >       default 0xffffffffffffffff
>> > >
>> > > +config UNWIND_TABLES
>> > > +     bool
>> > > +
>> > >  source "arch/arm64/Kconfig.platforms"
>> > >
>> > >  menu "Kernel Features"
>> > > diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
>> > > index 6d9d4a58b898..4fbca56fa602 100644
>> > > --- a/arch/arm64/Makefile
>> > > +++ b/arch/arm64/Makefile
>> > > @@ -45,8 +45,13 @@ KBUILD_CFLAGS      += $(call cc-option,-mabi=lp64)
>> > >  KBUILD_AFLAGS        += $(call cc-option,-mabi=lp64)
>> > >
>> > >  # Avoid generating .eh_frame* sections.
>> > > +ifneq ($(CONFIG_UNWIND_TABLES),y)
>> > >  KBUILD_CFLAGS        += -fno-asynchronous-unwind-tables -fno-unwind-tables
>> > >  KBUILD_AFLAGS        += -fno-asynchronous-unwind-tables -fno-unwind-tables
>> > > +else
>> > > +KBUILD_CFLAGS        += -fasynchronous-unwind-tables
>> > > +KBUILD_AFLAGS        += -fasynchronous-unwind-tables
>> > > +endif
>> > >
>> > >  ifeq ($(CONFIG_STACKPROTECTOR_PER_TASK),y)
>> > >  prepare: stack_protector_prepare
>> > > diff --git a/arch/arm64/include/asm/module.lds.h b/arch/arm64/include/asm/module.lds.h
>> > > index 094701ec5500..dbba4b7559aa 100644
>> > > --- a/arch/arm64/include/asm/module.lds.h
>> > > +++ b/arch/arm64/include/asm/module.lds.h
>> > > @@ -17,4 +17,12 @@ SECTIONS {
>> > >        */
>> > >       .text.hot : { *(.text.hot) }
>> > >  #endif
>> > > +
>> > > +#ifdef CONFIG_UNWIND_TABLES
>> > > +     /*
>> > > +      * Currently, we only use unwind info at module load time, so we can
>> > > +      * put it into the .init allocation.
>> > > +      */
>> > > +     .init.eh_frame : { *(.eh_frame) }
>> > > +#endif
>> > >  }
>> > > diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
>> > > index 2d4a8f995175..7bf4809f523d 100644
>> > > --- a/arch/arm64/kernel/vmlinux.lds.S
>> > > +++ b/arch/arm64/kernel/vmlinux.lds.S
>> > > @@ -120,6 +120,17 @@ jiffies = jiffies_64;
>> > >  #define TRAMP_TEXT
>> > >  #endif
>> > >
>> > > +#ifdef CONFIG_UNWIND_TABLES
>> > > +#define UNWIND_DATA_SECTIONS                         \
>> > > +     .eh_frame : {                                   \
>> > > +             __eh_frame_start = .;                   \
>> > > +             *(.eh_frame)                            \
>> > > +             __eh_frame_end = .;                     \
>> > > +     }
>> > > +#else
>> > > +#define UNWIND_DATA_SECTIONS
>> > > +#endif

How do you intend to use the encapsulation symbols __eh_frame_start
and __eh_frame_end ?

>> > How does this work with SANITIZER_DISCARDS dropping .eh_frame in
>> > include/asm-generic/vmlinux.lds.h and scripts/module.lds.S? We would
>> > definitely want to enable this together with CONFIG_CFI_CLANG, so it
>> > seems like we'd have to drop the discard rules as well.
>> >
>>
>> Good point, I had no idea that that existed.
>>
>> Clang 13 should have the fix for the original issue, so we could make
>> this workaround specific to 12 and earlier.
>
>Yeah, I like this -- I'd prefer to know when we get "surprise" sections again.

Does this need asynchronous unwind tables or just synchronous unwind
tables?

Note: with Clang < 15, the AArch64 codegen was in a quite bad state. It
has significantly improved since the https://reviews.llvm.org/D114545 patch series
but I am not confident to state that production use may not into an issue :-)

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