All of lore.kernel.org
 help / color / mirror / Atom feed
From: Mark Rutland <mark.rutland@arm.com>
To: Sami Tolvanen <samitolvanen@google.com>,
	Ard Biesheuvel <ardb@kernel.org>
Cc: Kees Cook <keescook@chromium.org>,
	Nathan Chancellor <nathan@kernel.org>,
	Nick Desaulniers <ndesaulniers@google.com>,
	Masahiro Yamada <masahiroy@kernel.org>,
	Will Deacon <will@kernel.org>, Jessica Yu <jeyu@kernel.org>,
	Arnd Bergmann <arnd@arndb.de>, Tejun Heo <tj@kernel.org>,
	"Paul E. McKenney" <paulmck@kernel.org>,
	Christoph Hellwig <hch@infradead.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Sedat Dilek <sedat.dilek@gmail.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	bpf@vger.kernel.org, linux-hardening@vger.kernel.org,
	linux-arch@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kbuild@vger.kernel.org, linux-pci@vger.kernel.org,
	linux-kernel@vger.kernel.org, clang-built-linux@googlegroups.com,
	ardb@kernel.org
Subject: Re: [PATCH v5 14/18] arm64: add __nocfi to functions that jump to a physical address
Date: Tue, 6 Apr 2021 12:53:57 +0100	[thread overview]
Message-ID: <20210406115357.GE96480@C02TD0UTHF1T.local> (raw)
In-Reply-To: <20210401233216.2540591-15-samitolvanen@google.com>

[adding Ard for EFI runtime services bits]

On Thu, Apr 01, 2021 at 04:32:12PM -0700, Sami Tolvanen wrote:
> Disable CFI checking for functions that switch to linear mapping and
> make an indirect call to a physical address, since the compiler only
> understands virtual addresses and the CFI check for such indirect calls
> would always fail.

What does physical vs virtual have to do with this? Does the address
actually matter, or is this just a general thing that when calling an
assembly function we won't have a trampoline that the caller expects?

I wonder if we need to do something with asmlinkage here, perhaps?

I didn't spot anything in the seriues handling EFI runtime services
calls, and I strongly suspect we need to do something for those, unless
they're handled implicitly by something else.

> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> ---
>  arch/arm64/include/asm/mmu_context.h | 2 +-
>  arch/arm64/kernel/cpu-reset.h        | 8 ++++----
>  arch/arm64/kernel/cpufeature.c       | 2 +-
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h
> index 386b96400a57..d3cef9133539 100644
> --- a/arch/arm64/include/asm/mmu_context.h
> +++ b/arch/arm64/include/asm/mmu_context.h
> @@ -119,7 +119,7 @@ static inline void cpu_install_idmap(void)
>   * Atomically replaces the active TTBR1_EL1 PGD with a new VA-compatible PGD,
>   * avoiding the possibility of conflicting TLB entries being allocated.
>   */
> -static inline void cpu_replace_ttbr1(pgd_t *pgdp)
> +static inline void __nocfi cpu_replace_ttbr1(pgd_t *pgdp)

Given these are inlines, what's the effect when these are inlined into a
function that would normally use CFI? Does CFI get supressed for the
whole function, or just the bit that got inlined?

Is there an attribute that we could place on a function pointer to tell
the compiler to not check calls via that pointer? If that existed we'd
be able to scope this much more tightly.

Thanks,
Mark.

>  {
>  	typedef void (ttbr_replace_func)(phys_addr_t);
>  	extern ttbr_replace_func idmap_cpu_replace_ttbr1;
> diff --git a/arch/arm64/kernel/cpu-reset.h b/arch/arm64/kernel/cpu-reset.h
> index f3adc574f969..9a7b1262ef17 100644
> --- a/arch/arm64/kernel/cpu-reset.h
> +++ b/arch/arm64/kernel/cpu-reset.h
> @@ -13,10 +13,10 @@
>  void __cpu_soft_restart(unsigned long el2_switch, unsigned long entry,
>  	unsigned long arg0, unsigned long arg1, unsigned long arg2);
>  
> -static inline void __noreturn cpu_soft_restart(unsigned long entry,
> -					       unsigned long arg0,
> -					       unsigned long arg1,
> -					       unsigned long arg2)
> +static inline void __noreturn __nocfi cpu_soft_restart(unsigned long entry,
> +						       unsigned long arg0,
> +						       unsigned long arg1,
> +						       unsigned long arg2)
>  {
>  	typeof(__cpu_soft_restart) *restart;
>  
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 0b2e0d7b13ec..c2f94a5206e0 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -1445,7 +1445,7 @@ static bool unmap_kernel_at_el0(const struct arm64_cpu_capabilities *entry,
>  }
>  
>  #ifdef CONFIG_UNMAP_KERNEL_AT_EL0
> -static void
> +static void __nocfi
>  kpti_install_ng_mappings(const struct arm64_cpu_capabilities *__unused)
>  {
>  	typedef void (kpti_remap_fn)(int, int, phys_addr_t);
> -- 
> 2.31.0.208.g409f899ff0-goog
> 

WARNING: multiple messages have this Message-ID (diff)
From: Mark Rutland <mark.rutland@arm.com>
To: Sami Tolvanen <samitolvanen@google.com>,
	Ard Biesheuvel <ardb@kernel.org>
Cc: Kees Cook <keescook@chromium.org>,
	Nathan Chancellor <nathan@kernel.org>,
	Nick Desaulniers <ndesaulniers@google.com>,
	Masahiro Yamada <masahiroy@kernel.org>,
	Will Deacon <will@kernel.org>, Jessica Yu <jeyu@kernel.org>,
	Arnd Bergmann <arnd@arndb.de>, Tejun Heo <tj@kernel.org>,
	"Paul E. McKenney" <paulmck@kernel.org>,
	Christoph Hellwig <hch@infradead.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Sedat Dilek <sedat.dilek@gmail.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	bpf@vger.kernel.org, linux-hardening@vger.kernel.org,
	linux-arch@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kbuild@vger.kernel.org, linux-pci@vger.kernel.org,
	linux-kernel@vger.kernel.org, clang-built-linux@googlegroups.com,
	ardb@kernel.org
Subject: Re: [PATCH v5 14/18] arm64: add __nocfi to functions that jump to a physical address
Date: Tue, 6 Apr 2021 12:53:57 +0100	[thread overview]
Message-ID: <20210406115357.GE96480@C02TD0UTHF1T.local> (raw)
In-Reply-To: <20210401233216.2540591-15-samitolvanen@google.com>

[adding Ard for EFI runtime services bits]

On Thu, Apr 01, 2021 at 04:32:12PM -0700, Sami Tolvanen wrote:
> Disable CFI checking for functions that switch to linear mapping and
> make an indirect call to a physical address, since the compiler only
> understands virtual addresses and the CFI check for such indirect calls
> would always fail.

What does physical vs virtual have to do with this? Does the address
actually matter, or is this just a general thing that when calling an
assembly function we won't have a trampoline that the caller expects?

I wonder if we need to do something with asmlinkage here, perhaps?

I didn't spot anything in the seriues handling EFI runtime services
calls, and I strongly suspect we need to do something for those, unless
they're handled implicitly by something else.

> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> ---
>  arch/arm64/include/asm/mmu_context.h | 2 +-
>  arch/arm64/kernel/cpu-reset.h        | 8 ++++----
>  arch/arm64/kernel/cpufeature.c       | 2 +-
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h
> index 386b96400a57..d3cef9133539 100644
> --- a/arch/arm64/include/asm/mmu_context.h
> +++ b/arch/arm64/include/asm/mmu_context.h
> @@ -119,7 +119,7 @@ static inline void cpu_install_idmap(void)
>   * Atomically replaces the active TTBR1_EL1 PGD with a new VA-compatible PGD,
>   * avoiding the possibility of conflicting TLB entries being allocated.
>   */
> -static inline void cpu_replace_ttbr1(pgd_t *pgdp)
> +static inline void __nocfi cpu_replace_ttbr1(pgd_t *pgdp)

Given these are inlines, what's the effect when these are inlined into a
function that would normally use CFI? Does CFI get supressed for the
whole function, or just the bit that got inlined?

Is there an attribute that we could place on a function pointer to tell
the compiler to not check calls via that pointer? If that existed we'd
be able to scope this much more tightly.

Thanks,
Mark.

>  {
>  	typedef void (ttbr_replace_func)(phys_addr_t);
>  	extern ttbr_replace_func idmap_cpu_replace_ttbr1;
> diff --git a/arch/arm64/kernel/cpu-reset.h b/arch/arm64/kernel/cpu-reset.h
> index f3adc574f969..9a7b1262ef17 100644
> --- a/arch/arm64/kernel/cpu-reset.h
> +++ b/arch/arm64/kernel/cpu-reset.h
> @@ -13,10 +13,10 @@
>  void __cpu_soft_restart(unsigned long el2_switch, unsigned long entry,
>  	unsigned long arg0, unsigned long arg1, unsigned long arg2);
>  
> -static inline void __noreturn cpu_soft_restart(unsigned long entry,
> -					       unsigned long arg0,
> -					       unsigned long arg1,
> -					       unsigned long arg2)
> +static inline void __noreturn __nocfi cpu_soft_restart(unsigned long entry,
> +						       unsigned long arg0,
> +						       unsigned long arg1,
> +						       unsigned long arg2)
>  {
>  	typeof(__cpu_soft_restart) *restart;
>  
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 0b2e0d7b13ec..c2f94a5206e0 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -1445,7 +1445,7 @@ static bool unmap_kernel_at_el0(const struct arm64_cpu_capabilities *entry,
>  }
>  
>  #ifdef CONFIG_UNMAP_KERNEL_AT_EL0
> -static void
> +static void __nocfi
>  kpti_install_ng_mappings(const struct arm64_cpu_capabilities *__unused)
>  {
>  	typedef void (kpti_remap_fn)(int, int, phys_addr_t);
> -- 
> 2.31.0.208.g409f899ff0-goog
> 

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

  reply	other threads:[~2021-04-06 11:54 UTC|newest]

Thread overview: 60+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-01 23:31 [PATCH v5 00/18] Add support for Clang CFI Sami Tolvanen
2021-04-01 23:31 ` Sami Tolvanen
2021-04-01 23:31 ` [PATCH v5 01/18] add " Sami Tolvanen
2021-04-01 23:31   ` Sami Tolvanen
2021-04-01 23:32 ` [PATCH v5 02/18] cfi: add __cficanonical Sami Tolvanen
2021-04-01 23:32   ` Sami Tolvanen
2021-04-01 23:32 ` [PATCH v5 03/18] mm: add generic function_nocfi macro Sami Tolvanen
2021-04-01 23:32   ` Sami Tolvanen
2021-04-02  6:37   ` Christoph Hellwig
2021-04-02  6:37     ` Christoph Hellwig
2021-04-06 11:27   ` Mark Rutland
2021-04-06 11:27     ` Mark Rutland
2021-04-01 23:32 ` [PATCH v5 04/18] module: ensure __cfi_check alignment Sami Tolvanen
2021-04-01 23:32   ` Sami Tolvanen
2021-04-01 23:32 ` [PATCH v5 05/18] workqueue: use WARN_ON_FUNCTION_MISMATCH Sami Tolvanen
2021-04-01 23:32   ` Sami Tolvanen
2021-04-01 23:32 ` [PATCH v5 06/18] kthread: " Sami Tolvanen
2021-04-01 23:32   ` Sami Tolvanen
2021-04-01 23:32 ` [PATCH v5 07/18] kallsyms: strip ThinLTO hashes from static functions Sami Tolvanen
2021-04-01 23:32   ` Sami Tolvanen
2021-04-01 23:32 ` [PATCH v5 08/18] bpf: disable CFI in dispatcher functions Sami Tolvanen
2021-04-01 23:32   ` Sami Tolvanen
2021-04-01 23:32 ` [PATCH v5 09/18] treewide: Change list_sort to use const pointers Sami Tolvanen
2021-04-01 23:32   ` Sami Tolvanen
2021-04-01 23:32 ` [PATCH v5 10/18] lkdtm: use function_nocfi Sami Tolvanen
2021-04-01 23:32   ` Sami Tolvanen
2021-04-01 23:32 ` [PATCH v5 11/18] psci: use function_nocfi for cpu_resume Sami Tolvanen
2021-04-01 23:32   ` Sami Tolvanen
2021-04-06 11:27   ` Mark Rutland
2021-04-06 11:27     ` Mark Rutland
2021-04-01 23:32 ` [PATCH v5 12/18] arm64: implement function_nocfi Sami Tolvanen
2021-04-01 23:32   ` Sami Tolvanen
2021-04-06 11:36   ` Mark Rutland
2021-04-06 11:36     ` Mark Rutland
2021-04-06 17:02     ` Sami Tolvanen
2021-04-06 17:02       ` Sami Tolvanen
2021-04-01 23:32 ` [PATCH v5 13/18] arm64: use function_nocfi with __pa_symbol Sami Tolvanen
2021-04-01 23:32   ` Sami Tolvanen
2021-04-06 11:41   ` Mark Rutland
2021-04-06 11:41     ` Mark Rutland
2021-04-01 23:32 ` [PATCH v5 14/18] arm64: add __nocfi to functions that jump to a physical address Sami Tolvanen
2021-04-01 23:32   ` Sami Tolvanen
2021-04-06 11:53   ` Mark Rutland [this message]
2021-04-06 11:53     ` Mark Rutland
2021-04-06 12:59     ` Ard Biesheuvel
2021-04-06 12:59       ` Ard Biesheuvel
2021-04-06 17:43     ` Sami Tolvanen
2021-04-06 17:43       ` Sami Tolvanen
2021-04-01 23:32 ` [PATCH v5 15/18] arm64: add __nocfi to __apply_alternatives Sami Tolvanen
2021-04-01 23:32   ` Sami Tolvanen
2021-04-01 23:32 ` [PATCH v5 16/18] arm64: ftrace: use function_nocfi for ftrace_call Sami Tolvanen
2021-04-01 23:32   ` Sami Tolvanen
2021-04-06 11:58   ` Mark Rutland
2021-04-06 11:58     ` Mark Rutland
2021-04-01 23:32 ` [PATCH v5 17/18] KVM: arm64: Disable CFI for nVHE Sami Tolvanen
2021-04-01 23:32   ` Sami Tolvanen
2021-04-01 23:32 ` [PATCH v5 18/18] arm64: allow CONFIG_CFI_CLANG to be selected Sami Tolvanen
2021-04-01 23:32   ` Sami Tolvanen
2021-04-02 19:58 ` [PATCH v5 00/18] Add support for Clang CFI Nathan Chancellor
2021-04-02 19:58   ` Nathan Chancellor

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210406115357.GE96480@C02TD0UTHF1T.local \
    --to=mark.rutland@arm.com \
    --cc=ardb@kernel.org \
    --cc=arnd@arndb.de \
    --cc=bpf@vger.kernel.org \
    --cc=catalin.marinas@arm.com \
    --cc=clang-built-linux@googlegroups.com \
    --cc=hch@infradead.org \
    --cc=jeyu@kernel.org \
    --cc=keescook@chromium.org \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-hardening@vger.kernel.org \
    --cc=linux-kbuild@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=masahiroy@kernel.org \
    --cc=nathan@kernel.org \
    --cc=ndesaulniers@google.com \
    --cc=paulmck@kernel.org \
    --cc=peterz@infradead.org \
    --cc=samitolvanen@google.com \
    --cc=sedat.dilek@gmail.com \
    --cc=tj@kernel.org \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.