[PATCH v4 00/15] x86: Add support for Clang CFI

[PATCH v4 00/15] x86: Add support for Clang CFI

[Sami Tolvanen]

This series adds support for Clang's Control-Flow Integrity (CFI)
checking to x86_64. With CFI, the compiler injects a runtime
check before each indirect function call to ensure the target is
a valid function with the correct static type. This restricts
possible call targets and makes it more difficult for an attacker
to exploit bugs that allow the modification of stored function
pointers. For more details, see:

  https://clang.llvm.org/docs/ControlFlowIntegrity.html

Note that v4 is based on tip/master. The first two patches contain
objtool support for CFI, the remaining patches change function
declarations to use opaque types, fix type mismatch issues that
confuse the compiler, and disable CFI where it can't be used.

You can also pull this series from

  https://github.com/samitolvanen/linux.git x86-cfi-v4

---
Changes in v4:
- Dropped the extable patch after the code was refactored in -tip.

- Switched to __section() instead of open-coding the attribute.

- Added an explicit ifdef for filtering out CC_FLAGS_CFI in
  purgatory for readability.

- Added a comment to arch_cfi_jump_reloc_offset() in objtool.

Changes in v3:
- Dropped Clang requirement to >= 13 after the missing compiler
  fix was backported there.

- Added DEFINE_CFI_IMMEDIATE_RETURN_STUB to address the issue
  with tp_stub_func in kernel/tracepoint.c.

- Renamed asm_func_t to asm_func_ptr.

- Changed extable handlers to use __cficanonical instead of
  disabling CFI for fixup_exception.

Changes in v2:
- Dropped the first objtool patch as the warnings were fixed in
  separate patches.

- Changed fix_cfi_relocs() in objtool to not rely on jump table
  symbols, and to return an error if it can't find a relocation.

- Fixed a build issue with ASM_STACK_FRAME_NON_STANDARD().

- Dropped workarounds for inline assembly references to
  address-taken static functions with CFI as this was fixed in
  the compiler.

- Changed the C declarations of non-callable functions to use
  opaque types and dropped the function_nocfi() patches.

- Changed ARCH_SUPPORTS_CFI_CLANG to depend on Clang >=14 for
  the compiler fixes.


Kees Cook (1):
  x86, relocs: Ignore __typeid__ relocations

Sami Tolvanen (14):
  objtool: Add CONFIG_CFI_CLANG support
  objtool: Add ASM_STACK_FRAME_NON_STANDARD
  linkage: Add DECLARE_ASM_FUNC_SYMBOL
  cfi: Add DEFINE_CFI_IMMEDIATE_RETURN_STUB
  tracepoint: Exclude tp_stub_func from CFI checking
  ftrace: Use an opaque type for functions not callable from C
  lkdtm: Disable UNSET_SMEP with CFI
  lkdtm: Use an opaque type for lkdtm_rodata_do_nothing
  x86: Use an opaque type for functions not callable from C
  x86/purgatory: Disable CFI
  x86, module: Ignore __typeid__ relocations
  x86, cpu: Use LTO for cpu.c with CFI
  x86, kprobes: Fix optprobe_template_func type mismatch
  x86, build: Allow CONFIG_CFI_CLANG to be selected

 arch/x86/Kconfig                      |  1 +
 arch/x86/include/asm/ftrace.h         |  2 +-
 arch/x86/include/asm/idtentry.h       | 10 +++---
 arch/x86/include/asm/page_64.h        |  7 ++--
 arch/x86/include/asm/paravirt_types.h |  3 +-
 arch/x86/include/asm/processor.h      |  2 +-
 arch/x86/include/asm/proto.h          | 25 ++++++-------
 arch/x86/include/asm/uaccess_64.h     |  9 ++---
 arch/x86/kernel/alternative.c         |  2 +-
 arch/x86/kernel/ftrace.c              |  2 +-
 arch/x86/kernel/kprobes/opt.c         |  4 +--
 arch/x86/kernel/module.c              |  4 +++
 arch/x86/kernel/paravirt.c            |  4 +--
 arch/x86/kvm/emulate.c                |  4 +--
 arch/x86/kvm/kvm_emulate.h            |  9 ++---
 arch/x86/power/Makefile               |  2 ++
 arch/x86/purgatory/Makefile           |  4 +++
 arch/x86/tools/relocs.c               |  7 ++++
 arch/x86/xen/enlighten_pv.c           |  6 ++--
 arch/x86/xen/xen-ops.h                | 10 +++---
 drivers/misc/lkdtm/bugs.c             |  2 +-
 drivers/misc/lkdtm/lkdtm.h            |  2 +-
 drivers/misc/lkdtm/perms.c            |  2 +-
 drivers/misc/lkdtm/rodata.c           |  2 +-
 include/asm-generic/vmlinux.lds.h     | 11 ++++++
 include/linux/cfi.h                   | 13 +++++++
 include/linux/ftrace.h                |  7 ++--
 include/linux/linkage.h               | 13 +++++++
 include/linux/objtool.h               |  6 ++++
 kernel/cfi.c                          | 24 ++++++++++++-
 kernel/tracepoint.c                   |  5 +--
 tools/include/linux/objtool.h         |  6 ++++
 tools/objtool/arch/x86/decode.c       | 17 +++++++++
 tools/objtool/elf.c                   | 51 +++++++++++++++++++++++++++
 tools/objtool/include/objtool/arch.h  |  3 ++
 tools/objtool/include/objtool/elf.h   |  2 +-
 36 files changed, 217 insertions(+), 66 deletions(-)


base-commit: d4bfebd9ef497ee0afb498f6028a5074a6ccf307
-- 
2.33.0.800.g4c38ced690-goog

[PATCH v4 01/15] objtool: Add CONFIG_CFI_CLANG support

[Sami Tolvanen]

With CONFIG_CFI_CLANG, the compiler replaces function references with
references to the CFI jump table, which confuses objtool. This change,
based on Josh's initial patch [1], goes through the list of relocations
and replaces jump table symbols with the actual function symbols.

[1] https://lore.kernel.org/r/d743f4b36e120c06506567a9f87a062ae03da47f.1611263462.git.jpoimboe@redhat.com/

Reported-by: Sedat Dilek <sedat.dilek@gmail.com>
Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
 tools/objtool/arch/x86/decode.c      | 17 ++++++++++
 tools/objtool/elf.c                  | 51 ++++++++++++++++++++++++++++
 tools/objtool/include/objtool/arch.h |  3 ++
 tools/objtool/include/objtool/elf.h  |  2 +-
 4 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
index 3172983bf808..9b043220b0af 100644
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -63,6 +63,23 @@ bool arch_callee_saved_reg(unsigned char reg)
 	}
 }
 
+unsigned long arch_cfi_section_reloc_offset(struct reloc *reloc)
+{
+	if (!reloc->addend)
+		return 0;
+
+	if (reloc->type == R_X86_64_PC32 || reloc->type == R_X86_64_PLT32)
+		return reloc->addend + 4;
+
+	return reloc->addend;
+}
+
+unsigned long arch_cfi_jump_reloc_offset(unsigned long offset)
+{
+	/* offset to the relocation in a jmp instruction */
+	return offset + 1;
+}
+
 unsigned long arch_dest_reloc_offset(int addend)
 {
 	return addend + 4;
diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index 8676c7598728..05a5f51aad2c 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -18,6 +18,7 @@
 #include <errno.h>
 #include <objtool/builtin.h>
 
+#include <objtool/arch.h>
 #include <objtool/elf.h>
 #include <objtool/warn.h>
 
@@ -291,6 +292,10 @@ static int read_sections(struct elf *elf)
 		if (sec->sh.sh_flags & SHF_EXECINSTR)
 			elf->text_size += sec->len;
 
+		/* Detect -fsanitize=cfi jump table sections */
+		if (!strncmp(sec->name, ".text..L.cfi.jumptable", 22))
+			sec->cfi_jt = true;
+
 		list_add_tail(&sec->list, &elf->sections);
 		elf_hash_add(section, &sec->hash, sec->idx);
 		elf_hash_add(section_name, &sec->name_hash, str_hash(sec->name));
@@ -576,6 +581,49 @@ static int read_rela_reloc(struct section *sec, int i, struct reloc *reloc, unsi
 	return 0;
 }
 
+/*
+ * CONFIG_CFI_CLANG replaces function relocations to refer to an intermediate
+ * jump table. Undo the conversion so objtool can make sense of things.
+ */
+static int fix_cfi_relocs(const struct elf *elf)
+{
+	struct section *sec;
+	struct reloc *reloc;
+
+	list_for_each_entry(sec, &elf->sections, list) {
+		list_for_each_entry(reloc, &sec->reloc_list, list) {
+			struct reloc *cfi_reloc;
+			unsigned long offset;
+
+			if (!reloc->sym->sec->cfi_jt)
+				continue;
+
+			if (reloc->sym->type == STT_SECTION)
+				offset = arch_cfi_section_reloc_offset(reloc);
+			else
+				offset = reloc->sym->offset;
+
+			/*
+			 * The jump table immediately jumps to the actual function,
+			 * so look up the relocation there.
+			 */
+			offset = arch_cfi_jump_reloc_offset(offset);
+			cfi_reloc = find_reloc_by_dest(elf, reloc->sym->sec, offset);
+
+			if (!cfi_reloc || !cfi_reloc->sym) {
+				WARN("can't find a CFI jump table relocation at %s+0x%lx",
+					reloc->sym->sec->name, offset);
+				return -1;
+			}
+
+			reloc->sym = cfi_reloc->sym;
+			reloc->addend = 0;
+		}
+	}
+
+	return 0;
+}
+
 static int read_relocs(struct elf *elf)
 {
 	struct section *sec;
@@ -639,6 +687,9 @@ static int read_relocs(struct elf *elf)
 		tot_reloc += nr_reloc;
 	}
 
+	if (fix_cfi_relocs(elf))
+		return -1;
+
 	if (stats) {
 		printf("max_reloc: %lu\n", max_reloc);
 		printf("tot_reloc: %lu\n", tot_reloc);
diff --git a/tools/objtool/include/objtool/arch.h b/tools/objtool/include/objtool/arch.h
index 589ff58426ab..93bde8aaf2e3 100644
--- a/tools/objtool/include/objtool/arch.h
+++ b/tools/objtool/include/objtool/arch.h
@@ -81,6 +81,9 @@ unsigned long arch_jump_destination(struct instruction *insn);
 
 unsigned long arch_dest_reloc_offset(int addend);
 
+unsigned long arch_cfi_section_reloc_offset(struct reloc *reloc);
+unsigned long arch_cfi_jump_reloc_offset(unsigned long offset);
+
 const char *arch_nop_insn(int len);
 const char *arch_ret_insn(int len);
 
diff --git a/tools/objtool/include/objtool/elf.h b/tools/objtool/include/objtool/elf.h
index c3857fadee7a..e8d838217c77 100644
--- a/tools/objtool/include/objtool/elf.h
+++ b/tools/objtool/include/objtool/elf.h
@@ -39,7 +39,7 @@ struct section {
 	char *name;
 	int idx;
 	unsigned int len;
-	bool changed, text, rodata, noinstr;
+	bool changed, text, rodata, noinstr, cfi_jt;
 };
 
 struct symbol {
-- 
2.33.0.800.g4c38ced690-goog

Re: [PATCH v4 01/15] objtool: Add CONFIG_CFI_CLANG support

[Nick Desaulniers]

On Thu, Sep 30, 2021 at 11:05 AM Sami Tolvanen <samitolvanen@google.com> wrote:
>
> With CONFIG_CFI_CLANG, the compiler replaces function references with
> references to the CFI jump table, which confuses objtool. This change,
> based on Josh's initial patch [1], goes through the list of relocations
> and replaces jump table symbols with the actual function symbols.
>
> [1] https://lore.kernel.org/r/d743f4b36e120c06506567a9f87a062ae03da47f.1611263462.git.jpoimboe@redhat.com/
>
> Reported-by: Sedat Dilek <sedat.dilek@gmail.com>
> Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> ---
>  tools/objtool/arch/x86/decode.c      | 17 ++++++++++
>  tools/objtool/elf.c                  | 51 ++++++++++++++++++++++++++++
>  tools/objtool/include/objtool/arch.h |  3 ++
>  tools/objtool/include/objtool/elf.h  |  2 +-
>  4 files changed, 72 insertions(+), 1 deletion(-)
>
> diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
> index 3172983bf808..9b043220b0af 100644
> --- a/tools/objtool/arch/x86/decode.c
> +++ b/tools/objtool/arch/x86/decode.c
> @@ -63,6 +63,23 @@ bool arch_callee_saved_reg(unsigned char reg)
>         }
>  }
>
> +unsigned long arch_cfi_section_reloc_offset(struct reloc *reloc)
> +{
> +       if (!reloc->addend)
> +               return 0;
> +
> +       if (reloc->type == R_X86_64_PC32 || reloc->type == R_X86_64_PLT32)
> +               return reloc->addend + 4;
> +
> +       return reloc->addend;
> +}
> +
> +unsigned long arch_cfi_jump_reloc_offset(unsigned long offset)
> +{
> +       /* offset to the relocation in a jmp instruction */

Thanks for the added comment. ;)

Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

> +       return offset + 1;
> +}
> +
>  unsigned long arch_dest_reloc_offset(int addend)
>  {
>         return addend + 4;
> diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
> index 8676c7598728..05a5f51aad2c 100644
> --- a/tools/objtool/elf.c
> +++ b/tools/objtool/elf.c
> @@ -18,6 +18,7 @@
>  #include <errno.h>
>  #include <objtool/builtin.h>
>
> +#include <objtool/arch.h>
>  #include <objtool/elf.h>
>  #include <objtool/warn.h>
>
> @@ -291,6 +292,10 @@ static int read_sections(struct elf *elf)
>                 if (sec->sh.sh_flags & SHF_EXECINSTR)
>                         elf->text_size += sec->len;
>
> +               /* Detect -fsanitize=cfi jump table sections */
> +               if (!strncmp(sec->name, ".text..L.cfi.jumptable", 22))
> +                       sec->cfi_jt = true;
> +
>                 list_add_tail(&sec->list, &elf->sections);
>                 elf_hash_add(section, &sec->hash, sec->idx);
>                 elf_hash_add(section_name, &sec->name_hash, str_hash(sec->name));
> @@ -576,6 +581,49 @@ static int read_rela_reloc(struct section *sec, int i, struct reloc *reloc, unsi
>         return 0;
>  }
>
> +/*
> + * CONFIG_CFI_CLANG replaces function relocations to refer to an intermediate
> + * jump table. Undo the conversion so objtool can make sense of things.
> + */
> +static int fix_cfi_relocs(const struct elf *elf)
> +{
> +       struct section *sec;
> +       struct reloc *reloc;
> +
> +       list_for_each_entry(sec, &elf->sections, list) {
> +               list_for_each_entry(reloc, &sec->reloc_list, list) {
> +                       struct reloc *cfi_reloc;
> +                       unsigned long offset;
> +
> +                       if (!reloc->sym->sec->cfi_jt)
> +                               continue;
> +
> +                       if (reloc->sym->type == STT_SECTION)
> +                               offset = arch_cfi_section_reloc_offset(reloc);
> +                       else
> +                               offset = reloc->sym->offset;
> +
> +                       /*
> +                        * The jump table immediately jumps to the actual function,
> +                        * so look up the relocation there.
> +                        */
> +                       offset = arch_cfi_jump_reloc_offset(offset);
> +                       cfi_reloc = find_reloc_by_dest(elf, reloc->sym->sec, offset);
> +
> +                       if (!cfi_reloc || !cfi_reloc->sym) {
> +                               WARN("can't find a CFI jump table relocation at %s+0x%lx",
> +                                       reloc->sym->sec->name, offset);
> +                               return -1;
> +                       }
> +
> +                       reloc->sym = cfi_reloc->sym;
> +                       reloc->addend = 0;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
>  static int read_relocs(struct elf *elf)
>  {
>         struct section *sec;
> @@ -639,6 +687,9 @@ static int read_relocs(struct elf *elf)
>                 tot_reloc += nr_reloc;
>         }
>
> +       if (fix_cfi_relocs(elf))
> +               return -1;
> +
>         if (stats) {
>                 printf("max_reloc: %lu\n", max_reloc);
>                 printf("tot_reloc: %lu\n", tot_reloc);
> diff --git a/tools/objtool/include/objtool/arch.h b/tools/objtool/include/objtool/arch.h
> index 589ff58426ab..93bde8aaf2e3 100644
> --- a/tools/objtool/include/objtool/arch.h
> +++ b/tools/objtool/include/objtool/arch.h
> @@ -81,6 +81,9 @@ unsigned long arch_jump_destination(struct instruction *insn);
>
>  unsigned long arch_dest_reloc_offset(int addend);
>
> +unsigned long arch_cfi_section_reloc_offset(struct reloc *reloc);
> +unsigned long arch_cfi_jump_reloc_offset(unsigned long offset);
> +
>  const char *arch_nop_insn(int len);
>  const char *arch_ret_insn(int len);
>
> diff --git a/tools/objtool/include/objtool/elf.h b/tools/objtool/include/objtool/elf.h
> index c3857fadee7a..e8d838217c77 100644
> --- a/tools/objtool/include/objtool/elf.h
> +++ b/tools/objtool/include/objtool/elf.h
> @@ -39,7 +39,7 @@ struct section {
>         char *name;
>         int idx;
>         unsigned int len;
> -       bool changed, text, rodata, noinstr;
> +       bool changed, text, rodata, noinstr, cfi_jt;
>  };
>
>  struct symbol {
> --
> 2.33.0.800.g4c38ced690-goog
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v4 01/15] objtool: Add CONFIG_CFI_CLANG support

[Josh Poimboeuf]

On Thu, Sep 30, 2021 at 11:05:17AM -0700, Sami Tolvanen wrote:
> With CONFIG_CFI_CLANG, the compiler replaces function references with
> references to the CFI jump table, which confuses objtool. This change,
> based on Josh's initial patch [1], goes through the list of relocations
> and replaces jump table symbols with the actual function symbols.
> 
> [1] https://lore.kernel.org/r/d743f4b36e120c06506567a9f87a062ae03da47f.1611263462.git.jpoimboe@redhat.com/

I found the original patch description to be much more useful than this
one ;-)

-- 
Josh

Re: [PATCH v4 01/15] objtool: Add CONFIG_CFI_CLANG support

[Sami Tolvanen]

On Tue, Oct 5, 2021 at 8:36 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Thu, Sep 30, 2021 at 11:05:17AM -0700, Sami Tolvanen wrote:
> > With CONFIG_CFI_CLANG, the compiler replaces function references with
> > references to the CFI jump table, which confuses objtool. This change,
> > based on Josh's initial patch [1], goes through the list of relocations
> > and replaces jump table symbols with the actual function symbols.
> >
> > [1] https://lore.kernel.org/r/d743f4b36e120c06506567a9f87a062ae03da47f.1611263462.git.jpoimboe@redhat.com/
>
> I found the original patch description to be much more useful than this
> one ;-)

Sure, I'll just copy that for the next version.

Sami

[PATCH v4 02/15] objtool: Add ASM_STACK_FRAME_NON_STANDARD

[Sami Tolvanen]

To use the STACK_FRAME_NON_STANDARD macro for a static symbol
defined in inline assembly, we need a C declaration that implies
global visibility. This type mismatch confuses the compiler with
CONFIG_CFI_CLANG. This change adds an inline assembly version of
the macro to avoid the issue.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
 include/linux/objtool.h       | 6 ++++++
 tools/include/linux/objtool.h | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/include/linux/objtool.h b/include/linux/objtool.h
index 7e72d975cb76..080e95174536 100644
--- a/include/linux/objtool.h
+++ b/include/linux/objtool.h
@@ -66,6 +66,11 @@ struct unwind_hint {
 	static void __used __section(".discard.func_stack_frame_non_standard") \
 		*__func_stack_frame_non_standard_##func = func
 
+#define ASM_STACK_FRAME_NON_STANDARD(func)				\
+	".pushsection .discard.func_stack_frame_non_standard, \"aw\"\n"	\
+	".long " __stringify(func) " - .\n"				\
+	".popsection\n"
+
 #else /* __ASSEMBLY__ */
 
 /*
@@ -127,6 +132,7 @@ struct unwind_hint {
 #define UNWIND_HINT(sp_reg, sp_offset, type, end)	\
 	"\n\t"
 #define STACK_FRAME_NON_STANDARD(func)
+#define ASM_STACK_FRAME_NON_STANDARD(func)
 #else
 #define ANNOTATE_INTRA_FUNCTION_CALL
 .macro UNWIND_HINT sp_reg:req sp_offset=0 type:req end=0
diff --git a/tools/include/linux/objtool.h b/tools/include/linux/objtool.h
index 7e72d975cb76..080e95174536 100644
--- a/tools/include/linux/objtool.h
+++ b/tools/include/linux/objtool.h
@@ -66,6 +66,11 @@ struct unwind_hint {
 	static void __used __section(".discard.func_stack_frame_non_standard") \
 		*__func_stack_frame_non_standard_##func = func
 
+#define ASM_STACK_FRAME_NON_STANDARD(func)				\
+	".pushsection .discard.func_stack_frame_non_standard, \"aw\"\n"	\
+	".long " __stringify(func) " - .\n"				\
+	".popsection\n"
+
 #else /* __ASSEMBLY__ */
 
 /*
@@ -127,6 +132,7 @@ struct unwind_hint {
 #define UNWIND_HINT(sp_reg, sp_offset, type, end)	\
 	"\n\t"
 #define STACK_FRAME_NON_STANDARD(func)
+#define ASM_STACK_FRAME_NON_STANDARD(func)
 #else
 #define ANNOTATE_INTRA_FUNCTION_CALL
 .macro UNWIND_HINT sp_reg:req sp_offset=0 type:req end=0
-- 
2.33.0.800.g4c38ced690-goog

Re: [PATCH v4 02/15] objtool: Add ASM_STACK_FRAME_NON_STANDARD

[Josh Poimboeuf]

On Thu, Sep 30, 2021 at 11:05:18AM -0700, Sami Tolvanen wrote:
> To use the STACK_FRAME_NON_STANDARD macro for a static symbol
> defined in inline assembly, we need a C declaration that implies
> global visibility. This type mismatch confuses the compiler with
> CONFIG_CFI_CLANG. This change adds an inline assembly version of
> the macro to avoid the issue.
> 
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>

Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>

-- 
Josh

[PATCH v4 03/15] linkage: Add DECLARE_ASM_FUNC_SYMBOL

[Sami Tolvanen]

The kernel has several assembly functions, which are not directly
callable from C but need to be referred to from C code. This change adds
the DECLARE_ASM_FUNC_SYMBOL macro, which allows us to declare these
symbols using an opaque type, which makes misuse harder, and avoids the
need to annotate references to the functions for Clang's Control-Flow
Integrity (CFI).

Suggested-by: Andy Lutomirski <luto@amacapital.net>
Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
 include/linux/linkage.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/linux/linkage.h b/include/linux/linkage.h
index dbf8506decca..f1eac26b2dd6 100644
--- a/include/linux/linkage.h
+++ b/include/linux/linkage.h
@@ -48,6 +48,19 @@
 #define __PAGE_ALIGNED_DATA	.section ".data..page_aligned", "aw"
 #define __PAGE_ALIGNED_BSS	.section ".bss..page_aligned", "aw"
 
+/*
+ * Declares a function not callable from C using an opaque type. Defined as
+ * an array to allow the address of the symbol to be taken without '&'.
+ */
+#ifndef DECLARE_ASM_FUNC_SYMBOL
+#define DECLARE_ASM_FUNC_SYMBOL(sym) \
+	extern const u8 sym[]
+#endif
+
+#ifndef __ASSEMBLY__
+typedef const u8 *asm_func_ptr;
+#endif
+
 /*
  * This is used by architectures to keep arguments on the stack
  * untouched by the compiler by keeping them live until the end.
-- 
2.33.0.800.g4c38ced690-goog

[PATCH v4 04/15] cfi: Add DEFINE_CFI_IMMEDIATE_RETURN_STUB

[Sami Tolvanen]

This change introduces the DEFINE_CFI_IMMEDIATE_RETURN_STUB macro,
which defines a stub function that immediately returns and when
defined in the core kernel, always passes indirect call checking
with CONFIG_CFI_CLANG. Note that this macro should only be used when
a stub cannot be called using the correct function type.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
 include/asm-generic/vmlinux.lds.h | 11 +++++++++++
 include/linux/cfi.h               | 13 +++++++++++++
 kernel/cfi.c                      | 24 +++++++++++++++++++++++-
 3 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index f2984af2b85b..5b77284f7221 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -407,6 +407,16 @@
 	KEEP(*(.static_call_tramp_key))					\
 	__stop_static_call_tramp_key = .;
 
+#ifdef CONFIG_CFI_CLANG
+#define CFI_EXCLUDED_DATA						\
+	. = ALIGN(8);							\
+	__start_cfi_excluded = .;					\
+	KEEP(*(.cfi_excluded_stubs))					\
+	__stop_cfi_excluded = .;
+#else
+#define CFI_EXCLUDED_DATA
+#endif
+
 /*
  * Allow architectures to handle ro_after_init data on their
  * own by defining an empty RO_AFTER_INIT_DATA.
@@ -430,6 +440,7 @@
 		__start_rodata = .;					\
 		*(.rodata) *(.rodata.*)					\
 		SCHED_DATA						\
+		CFI_EXCLUDED_DATA					\
 		RO_AFTER_INIT_DATA	/* Read only after init */	\
 		. = ALIGN(8);						\
 		__start___tracepoints_ptrs = .;				\
diff --git a/include/linux/cfi.h b/include/linux/cfi.h
index 879744aaa6e0..19f74af8eac2 100644
--- a/include/linux/cfi.h
+++ b/include/linux/cfi.h
@@ -20,6 +20,17 @@ extern void __cfi_check(uint64_t id, void *ptr, void *diag);
 #define __CFI_ADDRESSABLE(fn, __attr) \
 	const void *__cfi_jt_ ## fn __visible __attr = (void *)&fn
 
+/*
+ * Defines a stub function that returns immediately, and when defined and
+ * referenced in the core kernel, always passes CFI checking. This should
+ * be used only for stubs that cannot be called using the correct function
+ * pointer type, which should be rare.
+ */
+#define DEFINE_CFI_IMMEDIATE_RETURN_STUB(fn) \
+	void fn(void) { return; } \
+	const void *__cfi_excl_ ## fn __visible \
+		__section(".cfi_excluded_stubs") = (void *)&fn
+
 #ifdef CONFIG_CFI_CLANG_SHADOW
 
 extern void cfi_module_add(struct module *mod, unsigned long base_addr);
@@ -35,6 +46,8 @@ static inline void cfi_module_remove(struct module *mod, unsigned long base_addr
 #else /* !CONFIG_CFI_CLANG */
 
 #define __CFI_ADDRESSABLE(fn, __attr)
+#define DEFINE_CFI_IMMEDIATE_RETURN_STUB(fn) \
+	void fn(void) { return; }
 
 #endif /* CONFIG_CFI_CLANG */
 
diff --git a/kernel/cfi.c b/kernel/cfi.c
index 9594cfd1cf2c..8d931089141b 100644
--- a/kernel/cfi.c
+++ b/kernel/cfi.c
@@ -278,12 +278,34 @@ static inline cfi_check_fn find_module_check_fn(unsigned long ptr)
 	return fn;
 }
 
+extern unsigned long __start_cfi_excluded[];
+extern unsigned long __stop_cfi_excluded[];
+
+static inline bool is_cfi_excluded(unsigned long ptr)
+{
+	unsigned long *p = __start_cfi_excluded;
+
+	for ( ; p < __stop_cfi_excluded; ++p)
+		if (*p == ptr)
+			return true;
+
+	return false;
+}
+
+static void __cfi_pass(uint64_t id, void *ptr, void *diag)
+{
+}
+
 static inline cfi_check_fn find_check_fn(unsigned long ptr)
 {
 	cfi_check_fn fn = NULL;
 
-	if (is_kernel_text(ptr))
+	if (is_kernel_text(ptr)) {
+		if (unlikely(is_cfi_excluded(ptr)))
+			return __cfi_pass;
+
 		return __cfi_check;
+	}
 
 	/*
 	 * Indirect call checks can happen when RCU is not watching. Both
-- 
2.33.0.800.g4c38ced690-goog

Re: [PATCH v4 04/15] cfi: Add DEFINE_CFI_IMMEDIATE_RETURN_STUB

[Nick Desaulniers]

On Thu, Sep 30, 2021 at 11:05 AM Sami Tolvanen <samitolvanen@google.com> wrote:
>
> This change introduces the DEFINE_CFI_IMMEDIATE_RETURN_STUB macro,
> which defines a stub function that immediately returns and when
> defined in the core kernel, always passes indirect call checking
> with CONFIG_CFI_CLANG. Note that this macro should only be used when
> a stub cannot be called using the correct function type.

Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

Seems like the only use is in patch 5/15. Probably could be squashed...

>
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> ---
>  include/asm-generic/vmlinux.lds.h | 11 +++++++++++
>  include/linux/cfi.h               | 13 +++++++++++++
>  kernel/cfi.c                      | 24 +++++++++++++++++++++++-
>  3 files changed, 47 insertions(+), 1 deletion(-)
>
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index f2984af2b85b..5b77284f7221 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -407,6 +407,16 @@
>         KEEP(*(.static_call_tramp_key))                                 \
>         __stop_static_call_tramp_key = .;
>
> +#ifdef CONFIG_CFI_CLANG
> +#define CFI_EXCLUDED_DATA                                              \
> +       . = ALIGN(8);                                                   \
> +       __start_cfi_excluded = .;                                       \
> +       KEEP(*(.cfi_excluded_stubs))                                    \
> +       __stop_cfi_excluded = .;
> +#else
> +#define CFI_EXCLUDED_DATA
> +#endif
> +
>  /*
>   * Allow architectures to handle ro_after_init data on their
>   * own by defining an empty RO_AFTER_INIT_DATA.
> @@ -430,6 +440,7 @@
>                 __start_rodata = .;                                     \
>                 *(.rodata) *(.rodata.*)                                 \
>                 SCHED_DATA                                              \
> +               CFI_EXCLUDED_DATA                                       \
>                 RO_AFTER_INIT_DATA      /* Read only after init */      \
>                 . = ALIGN(8);                                           \
>                 __start___tracepoints_ptrs = .;                         \
> diff --git a/include/linux/cfi.h b/include/linux/cfi.h
> index 879744aaa6e0..19f74af8eac2 100644
> --- a/include/linux/cfi.h
> +++ b/include/linux/cfi.h
> @@ -20,6 +20,17 @@ extern void __cfi_check(uint64_t id, void *ptr, void *diag);
>  #define __CFI_ADDRESSABLE(fn, __attr) \
>         const void *__cfi_jt_ ## fn __visible __attr = (void *)&fn
>
> +/*
> + * Defines a stub function that returns immediately, and when defined and
> + * referenced in the core kernel, always passes CFI checking. This should
> + * be used only for stubs that cannot be called using the correct function
> + * pointer type, which should be rare.
> + */
> +#define DEFINE_CFI_IMMEDIATE_RETURN_STUB(fn) \
> +       void fn(void) { return; } \
> +       const void *__cfi_excl_ ## fn __visible \
> +               __section(".cfi_excluded_stubs") = (void *)&fn
> +
>  #ifdef CONFIG_CFI_CLANG_SHADOW
>
>  extern void cfi_module_add(struct module *mod, unsigned long base_addr);
> @@ -35,6 +46,8 @@ static inline void cfi_module_remove(struct module *mod, unsigned long base_addr
>  #else /* !CONFIG_CFI_CLANG */
>
>  #define __CFI_ADDRESSABLE(fn, __attr)
> +#define DEFINE_CFI_IMMEDIATE_RETURN_STUB(fn) \
> +       void fn(void) { return; }
>
>  #endif /* CONFIG_CFI_CLANG */
>
> diff --git a/kernel/cfi.c b/kernel/cfi.c
> index 9594cfd1cf2c..8d931089141b 100644
> --- a/kernel/cfi.c
> +++ b/kernel/cfi.c
> @@ -278,12 +278,34 @@ static inline cfi_check_fn find_module_check_fn(unsigned long ptr)
>         return fn;
>  }
>
> +extern unsigned long __start_cfi_excluded[];
> +extern unsigned long __stop_cfi_excluded[];
> +
> +static inline bool is_cfi_excluded(unsigned long ptr)
> +{
> +       unsigned long *p = __start_cfi_excluded;
> +
> +       for ( ; p < __stop_cfi_excluded; ++p)
> +               if (*p == ptr)
> +                       return true;
> +
> +       return false;
> +}
> +
> +static void __cfi_pass(uint64_t id, void *ptr, void *diag)
> +{
> +}
> +
>  static inline cfi_check_fn find_check_fn(unsigned long ptr)
>  {
>         cfi_check_fn fn = NULL;
>
> -       if (is_kernel_text(ptr))
> +       if (is_kernel_text(ptr)) {
> +               if (unlikely(is_cfi_excluded(ptr)))
> +                       return __cfi_pass;
> +
>                 return __cfi_check;
> +       }
>
>         /*
>          * Indirect call checks can happen when RCU is not watching. Both
> --
> 2.33.0.800.g4c38ced690-goog
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v4 04/15] cfi: Add DEFINE_CFI_IMMEDIATE_RETURN_STUB

[Sami Tolvanen]

On Thu, Sep 30, 2021 at 11:50 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Thu, Sep 30, 2021 at 11:05 AM Sami Tolvanen <samitolvanen@google.com> wrote:
> >
> > This change introduces the DEFINE_CFI_IMMEDIATE_RETURN_STUB macro,
> > which defines a stub function that immediately returns and when
> > defined in the core kernel, always passes indirect call checking
> > with CONFIG_CFI_CLANG. Note that this macro should only be used when
> > a stub cannot be called using the correct function type.
>
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
>
> Seems like the only use is in patch 5/15. Probably could be squashed...

I would prefer to keep these separate, but if everyone thinks that's
unnecessary, I'm happy to combine them.

Sami

Re: [PATCH v4 04/15] cfi: Add DEFINE_CFI_IMMEDIATE_RETURN_STUB

[Peter Zijlstra]

On Thu, Sep 30, 2021 at 11:05:20AM -0700, Sami Tolvanen wrote:
> This change introduces the DEFINE_CFI_IMMEDIATE_RETURN_STUB macro,
> which defines a stub function that immediately returns and when
> defined in the core kernel, always passes indirect call checking
> with CONFIG_CFI_CLANG. Note that this macro should only be used when
> a stub cannot be called using the correct function type.

> diff --git a/include/linux/cfi.h b/include/linux/cfi.h
> index 879744aaa6e0..19f74af8eac2 100644
> --- a/include/linux/cfi.h
> +++ b/include/linux/cfi.h
> @@ -20,6 +20,17 @@ extern void __cfi_check(uint64_t id, void *ptr, void *diag);
>  #define __CFI_ADDRESSABLE(fn, __attr) \
>  	const void *__cfi_jt_ ## fn __visible __attr = (void *)&fn
>  
> +/*
> + * Defines a stub function that returns immediately, and when defined and
> + * referenced in the core kernel, always passes CFI checking. This should
> + * be used only for stubs that cannot be called using the correct function
> + * pointer type, which should be rare.
> + */
> +#define DEFINE_CFI_IMMEDIATE_RETURN_STUB(fn) \
> +	void fn(void) { return; } \
> +	const void *__cfi_excl_ ## fn __visible \
> +		__section(".cfi_excluded_stubs") = (void *)&fn
> +
>  #ifdef CONFIG_CFI_CLANG_SHADOW
>  
>  extern void cfi_module_add(struct module *mod, unsigned long base_addr);
> @@ -35,6 +46,8 @@ static inline void cfi_module_remove(struct module *mod, unsigned long base_addr
>  #else /* !CONFIG_CFI_CLANG */
>  
>  #define __CFI_ADDRESSABLE(fn, __attr)
> +#define DEFINE_CFI_IMMEDIATE_RETURN_STUB(fn) \
> +	void fn(void) { return; }
>  
>  #endif /* CONFIG_CFI_CLANG */
>  

Why DEFINE_CFI_IMMEDIATE_RETURN_STUB() vs __no_cfi attribute that we can
stick on the relvant functions?

Because I've got at least one more variant for you :-) See
kernel/static_call.c:__static_call_return0

Re: [PATCH v4 04/15] cfi: Add DEFINE_CFI_IMMEDIATE_RETURN_STUB

[Sami Tolvanen]

On Mon, Oct 4, 2021 at 6:50 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Sep 30, 2021 at 11:05:20AM -0700, Sami Tolvanen wrote:
> > This change introduces the DEFINE_CFI_IMMEDIATE_RETURN_STUB macro,
> > which defines a stub function that immediately returns and when
> > defined in the core kernel, always passes indirect call checking
> > with CONFIG_CFI_CLANG. Note that this macro should only be used when
> > a stub cannot be called using the correct function type.
>
> > diff --git a/include/linux/cfi.h b/include/linux/cfi.h
> > index 879744aaa6e0..19f74af8eac2 100644
> > --- a/include/linux/cfi.h
> > +++ b/include/linux/cfi.h
> > @@ -20,6 +20,17 @@ extern void __cfi_check(uint64_t id, void *ptr, void *diag);
> >  #define __CFI_ADDRESSABLE(fn, __attr) \
> >       const void *__cfi_jt_ ## fn __visible __attr = (void *)&fn
> >
> > +/*
> > + * Defines a stub function that returns immediately, and when defined and
> > + * referenced in the core kernel, always passes CFI checking. This should
> > + * be used only for stubs that cannot be called using the correct function
> > + * pointer type, which should be rare.
> > + */
> > +#define DEFINE_CFI_IMMEDIATE_RETURN_STUB(fn) \
> > +     void fn(void) { return; } \
> > +     const void *__cfi_excl_ ## fn __visible \
> > +             __section(".cfi_excluded_stubs") = (void *)&fn
> > +
> >  #ifdef CONFIG_CFI_CLANG_SHADOW
> >
> >  extern void cfi_module_add(struct module *mod, unsigned long base_addr);
> > @@ -35,6 +46,8 @@ static inline void cfi_module_remove(struct module *mod, unsigned long base_addr
> >  #else /* !CONFIG_CFI_CLANG */
> >
> >  #define __CFI_ADDRESSABLE(fn, __attr)
> > +#define DEFINE_CFI_IMMEDIATE_RETURN_STUB(fn) \
> > +     void fn(void) { return; }
> >
> >  #endif /* CONFIG_CFI_CLANG */
> >
>
> Why DEFINE_CFI_IMMEDIATE_RETURN_STUB() vs __no_cfi attribute that we can
> stick on the relvant functions?

To avoid accidentally creating useful gadgets for attackers. For
example, while excluding an empty stub isn't necessarily ideal,
allowing calls to a function that always returns zero would be worse.

> Because I've got at least one more variant for you :-) See
> kernel/static_call.c:__static_call_return0

Does __static_call_return0 ever get called indirectly on architectures
that support static calls? If it's always patched into a direct call,
the type mismatch isn't an issue.

Sami

Re: [PATCH v4 04/15] cfi: Add DEFINE_CFI_IMMEDIATE_RETURN_STUB

[Peter Zijlstra]

On Mon, Oct 04, 2021 at 12:10:46PM -0700, Sami Tolvanen wrote:
> On Mon, Oct 4, 2021 at 6:50 AM Peter Zijlstra <peterz@infradead.org> wrote:

> > Why DEFINE_CFI_IMMEDIATE_RETURN_STUB() vs __no_cfi attribute that we can
> > stick on the relvant functions?
> 
> To avoid accidentally creating useful gadgets for attackers. For
> example, while excluding an empty stub isn't necessarily ideal,
> allowing calls to a function that always returns zero would be worse.

I was afraid you'd say something like that...

> > Because I've got at least one more variant for you :-) See
> > kernel/static_call.c:__static_call_return0
> 
> Does __static_call_return0 ever get called indirectly on architectures
> that support static calls? If it's always patched into a direct call,
> the type mismatch isn't an issue.

For x86_64 it should indeed never get called, however if you plan on
supporting i386 then you need the annotation. Also, it might get called
on arm64 which is about to grow basic HAVE_STATIC_CALL support.

(and just in case you care about CFI on PPC32, they too grew basic
static_call support)

Re: [PATCH v4 04/15] cfi: Add DEFINE_CFI_IMMEDIATE_RETURN_STUB

[Sami Tolvanen]

On Mon, Oct 4, 2021 at 11:59 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Mon, Oct 04, 2021 at 12:10:46PM -0700, Sami Tolvanen wrote:
> > On Mon, Oct 4, 2021 at 6:50 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> > > Why DEFINE_CFI_IMMEDIATE_RETURN_STUB() vs __no_cfi attribute that we can
> > > stick on the relvant functions?
> >
> > To avoid accidentally creating useful gadgets for attackers. For
> > example, while excluding an empty stub isn't necessarily ideal,
> > allowing calls to a function that always returns zero would be worse.
>
> I was afraid you'd say something like that...
>
> > > Because I've got at least one more variant for you :-) See
> > > kernel/static_call.c:__static_call_return0
> >
> > Does __static_call_return0 ever get called indirectly on architectures
> > that support static calls? If it's always patched into a direct call,
> > the type mismatch isn't an issue.
>
> For x86_64 it should indeed never get called, however if you plan on
> supporting i386 then you need the annotation. Also, it might get called
> on arm64 which is about to grow basic HAVE_STATIC_CALL support.

Good point. I read through the latest arm64 static call proposal and
while it can fall back to an indirect call, it doesn't look like that
would cause issues with CFI.

> (and just in case you care about CFI on PPC32, they too grew basic
> static_call support)

We are currently targeting only x86_64 and arm64, but I'll keep that
in mind in case we want to add more platforms.

Sami

Re: [PATCH v4 04/15] cfi: Add DEFINE_CFI_IMMEDIATE_RETURN_STUB

[Peter Zijlstra]

On Tue, Oct 05, 2021 at 01:29:02PM -0700, Sami Tolvanen wrote:
> On Mon, Oct 4, 2021 at 11:59 PM Peter Zijlstra <peterz@infradead.org> wrote:

> > For x86_64 it should indeed never get called, however if you plan on
> > supporting i386 then you need the annotation. Also, it might get called
> > on arm64 which is about to grow basic HAVE_STATIC_CALL support.
> 
> Good point. I read through the latest arm64 static call proposal and
> while it can fall back to an indirect call, it doesn't look like that
> would cause issues with CFI.

Because that call is outside of compiler control? Same will be true for
any HAVE_STATIC_CALL implementation I suppose. The trampoline will be
outside of compiler control.

Re: [PATCH v4 04/15] cfi: Add DEFINE_CFI_IMMEDIATE_RETURN_STUB

[Sami Tolvanen]

On Tue, Oct 5, 2021 at 1:58 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Tue, Oct 05, 2021 at 01:29:02PM -0700, Sami Tolvanen wrote:
> > On Mon, Oct 4, 2021 at 11:59 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> > > For x86_64 it should indeed never get called, however if you plan on
> > > supporting i386 then you need the annotation. Also, it might get called
> > > on arm64 which is about to grow basic HAVE_STATIC_CALL support.
> >
> > Good point. I read through the latest arm64 static call proposal and
> > while it can fall back to an indirect call, it doesn't look like that
> > would cause issues with CFI.
>
> Because that call is outside of compiler control?

Correct.

> Same will be true for
> any HAVE_STATIC_CALL implementation I suppose. The trampoline will be
> outside of compiler control.

True, so it shouldn't be a problem.

Sami

[PATCH v4 05/15] tracepoint: Exclude tp_stub_func from CFI checking

[Sami Tolvanen]

If allocate_probes fails, func_remove replaces the old function
with a pointer to tp_stub_func, which is called using a mismatching
function pointer that will always trip indirect call checks with
CONFIG_CFI_CLANG. Use DEFINE_CFI_IMMEDATE_RETURN_STUB to define
tp_stub_func to allow it to pass CFI checking.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
---
 kernel/tracepoint.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
index 64ea283f2f86..58acc7d86c3f 100644
--- a/kernel/tracepoint.c
+++ b/kernel/tracepoint.c
@@ -99,10 +99,7 @@ struct tp_probes {
 };
 
 /* Called in removal of a func but failed to allocate a new tp_funcs */
-static void tp_stub_func(void)
-{
-	return;
-}
+static DEFINE_CFI_IMMEDIATE_RETURN_STUB(tp_stub_func);
 
 static inline void *allocate_probes(int count)
 {
-- 
2.33.0.800.g4c38ced690-goog

Re: [PATCH v4 05/15] tracepoint: Exclude tp_stub_func from CFI checking

[Nick Desaulniers]

On Thu, Sep 30, 2021 at 11:05 AM Sami Tolvanen <samitolvanen@google.com> wrote:
>
> If allocate_probes fails, func_remove replaces the old function
> with a pointer to tp_stub_func, which is called using a mismatching
> function pointer that will always trip indirect call checks with
> CONFIG_CFI_CLANG. Use DEFINE_CFI_IMMEDATE_RETURN_STUB to define
> tp_stub_func to allow it to pass CFI checking.
>
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
>  kernel/tracepoint.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> index 64ea283f2f86..58acc7d86c3f 100644
> --- a/kernel/tracepoint.c
> +++ b/kernel/tracepoint.c

looking at 4+5/15 together, I wonder if this TU should explicitly
include linux/cfi.h?

> @@ -99,10 +99,7 @@ struct tp_probes {
>  };
>
>  /* Called in removal of a func but failed to allocate a new tp_funcs */
> -static void tp_stub_func(void)
> -{
> -       return;
> -}
> +static DEFINE_CFI_IMMEDIATE_RETURN_STUB(tp_stub_func);
>
>  static inline void *allocate_probes(int count)
>  {
> --
> 2.33.0.800.g4c38ced690-goog
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v4 05/15] tracepoint: Exclude tp_stub_func from CFI checking

[Sami Tolvanen]

On Thu, Sep 30, 2021 at 11:51 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Thu, Sep 30, 2021 at 11:05 AM Sami Tolvanen <samitolvanen@google.com> wrote:
> >
> > If allocate_probes fails, func_remove replaces the old function
> > with a pointer to tp_stub_func, which is called using a mismatching
> > function pointer that will always trip indirect call checks with
> > CONFIG_CFI_CLANG. Use DEFINE_CFI_IMMEDATE_RETURN_STUB to define
> > tp_stub_func to allow it to pass CFI checking.
> >
> > Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> > ---
> >  kernel/tracepoint.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> > index 64ea283f2f86..58acc7d86c3f 100644
> > --- a/kernel/tracepoint.c
> > +++ b/kernel/tracepoint.c
>
> looking at 4+5/15 together, I wonder if this TU should explicitly
> include linux/cfi.h?

Good point. Currently cfi.h is included in module.h, but including it
explicitly makes this less likely to break in future. I'll add an
explicit include in v5. Thanks for taking a look!

Sami

[PATCH v4 06/15] ftrace: Use an opaque type for functions not callable from C

[Sami Tolvanen]

With CONFIG_CFI_CLANG, the compiler changes function references to point
to the CFI jump table. As ftrace_call, ftrace_regs_call, and mcount_call
are not called from C, use DECLARE_ASM_FUNC_SYMBOL to declare them.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
 include/linux/ftrace.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 832e65f06754..67de28464aeb 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -578,9 +578,10 @@ extern void ftrace_replace_code(int enable);
 extern int ftrace_update_ftrace_func(ftrace_func_t func);
 extern void ftrace_caller(void);
 extern void ftrace_regs_caller(void);
-extern void ftrace_call(void);
-extern void ftrace_regs_call(void);
-extern void mcount_call(void);
+
+DECLARE_ASM_FUNC_SYMBOL(ftrace_call);
+DECLARE_ASM_FUNC_SYMBOL(ftrace_regs_call);
+DECLARE_ASM_FUNC_SYMBOL(mcount_call);
 
 void ftrace_modify_all_code(int command);
 
-- 
2.33.0.800.g4c38ced690-goog

Re: [PATCH v4 06/15] ftrace: Use an opaque type for functions not callable from C

[Josh Poimboeuf]

On Thu, Sep 30, 2021 at 11:05:22AM -0700, Sami Tolvanen wrote:
> With CONFIG_CFI_CLANG, the compiler changes function references to point
> to the CFI jump table. As ftrace_call, ftrace_regs_call, and mcount_call
> are not called from C, use DECLARE_ASM_FUNC_SYMBOL to declare them.
> 
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> ---
>  include/linux/ftrace.h | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 832e65f06754..67de28464aeb 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -578,9 +578,10 @@ extern void ftrace_replace_code(int enable);
>  extern int ftrace_update_ftrace_func(ftrace_func_t func);
>  extern void ftrace_caller(void);
>  extern void ftrace_regs_caller(void);
> -extern void ftrace_call(void);
> -extern void ftrace_regs_call(void);
> -extern void mcount_call(void);
> +
> +DECLARE_ASM_FUNC_SYMBOL(ftrace_call);
> +DECLARE_ASM_FUNC_SYMBOL(ftrace_regs_call);
> +DECLARE_ASM_FUNC_SYMBOL(mcount_call);

I'm thinking DECLARE_ASM_FUNC_SYMBOL needs a better name. It's not clear
from reading it why some asm symbols need the macro and others don't.

I guess it means "an asm text symbol which isn't callable from C code
(not including alternatives)"?

DECLARE_UNCALLED_SYMBOL() maybe?

-- 
Josh

Re: [PATCH v4 06/15] ftrace: Use an opaque type for functions not callable from C

[Steven Rostedt]

On Tue, 5 Oct 2021 20:29:45 -0700
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

Thanks for Cc'ing me, as I should have been Cc'd on the original patch.

> On Thu, Sep 30, 2021 at 11:05:22AM -0700, Sami Tolvanen wrote:
> > With CONFIG_CFI_CLANG, the compiler changes function references to point
> > to the CFI jump table. As ftrace_call, ftrace_regs_call, and mcount_call
> > are not called from C, use DECLARE_ASM_FUNC_SYMBOL to declare them.

"not called from C" is a bit confusing.

> > 
> > Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> > ---
> >  include/linux/ftrace.h | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > index 832e65f06754..67de28464aeb 100644
> > --- a/include/linux/ftrace.h
> > +++ b/include/linux/ftrace.h
> > @@ -578,9 +578,10 @@ extern void ftrace_replace_code(int enable);
> >  extern int ftrace_update_ftrace_func(ftrace_func_t func);
> >  extern void ftrace_caller(void);
> >  extern void ftrace_regs_caller(void);
> > -extern void ftrace_call(void);
> > -extern void ftrace_regs_call(void);
> > -extern void mcount_call(void);
> > +
> > +DECLARE_ASM_FUNC_SYMBOL(ftrace_call);
> > +DECLARE_ASM_FUNC_SYMBOL(ftrace_regs_call);
> > +DECLARE_ASM_FUNC_SYMBOL(mcount_call);  
> 
> I'm thinking DECLARE_ASM_FUNC_SYMBOL needs a better name. It's not clear
> from reading it why some asm symbols need the macro and others don't.
> 
> I guess it means "an asm text symbol which isn't callable from C code
> (not including alternatives)"?
> 
> DECLARE_UNCALLED_SYMBOL() maybe?
> 

That's even worse ;-) Because "called" is an assembler command in x86, and
it is "called" from assembly (when you look at an objdump, it is most
definitely "called").

Perhaps DECLARE_ASM_INTERNAL_SYMBOL() ?

Or call it "DECLARE_ASM_MCOUNT_SYMBOL()" as "mcount" is the original name
of what a compiler does when passed the -pg option, and that's exactly what
those functions are.

-- Steve

Re: [PATCH v4 06/15] ftrace: Use an opaque type for functions not callable from C

[Josh Poimboeuf]

On Wed, Oct 06, 2021 at 09:02:49AM -0400, Steven Rostedt wrote:
> On Tue, 5 Oct 2021 20:29:45 -0700
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> Thanks for Cc'ing me, as I should have been Cc'd on the original patch.
> 
> > On Thu, Sep 30, 2021 at 11:05:22AM -0700, Sami Tolvanen wrote:
> > > With CONFIG_CFI_CLANG, the compiler changes function references to point
> > > to the CFI jump table. As ftrace_call, ftrace_regs_call, and mcount_call
> > > are not called from C, use DECLARE_ASM_FUNC_SYMBOL to declare them.
> 
> "not called from C" is a bit confusing.
> 
> > > 
> > > Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> > > ---
> > >  include/linux/ftrace.h | 7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > > index 832e65f06754..67de28464aeb 100644
> > > --- a/include/linux/ftrace.h
> > > +++ b/include/linux/ftrace.h
> > > @@ -578,9 +578,10 @@ extern void ftrace_replace_code(int enable);
> > >  extern int ftrace_update_ftrace_func(ftrace_func_t func);
> > >  extern void ftrace_caller(void);
> > >  extern void ftrace_regs_caller(void);
> > > -extern void ftrace_call(void);
> > > -extern void ftrace_regs_call(void);
> > > -extern void mcount_call(void);
> > > +
> > > +DECLARE_ASM_FUNC_SYMBOL(ftrace_call);
> > > +DECLARE_ASM_FUNC_SYMBOL(ftrace_regs_call);
> > > +DECLARE_ASM_FUNC_SYMBOL(mcount_call);  
> > 
> > I'm thinking DECLARE_ASM_FUNC_SYMBOL needs a better name. It's not clear
> > from reading it why some asm symbols need the macro and others don't.
> > 
> > I guess it means "an asm text symbol which isn't callable from C code
> > (not including alternatives)"?
> > 
> > DECLARE_UNCALLED_SYMBOL() maybe?
> > 
> 
> That's even worse ;-) Because "called" is an assembler command in x86, and
> it is "called" from assembly (when you look at an objdump, it is most
> definitely "called").
> 
> Perhaps DECLARE_ASM_INTERNAL_SYMBOL() ?
> 
> Or call it "DECLARE_ASM_MCOUNT_SYMBOL()" as "mcount" is the original name
> of what a compiler does when passed the -pg option, and that's exactly what
> those functions are.

But this macro is used in other places as well:

  https://lkml.kernel.org/r/20210930180531.1190642-10-samitolvanen@google.com

And many of them aren't internal to a function like the above symbols,
they're actual functions that are called in other ways.

DECLARE_UNCALLED_FROM_C() ?

-- 
Josh

Re: [PATCH v4 06/15] ftrace: Use an opaque type for functions not callable from C

[Steven Rostedt]

On Wed, 6 Oct 2021 06:54:17 -0700
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> But this macro is used in other places as well:
> 
>   https://lkml.kernel.org/r/20210930180531.1190642-10-samitolvanen@google.com
> 
> And many of them aren't internal to a function like the above symbols,
> they're actual functions that are called in other ways.
> 
> DECLARE_UNCALLED_FROM_C() ?

  DECLARE_NOT_CALLED_FROM_C() ;-)

Straight and to the point!

"uncalled" is a strange word.

-- Steve

Re: [PATCH v4 06/15] ftrace: Use an opaque type for functions not callable from C

[Sami Tolvanen]

On Wed, Oct 6, 2021 at 6:02 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Tue, 5 Oct 2021 20:29:45 -0700
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> Thanks for Cc'ing me, as I should have been Cc'd on the original patch.

Sorry about that. I'll make sure you're cc'd on the next version.

> > On Thu, Sep 30, 2021 at 11:05:22AM -0700, Sami Tolvanen wrote:
> > > With CONFIG_CFI_CLANG, the compiler changes function references to point
> > > to the CFI jump table. As ftrace_call, ftrace_regs_call, and mcount_call
> > > are not called from C, use DECLARE_ASM_FUNC_SYMBOL to declare them.
>
> "not called from C" is a bit confusing.

Any thoughts on how to make this less confusing?

Sami

Re: [PATCH v4 06/15] ftrace: Use an opaque type for functions not callable from C

[Steven Rostedt]

On Wed, 6 Oct 2021 09:31:04 -0700
Sami Tolvanen <samitolvanen@google.com> wrote:

> > > On Thu, Sep 30, 2021 at 11:05:22AM -0700, Sami Tolvanen wrote:  
> > > > With CONFIG_CFI_CLANG, the compiler changes function references to point
> > > > to the CFI jump table. As ftrace_call, ftrace_regs_call, and mcount_call
> > > > are not called from C, use DECLARE_ASM_FUNC_SYMBOL to declare them.  
> >
> > "not called from C" is a bit confusing.  
> 
> Any thoughts on how to make this less confusing?

 "Not called by C code, but injected by the compiler."

?

-- Steve

Re: [PATCH v4 06/15] ftrace: Use an opaque type for functions not callable from C

[Sami Tolvanen]

On Wed, Oct 6, 2021 at 9:58 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Wed, 6 Oct 2021 09:31:04 -0700
> Sami Tolvanen <samitolvanen@google.com> wrote:
>
> > > > On Thu, Sep 30, 2021 at 11:05:22AM -0700, Sami Tolvanen wrote:
> > > > > With CONFIG_CFI_CLANG, the compiler changes function references to point
> > > > > to the CFI jump table. As ftrace_call, ftrace_regs_call, and mcount_call
> > > > > are not called from C, use DECLARE_ASM_FUNC_SYMBOL to declare them.
> > >
> > > "not called from C" is a bit confusing.
> >
> > Any thoughts on how to make this less confusing?
>
>  "Not called by C code, but injected by the compiler."
>
> ?

Sure, sounds good to me. I'll update this in v5.

Sami

Re: [PATCH v4 06/15] ftrace: Use an opaque type for functions not callable from C

[Josh Poimboeuf]

On Wed, Oct 06, 2021 at 10:45:41AM -0700, Sami Tolvanen wrote:
> On Wed, Oct 6, 2021 at 9:58 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > On Wed, 6 Oct 2021 09:31:04 -0700
> > Sami Tolvanen <samitolvanen@google.com> wrote:
> >
> > > > > On Thu, Sep 30, 2021 at 11:05:22AM -0700, Sami Tolvanen wrote:
> > > > > > With CONFIG_CFI_CLANG, the compiler changes function references to point
> > > > > > to the CFI jump table. As ftrace_call, ftrace_regs_call, and mcount_call
> > > > > > are not called from C, use DECLARE_ASM_FUNC_SYMBOL to declare them.
> > > >
> > > > "not called from C" is a bit confusing.
> > >
> > > Any thoughts on how to make this less confusing?
> >
> >  "Not called by C code, but injected by the compiler."
> >
> > ?
> 
> Sure, sounds good to me. I'll update this in v5.

"injected by the compiler" sounds even more confusing.  It almost sounds
like those functions are generated by GCC, which they are most
definitely not.

-- 
Josh

Re: [PATCH v4 06/15] ftrace: Use an opaque type for functions not callable from C

[Steven Rostedt]

On Wed, 6 Oct 2021 13:43:35 -0700
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> On Wed, Oct 06, 2021 at 10:45:41AM -0700, Sami Tolvanen wrote:
> > On Wed, Oct 6, 2021 at 9:58 AM Steven Rostedt <rostedt@goodmis.org> wrote:  
> > >
> > > On Wed, 6 Oct 2021 09:31:04 -0700
> > > Sami Tolvanen <samitolvanen@google.com> wrote:
> > >  
> > > > > > On Thu, Sep 30, 2021 at 11:05:22AM -0700, Sami Tolvanen wrote:  
> > > > > > > With CONFIG_CFI_CLANG, the compiler changes function references to point
> > > > > > > to the CFI jump table. As ftrace_call, ftrace_regs_call, and mcount_call
> > > > > > > are not called from C, use DECLARE_ASM_FUNC_SYMBOL to declare them.  
> > > > >
> > > > > "not called from C" is a bit confusing.  
> > > >
> > > > Any thoughts on how to make this less confusing?  
> > >
> > >  "Not called by C code, but injected by the compiler."
> > >
> > > ?  
> > 
> > Sure, sounds good to me. I'll update this in v5.  
> 
> "injected by the compiler" sounds even more confusing.  It almost sounds
> like those functions are generated by GCC, which they are most
> definitely not.
> 

Heh, I was thinking of the locations that are injected (mcount / fentry) as
these are just replacements for them. Those injections are added by GCC.

So, continuing the bikeshedding, what about "not called by C code, but are
trampolines injected as calls replacing the nops at the start of
functions added by the compiler." ?

-- Steve

Re: [PATCH v4 06/15] ftrace: Use an opaque type for functions not callable from C

[Josh Poimboeuf]

On Wed, Oct 06, 2021 at 05:10:16PM -0400, Steven Rostedt wrote:
> On Wed, 6 Oct 2021 13:43:35 -0700
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> > On Wed, Oct 06, 2021 at 10:45:41AM -0700, Sami Tolvanen wrote:
> > > On Wed, Oct 6, 2021 at 9:58 AM Steven Rostedt <rostedt@goodmis.org> wrote:  
> > > >
> > > > On Wed, 6 Oct 2021 09:31:04 -0700
> > > > Sami Tolvanen <samitolvanen@google.com> wrote:
> > > >  
> > > > > > > On Thu, Sep 30, 2021 at 11:05:22AM -0700, Sami Tolvanen wrote:  
> > > > > > > > With CONFIG_CFI_CLANG, the compiler changes function references to point
> > > > > > > > to the CFI jump table. As ftrace_call, ftrace_regs_call, and mcount_call
> > > > > > > > are not called from C, use DECLARE_ASM_FUNC_SYMBOL to declare them.  
> > > > > >
> > > > > > "not called from C" is a bit confusing.  
> > > > >
> > > > > Any thoughts on how to make this less confusing?  
> > > >
> > > >  "Not called by C code, but injected by the compiler."
> > > >
> > > > ?  
> > > 
> > > Sure, sounds good to me. I'll update this in v5.  
> > 
> > "injected by the compiler" sounds even more confusing.  It almost sounds
> > like those functions are generated by GCC, which they are most
> > definitely not.
> > 
> 
> Heh, I was thinking of the locations that are injected (mcount / fentry) as
> these are just replacements for them. Those injections are added by GCC.
>
> So, continuing the bikeshedding, what about "not called by C code, but are
> trampolines injected as calls replacing the nops at the start of
> functions added by the compiler." ?

I'm not quite sure what that means, but I'll allow it ;-)

-- 
Josh

Re: [PATCH v4 06/15] ftrace: Use an opaque type for functions not callable from C

[Sami Tolvanen]

On Wed, Oct 6, 2021 at 2:24 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Wed, Oct 06, 2021 at 05:10:16PM -0400, Steven Rostedt wrote:
> > On Wed, 6 Oct 2021 13:43:35 -0700
> > Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > > On Wed, Oct 06, 2021 at 10:45:41AM -0700, Sami Tolvanen wrote:
> > > > On Wed, Oct 6, 2021 at 9:58 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> > > > >
> > > > > On Wed, 6 Oct 2021 09:31:04 -0700
> > > > > Sami Tolvanen <samitolvanen@google.com> wrote:
> > > > >
> > > > > > > > On Thu, Sep 30, 2021 at 11:05:22AM -0700, Sami Tolvanen wrote:
> > > > > > > > > With CONFIG_CFI_CLANG, the compiler changes function references to point
> > > > > > > > > to the CFI jump table. As ftrace_call, ftrace_regs_call, and mcount_call
> > > > > > > > > are not called from C, use DECLARE_ASM_FUNC_SYMBOL to declare them.
> > > > > > >
> > > > > > > "not called from C" is a bit confusing.
> > > > > >
> > > > > > Any thoughts on how to make this less confusing?
> > > > >
> > > > >  "Not called by C code, but injected by the compiler."
> > > > >
> > > > > ?
> > > >
> > > > Sure, sounds good to me. I'll update this in v5.
> > >
> > > "injected by the compiler" sounds even more confusing.  It almost sounds
> > > like those functions are generated by GCC, which they are most
> > > definitely not.
> > >
> >
> > Heh, I was thinking of the locations that are injected (mcount / fentry) as
> > these are just replacements for them. Those injections are added by GCC.
> >
> > So, continuing the bikeshedding, what about "not called by C code, but are
> > trampolines injected as calls replacing the nops at the start of
> > functions added by the compiler." ?
>
> I'm not quite sure what that means, but I'll allow it ;-)

Alright, I'll go with the updated version then. I'll also rename
DECLARE_ASM_FUNC_SYMBOL() to DECLARE_NOT_CALLED_FROM_C() unless
someone has strong objections about that.

Sami

Re: [PATCH v4 06/15] ftrace: Use an opaque type for functions not callable from C

[Steven Rostedt]

On Wed, 6 Oct 2021 16:14:17 -0700
Sami Tolvanen <samitolvanen@google.com> wrote:

> > I'm not quite sure what that means, but I'll allow it ;-)  
> 
> Alright, I'll go with the updated version then. I'll also rename
> DECLARE_ASM_FUNC_SYMBOL() to DECLARE_NOT_CALLED_FROM_C() unless
> someone has strong objections about that.

No objection from me. I like the purple color.

-- Steve

[PATCH v4 07/15] lkdtm: Disable UNSET_SMEP with CFI

[Sami Tolvanen]

Disable the UNSET_SMEP test when CONFIG_CFI_CLANG is enabled as
jumping to a call gadget would always trip CFI instead.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
Acked-by: Kees Cook <keescook@chromium.org>
---
 drivers/misc/lkdtm/bugs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/lkdtm/bugs.c b/drivers/misc/lkdtm/bugs.c
index 4282b625200f..6e8677852262 100644
--- a/drivers/misc/lkdtm/bugs.c
+++ b/drivers/misc/lkdtm/bugs.c
@@ -367,7 +367,7 @@ void lkdtm_STACK_GUARD_PAGE_TRAILING(void)
 
 void lkdtm_UNSET_SMEP(void)
 {
-#if IS_ENABLED(CONFIG_X86_64) && !IS_ENABLED(CONFIG_UML)
+#if IS_ENABLED(CONFIG_X86_64) && !IS_ENABLED(CONFIG_UML) && !IS_ENABLED(CONFIG_CFI_CLANG)
 #define MOV_CR4_DEPTH	64
 	void (*direct_write_cr4)(unsigned long val);
 	unsigned char *insn;
-- 
2.33.0.800.g4c38ced690-goog

[PATCH v4 08/15] lkdtm: Use an opaque type for lkdtm_rodata_do_nothing

[Sami Tolvanen]

Use an opaque type for lkdtm_rodata_do_nothing to stop the compiler
from generating a CFI jump table entry that jumps to .rodata.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
Acked-by: Kees Cook <keescook@chromium.org>
---
 drivers/misc/lkdtm/lkdtm.h  | 2 +-
 drivers/misc/lkdtm/perms.c  | 2 +-
 drivers/misc/lkdtm/rodata.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
index c212a253edde..2da74236c005 100644
--- a/drivers/misc/lkdtm/lkdtm.h
+++ b/drivers/misc/lkdtm/lkdtm.h
@@ -137,7 +137,7 @@ void lkdtm_REFCOUNT_TIMING(void);
 void lkdtm_ATOMIC_TIMING(void);
 
 /* rodata.c */
-void lkdtm_rodata_do_nothing(void);
+DECLARE_ASM_FUNC_SYMBOL(lkdtm_rodata_do_nothing);
 
 /* usercopy.c */
 void __init lkdtm_usercopy_init(void);
diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c
index 2dede2ef658f..fa2bd90bd8ee 100644
--- a/drivers/misc/lkdtm/perms.c
+++ b/drivers/misc/lkdtm/perms.c
@@ -151,7 +151,7 @@ void lkdtm_EXEC_VMALLOC(void)
 
 void lkdtm_EXEC_RODATA(void)
 {
-	execute_location(lkdtm_rodata_do_nothing, CODE_AS_IS);
+	execute_location((void *)lkdtm_rodata_do_nothing, CODE_AS_IS);
 }
 
 void lkdtm_EXEC_USERSPACE(void)
diff --git a/drivers/misc/lkdtm/rodata.c b/drivers/misc/lkdtm/rodata.c
index baacb876d1d9..17ed0ad4e6ae 100644
--- a/drivers/misc/lkdtm/rodata.c
+++ b/drivers/misc/lkdtm/rodata.c
@@ -3,7 +3,7 @@
  * This includes functions that are meant to live entirely in .rodata
  * (via objcopy tricks), to validate the non-executability of .rodata.
  */
-#include "lkdtm.h"
+void lkdtm_rodata_do_nothing(void);
 
 void noinstr lkdtm_rodata_do_nothing(void)
 {
-- 
2.33.0.800.g4c38ced690-goog

[PATCH v4 09/15] x86: Use an opaque type for functions not callable from C

[Sami Tolvanen]

The kernel has several assembly functions that are not directly callable
from C. Use an opaque type for these function prototypes to make misuse
harder, and to avoid the need to annotate references to these functions
for Clang's Control-Flow Integrity (CFI).

Suggested-by: Andy Lutomirski <luto@amacapital.net>
Suggested-by: Alexander Lobakin <alobakin@pm.me>
Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
---
 arch/x86/include/asm/ftrace.h         |  2 +-
 arch/x86/include/asm/idtentry.h       | 10 +++++-----
 arch/x86/include/asm/page_64.h        |  7 ++++---
 arch/x86/include/asm/paravirt_types.h |  3 ++-
 arch/x86/include/asm/processor.h      |  2 +-
 arch/x86/include/asm/proto.h          | 25 +++++++++++++------------
 arch/x86/include/asm/uaccess_64.h     |  9 +++------
 arch/x86/kernel/alternative.c         |  2 +-
 arch/x86/kernel/ftrace.c              |  2 +-
 arch/x86/kernel/paravirt.c            |  4 ++--
 arch/x86/kvm/emulate.c                |  4 ++--
 arch/x86/kvm/kvm_emulate.h            |  9 ++-------
 arch/x86/xen/enlighten_pv.c           |  6 +++---
 arch/x86/xen/xen-ops.h                | 10 +++++-----
 14 files changed, 45 insertions(+), 50 deletions(-)

diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
index 9f3130f40807..54d23f421c16 100644
--- a/arch/x86/include/asm/ftrace.h
+++ b/arch/x86/include/asm/ftrace.h
@@ -17,7 +17,7 @@
 
 #ifndef __ASSEMBLY__
 extern atomic_t modifying_ftrace_code;
-extern void __fentry__(void);
+DECLARE_ASM_FUNC_SYMBOL(__fentry__);
 
 static inline unsigned long ftrace_call_adjust(unsigned long addr)
 {
diff --git a/arch/x86/include/asm/idtentry.h b/arch/x86/include/asm/idtentry.h
index 1345088e9902..2f6d0528bdd2 100644
--- a/arch/x86/include/asm/idtentry.h
+++ b/arch/x86/include/asm/idtentry.h
@@ -27,8 +27,8 @@
  * as well which is used to emit the entry stubs in entry_32/64.S.
  */
 #define DECLARE_IDTENTRY(vector, func)					\
-	asmlinkage void asm_##func(void);				\
-	asmlinkage void xen_asm_##func(void);				\
+	DECLARE_ASM_FUNC_SYMBOL(asm_##func);				\
+	DECLARE_ASM_FUNC_SYMBOL(xen_asm_##func);				\
 	__visible void func(struct pt_regs *regs)
 
 /**
@@ -78,8 +78,8 @@ static __always_inline void __##func(struct pt_regs *regs)
  * C-handler.
  */
 #define DECLARE_IDTENTRY_ERRORCODE(vector, func)			\
-	asmlinkage void asm_##func(void);				\
-	asmlinkage void xen_asm_##func(void);				\
+	DECLARE_ASM_FUNC_SYMBOL(asm_##func);				\
+	DECLARE_ASM_FUNC_SYMBOL(xen_asm_##func);				\
 	__visible void func(struct pt_regs *regs, unsigned long error_code)
 
 /**
@@ -386,7 +386,7 @@ static __always_inline void __##func(struct pt_regs *regs)
  * - The C handler called from the C shim
  */
 #define DECLARE_IDTENTRY_DF(vector, func)				\
-	asmlinkage void asm_##func(void);				\
+	DECLARE_ASM_FUNC_SYMBOL(asm_##func);				\
 	__visible void func(struct pt_regs *regs,			\
 			    unsigned long error_code,			\
 			    unsigned long address)
diff --git a/arch/x86/include/asm/page_64.h b/arch/x86/include/asm/page_64.h
index 4bde0dc66100..d6760b6773de 100644
--- a/arch/x86/include/asm/page_64.h
+++ b/arch/x86/include/asm/page_64.h
@@ -5,6 +5,7 @@
 #include <asm/page_64_types.h>
 
 #ifndef __ASSEMBLY__
+#include <linux/linkage.h>
 #include <asm/alternative.h>
 
 /* duplicated to the one in bootmem.h */
@@ -40,9 +41,9 @@ extern unsigned long __phys_addr_symbol(unsigned long);
 #define pfn_valid(pfn)          ((pfn) < max_pfn)
 #endif
 
-void clear_page_orig(void *page);
-void clear_page_rep(void *page);
-void clear_page_erms(void *page);
+DECLARE_ASM_FUNC_SYMBOL(clear_page_orig);
+DECLARE_ASM_FUNC_SYMBOL(clear_page_rep);
+DECLARE_ASM_FUNC_SYMBOL(clear_page_erms);
 
 static inline void clear_page(void *page)
 {
diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
index d9d6b0203ec4..dfaa50d20d6a 100644
--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -38,6 +38,7 @@
 #include <asm/desc_defs.h>
 #include <asm/pgtable_types.h>
 #include <asm/nospec-branch.h>
+#include <asm/proto.h>
 
 struct page;
 struct thread_struct;
@@ -271,7 +272,7 @@ struct paravirt_patch_template {
 
 extern struct pv_info pv_info;
 extern struct paravirt_patch_template pv_ops;
-extern void (*paravirt_iret)(void);
+extern asm_func_ptr paravirt_iret;
 
 #define PARAVIRT_PATCH(x)					\
 	(offsetof(struct paravirt_patch_template, x) / sizeof(void *))
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 577f342dbfb2..02743d701fa8 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -449,7 +449,7 @@ static inline unsigned long cpu_kernelmode_gs_base(int cpu)
 
 DECLARE_PER_CPU(void *, hardirq_stack_ptr);
 DECLARE_PER_CPU(bool, hardirq_stack_inuse);
-extern asmlinkage void ignore_sysret(void);
+DECLARE_ASM_FUNC_SYMBOL(ignore_sysret);
 
 /* Save actual FS/GS selectors and bases to current->thread */
 void current_save_fsgs(void);
diff --git a/arch/x86/include/asm/proto.h b/arch/x86/include/asm/proto.h
index 8c5d1910a848..a6aa64eb3657 100644
--- a/arch/x86/include/asm/proto.h
+++ b/arch/x86/include/asm/proto.h
@@ -2,6 +2,7 @@
 #ifndef _ASM_X86_PROTO_H
 #define _ASM_X86_PROTO_H
 
+#include <linux/linkage.h>
 #include <asm/ldt.h>
 
 struct task_struct;
@@ -11,26 +12,26 @@ struct task_struct;
 void syscall_init(void);
 
 #ifdef CONFIG_X86_64
-void entry_SYSCALL_64(void);
-void entry_SYSCALL_64_safe_stack(void);
+DECLARE_ASM_FUNC_SYMBOL(entry_SYSCALL_64);
+DECLARE_ASM_FUNC_SYMBOL(entry_SYSCALL_64_safe_stack);
 long do_arch_prctl_64(struct task_struct *task, int option, unsigned long arg2);
 #endif
 
 #ifdef CONFIG_X86_32
-void entry_INT80_32(void);
-void entry_SYSENTER_32(void);
-void __begin_SYSENTER_singlestep_region(void);
-void __end_SYSENTER_singlestep_region(void);
+DECLARE_ASM_FUNC_SYMBOL(entry_INT80_32);
+DECLARE_ASM_FUNC_SYMBOL(entry_SYSENTER_32);
+DECLARE_ASM_FUNC_SYMBOL(__begin_SYSENTER_singlestep_region);
+DECLARE_ASM_FUNC_SYMBOL(__end_SYSENTER_singlestep_region);
 #endif
 
 #ifdef CONFIG_IA32_EMULATION
-void entry_SYSENTER_compat(void);
-void __end_entry_SYSENTER_compat(void);
-void entry_SYSCALL_compat(void);
-void entry_SYSCALL_compat_safe_stack(void);
-void entry_INT80_compat(void);
+DECLARE_ASM_FUNC_SYMBOL(entry_SYSENTER_compat);
+DECLARE_ASM_FUNC_SYMBOL(__end_entry_SYSENTER_compat);
+DECLARE_ASM_FUNC_SYMBOL(entry_SYSCALL_compat);
+DECLARE_ASM_FUNC_SYMBOL(entry_SYSCALL_compat_safe_stack);
+DECLARE_ASM_FUNC_SYMBOL(entry_INT80_compat);
 #ifdef CONFIG_XEN_PV
-void xen_entry_INT80_compat(void);
+DECLARE_ASM_FUNC_SYMBOL(xen_entry_INT80_compat);
 #endif
 #endif
 
diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
index 45697e04d771..df2be1efa35e 100644
--- a/arch/x86/include/asm/uaccess_64.h
+++ b/arch/x86/include/asm/uaccess_64.h
@@ -17,12 +17,9 @@
  */
 
 /* Handles exceptions in both to and from, but doesn't do access_ok */
-__must_check unsigned long
-copy_user_enhanced_fast_string(void *to, const void *from, unsigned len);
-__must_check unsigned long
-copy_user_generic_string(void *to, const void *from, unsigned len);
-__must_check unsigned long
-copy_user_generic_unrolled(void *to, const void *from, unsigned len);
+DECLARE_ASM_FUNC_SYMBOL(copy_user_enhanced_fast_string);
+DECLARE_ASM_FUNC_SYMBOL(copy_user_generic_string);
+DECLARE_ASM_FUNC_SYMBOL(copy_user_generic_unrolled);
 
 static __always_inline __must_check unsigned long
 copy_user_generic(void *to, const void *from, unsigned len)
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index e9da3dc71254..0c60a7fa6fa5 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -530,7 +530,7 @@ extern struct paravirt_patch_site __start_parainstructions[],
  * convention such that we can 'call' it from assembly.
  */
 
-extern void int3_magic(unsigned int *ptr); /* defined in asm */
+DECLARE_ASM_FUNC_SYMBOL(int3_magic);
 
 asm (
 "	.pushsection	.init.text, \"ax\", @progbits\n"
diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 1b3ce3b4a2a2..9e0c07a82b44 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -589,7 +589,7 @@ void arch_ftrace_trampoline_free(struct ftrace_ops *ops)
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 
 #ifdef CONFIG_DYNAMIC_FTRACE
-extern void ftrace_graph_call(void);
+DECLARE_ASM_FUNC_SYMBOL(ftrace_graph_call);
 
 static const char *ftrace_jmp_replace(unsigned long ip, unsigned long addr)
 {
diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
index ebc45360ffd4..737437043e40 100644
--- a/arch/x86/kernel/paravirt.c
+++ b/arch/x86/kernel/paravirt.c
@@ -138,7 +138,7 @@ void paravirt_set_sched_clock(u64 (*func)(void))
 }
 
 /* These are in entry.S */
-extern void native_iret(void);
+DECLARE_ASM_FUNC_SYMBOL(native_iret);
 
 static struct resource reserve_ioports = {
 	.start = 0,
@@ -403,7 +403,7 @@ struct paravirt_patch_template pv_ops = {
 #ifdef CONFIG_PARAVIRT_XXL
 NOKPROBE_SYMBOL(native_load_idt);
 
-void (*paravirt_iret)(void) = native_iret;
+asm_func_ptr paravirt_iret = native_iret;
 #endif
 
 EXPORT_SYMBOL(pv_ops);
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 2837110e66ed..1f81f939d982 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -201,7 +201,7 @@ struct opcode {
 		const struct escape *esc;
 		const struct instr_dual *idual;
 		const struct mode_dual *mdual;
-		void (*fastop)(struct fastop *fake);
+		fastop_t fastop;
 	} u;
 	int (*check_perm)(struct x86_emulate_ctxt *ctxt);
 };
@@ -322,7 +322,7 @@ static int fastop(struct x86_emulate_ctxt *ctxt, fastop_t fop);
 	__FOP_RET(#name)
 
 #define FOP_START(op) \
-	extern void em_##op(struct fastop *fake); \
+	DECLARE_ASM_FUNC_SYMBOL(em_##op); \
 	asm(".pushsection .text, \"ax\" \n\t" \
 	    ".global em_" #op " \n\t" \
 	    ".align " __stringify(FASTOP_SIZE) " \n\t" \
diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h
index 68b420289d7e..44c1a9324e1c 100644
--- a/arch/x86/kvm/kvm_emulate.h
+++ b/arch/x86/kvm/kvm_emulate.h
@@ -290,13 +290,8 @@ enum x86emul_mode {
 #define X86EMUL_SMM_MASK             (1 << 6)
 #define X86EMUL_SMM_INSIDE_NMI_MASK  (1 << 7)
 
-/*
- * fastop functions are declared as taking a never-defined fastop parameter,
- * so they can't be called from C directly.
- */
-struct fastop;
-
-typedef void (*fastop_t)(struct fastop *);
+/* fastop functions cannot be called from C directly. */
+typedef asm_func_ptr fastop_t;
 
 struct x86_emulate_ctxt {
 	void *vcpu;
diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index 6cf3c379bbaa..62dd7ae00e3f 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -612,8 +612,8 @@ DEFINE_IDTENTRY_RAW(xenpv_exc_machine_check)
 #endif
 
 struct trap_array_entry {
-	void (*orig)(void);
-	void (*xen)(void);
+	asm_func_ptr orig;
+	asm_func_ptr xen;
 	bool ist_okay;
 };
 
@@ -672,7 +672,7 @@ static bool __ref get_trap_addr(void **addr, unsigned int ist)
 		struct trap_array_entry *entry = trap_array + nr;
 
 		if (*addr == entry->orig) {
-			*addr = entry->xen;
+			*addr = (void *)entry->xen;
 			ist_okay = entry->ist_okay;
 			found = true;
 			break;
diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h
index 8d7ec49a35fb..b5ceb3007cfe 100644
--- a/arch/x86/xen/xen-ops.h
+++ b/arch/x86/xen/xen-ops.h
@@ -8,12 +8,12 @@
 #include <xen/xen-ops.h>
 
 /* These are code, but not functions.  Defined in entry.S */
-extern const char xen_failsafe_callback[];
+DECLARE_ASM_FUNC_SYMBOL(xen_failsafe_callback);
 
-void xen_sysenter_target(void);
+DECLARE_ASM_FUNC_SYMBOL(xen_sysenter_target);
 #ifdef CONFIG_X86_64
-void xen_syscall_target(void);
-void xen_syscall32_target(void);
+DECLARE_ASM_FUNC_SYMBOL(xen_syscall_target);
+DECLARE_ASM_FUNC_SYMBOL(xen_syscall32_target);
 #endif
 
 extern void *xen_initial_gdt;
@@ -136,7 +136,7 @@ __visible unsigned long xen_read_cr2(void);
 __visible unsigned long xen_read_cr2_direct(void);
 
 /* These are not functions, and cannot be called normally */
-__visible void xen_iret(void);
+DECLARE_ASM_FUNC_SYMBOL(xen_iret);
 
 extern int xen_panic_handler_init(void);
 
-- 
2.33.0.800.g4c38ced690-goog

[PATCH v4 10/15] x86/purgatory: Disable CFI

[Sami Tolvanen]

Disable CONFIG_CFI_CLANG for the stand-alone purgatory.ro.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
 arch/x86/purgatory/Makefile | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/purgatory/Makefile b/arch/x86/purgatory/Makefile
index 95ea17a9d20c..911954fec31c 100644
--- a/arch/x86/purgatory/Makefile
+++ b/arch/x86/purgatory/Makefile
@@ -55,6 +55,10 @@ ifdef CONFIG_RETPOLINE
 PURGATORY_CFLAGS_REMOVE		+= $(RETPOLINE_CFLAGS)
 endif
 
+ifdef CONFIG_CFI_CLANG
+PURGATORY_CFLAGS_REMOVE		+= $(CC_FLAGS_CFI)
+endif
+
 CFLAGS_REMOVE_purgatory.o	+= $(PURGATORY_CFLAGS_REMOVE)
 CFLAGS_purgatory.o		+= $(PURGATORY_CFLAGS)
 
-- 
2.33.0.800.g4c38ced690-goog

Re: [PATCH v4 10/15] x86/purgatory: Disable CFI

[Nick Desaulniers]

On Thu, Sep 30, 2021 at 11:06 AM Sami Tolvanen <samitolvanen@google.com> wrote:
>
> Disable CONFIG_CFI_CLANG for the stand-alone purgatory.ro.
>
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>

Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

> ---
>  arch/x86/purgatory/Makefile | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/x86/purgatory/Makefile b/arch/x86/purgatory/Makefile
> index 95ea17a9d20c..911954fec31c 100644
> --- a/arch/x86/purgatory/Makefile
> +++ b/arch/x86/purgatory/Makefile
> @@ -55,6 +55,10 @@ ifdef CONFIG_RETPOLINE
>  PURGATORY_CFLAGS_REMOVE                += $(RETPOLINE_CFLAGS)
>  endif
>
> +ifdef CONFIG_CFI_CLANG
> +PURGATORY_CFLAGS_REMOVE                += $(CC_FLAGS_CFI)
> +endif
> +
>  CFLAGS_REMOVE_purgatory.o      += $(PURGATORY_CFLAGS_REMOVE)
>  CFLAGS_purgatory.o             += $(PURGATORY_CFLAGS)
>
> --
> 2.33.0.800.g4c38ced690-goog
>


-- 
Thanks,
~Nick Desaulniers

[PATCH v4 11/15] x86, relocs: Ignore __typeid__ relocations

[Sami Tolvanen]

From: Kees Cook <keescook@chromium.org>

The __typeid__* symbols aren't actually relocations, so they can be
ignored during relocation generation.

Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
 arch/x86/tools/relocs.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/tools/relocs.c b/arch/x86/tools/relocs.c
index 27c82207d387..5304a6037924 100644
--- a/arch/x86/tools/relocs.c
+++ b/arch/x86/tools/relocs.c
@@ -51,6 +51,7 @@ static const char * const sym_regex_kernel[S_NSYMTYPES] = {
 	"^(xen_irq_disable_direct_reloc$|"
 	"xen_save_fl_direct_reloc$|"
 	"VDSO|"
+	"__typeid__|"
 	"__crc_)",
 
 /*
@@ -811,6 +812,12 @@ static int do_reloc64(struct section *sec, Elf_Rel *rel, ElfW(Sym) *sym,
 			    symname);
 		break;
 
+	case R_X86_64_8:
+		if (!shn_abs || !is_reloc(S_ABS, symname))
+			die("Non-whitelisted %s relocation: %s\n",
+				rel_type(r_type), symname);
+		break;
+
 	case R_X86_64_32:
 	case R_X86_64_32S:
 	case R_X86_64_64:
-- 
2.33.0.800.g4c38ced690-goog

Re: [PATCH v4 11/15] x86, relocs: Ignore __typeid__ relocations

[Josh Poimboeuf]

On Thu, Sep 30, 2021 at 11:05:27AM -0700, Sami Tolvanen wrote:
> From: Kees Cook <keescook@chromium.org>
> 
> The __typeid__* symbols aren't actually relocations, so they can be
> ignored during relocation generation.

Then what are they?  It would be good to add that information here.

-- 
Josh

Re: [PATCH v4 11/15] x86, relocs: Ignore __typeid__ relocations

[Sami Tolvanen]

On Tue, Oct 5, 2021 at 8:31 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Thu, Sep 30, 2021 at 11:05:27AM -0700, Sami Tolvanen wrote:
> > From: Kees Cook <keescook@chromium.org>
> >
> > The __typeid__* symbols aren't actually relocations, so they can be
> > ignored during relocation generation.
>
> Then what are they?  It would be good to add that information here.

These relocations are for compiler-generated constants that it uses
for indirect call type checking. I think we can clarify this in the
next version.

Sami

[PATCH v4 12/15] x86, module: Ignore __typeid__ relocations

[Sami Tolvanen]

Ignore the __typeid__ relocations generated with CONFIG_CFI_CLANG
when loading modules.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
 arch/x86/kernel/module.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/kernel/module.c b/arch/x86/kernel/module.c
index 5e9a34b5bd74..c4aeba237eef 100644
--- a/arch/x86/kernel/module.c
+++ b/arch/x86/kernel/module.c
@@ -197,6 +197,10 @@ static int __apply_relocate_add(Elf64_Shdr *sechdrs,
 			val -= (u64)loc;
 			write(loc, &val, 8);
 			break;
+		case R_X86_64_8:
+			if (!strncmp(strtab + sym->st_name, "__typeid__", 10))
+				break;
+			fallthrough;
 		default:
 			pr_err("%s: Unknown rela relocation: %llu\n",
 			       me->name, ELF64_R_TYPE(rel[i].r_info));
-- 
2.33.0.800.g4c38ced690-goog

[PATCH v4 13/15] x86, cpu: Use LTO for cpu.c with CFI

[Sami Tolvanen]

Allow LTO to be used for cpu.c when CONFIG_CFI_CLANG is enabled to avoid
indirect call failures. CFI requires Clang >= 13, which doesn't have the
stack protector inlining bug.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
---
 arch/x86/power/Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/power/Makefile b/arch/x86/power/Makefile
index 379777572bc9..a0532851fed7 100644
--- a/arch/x86/power/Makefile
+++ b/arch/x86/power/Makefile
@@ -4,9 +4,11 @@
 # itself be stack-protected
 CFLAGS_cpu.o	:= -fno-stack-protector
 
+ifndef CONFIG_CFI_CLANG
 # Clang may incorrectly inline functions with stack protector enabled into
 # __restore_processor_state(): https://bugs.llvm.org/show_bug.cgi?id=47479
 CFLAGS_REMOVE_cpu.o := $(CC_FLAGS_LTO)
+endif
 
 obj-$(CONFIG_PM_SLEEP)		+= cpu.o
 obj-$(CONFIG_HIBERNATION)	+= hibernate_$(BITS).o hibernate_asm_$(BITS).o hibernate.o
-- 
2.33.0.800.g4c38ced690-goog

[PATCH v4 14/15] x86, kprobes: Fix optprobe_template_func type mismatch

[Sami Tolvanen]

The optprobe_template_func symbol is defined in inline assembly,
but it's not marked global, which conflicts with the C declaration
needed for STACK_FRAME_NON_STANDARD and confuses the compiler when
CONFIG_CFI_CLANG is enabled.

Marking the symbol global would make the compiler happy, but as the
compiler also generates a CFI jump table entry for all address-taken
functions, the jump table ends up containing a jump to the .rodata
section where optprobe_template_func resides, which results in an
objtool warning.

Use ASM_STACK_FRAME_NON_STANDARD instead to avoid both issues.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
---
 arch/x86/kernel/kprobes/opt.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
index 71425ebba98a..95375ef5deee 100644
--- a/arch/x86/kernel/kprobes/opt.c
+++ b/arch/x86/kernel/kprobes/opt.c
@@ -103,6 +103,7 @@ static void synthesize_set_arg1(kprobe_opcode_t *addr, unsigned long val)
 asm (
 			".pushsection .rodata\n"
 			"optprobe_template_func:\n"
+			ASM_STACK_FRAME_NON_STANDARD(optprobe_template_func)
 			".global optprobe_template_entry\n"
 			"optprobe_template_entry:\n"
 #ifdef CONFIG_X86_64
@@ -154,9 +155,6 @@ asm (
 			"optprobe_template_end:\n"
 			".popsection\n");
 
-void optprobe_template_func(void);
-STACK_FRAME_NON_STANDARD(optprobe_template_func);
-
 #define TMPL_CLAC_IDX \
 	((long)optprobe_template_clac - (long)optprobe_template_entry)
 #define TMPL_MOVE_IDX \
-- 
2.33.0.800.g4c38ced690-goog

[PATCH v4 15/15] x86, build: Allow CONFIG_CFI_CLANG to be selected

[Sami Tolvanen]

Select ARCH_SUPPORTS_CFI_CLANG to allow CFI to be enabled with
Clang >= 13.

Link: https://bugs.llvm.org/show_bug.cgi?id=51588
Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
 arch/x86/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 16e216b57863..ea6d255a125f 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -107,6 +107,7 @@ config X86
 	select ARCH_SUPPORTS_KMAP_LOCAL_FORCE_MAP	if NR_CPUS <= 4096
 	select ARCH_SUPPORTS_LTO_CLANG
 	select ARCH_SUPPORTS_LTO_CLANG_THIN
+	select ARCH_SUPPORTS_CFI_CLANG		if X86_64 && CLANG_VERSION >= 130000
 	select ARCH_USE_BUILTIN_BSWAP
 	select ARCH_USE_MEMTEST
 	select ARCH_USE_QUEUED_RWLOCKS
-- 
2.33.0.800.g4c38ced690-goog

Re: [PATCH v4 00/15] x86: Add support for Clang CFI

[Nick Desaulniers]

On Thu, Sep 30, 2021 at 11:05 AM Sami Tolvanen <samitolvanen@google.com> wrote:
>
> This series adds support for Clang's Control-Flow Integrity (CFI)
> checking to x86_64. With CFI, the compiler injects a runtime
> check before each indirect function call to ensure the target is
> a valid function with the correct static type. This restricts
> possible call targets and makes it more difficult for an attacker
> to exploit bugs that allow the modification of stored function
> pointers. For more details, see:
>
>   https://clang.llvm.org/docs/ControlFlowIntegrity.html
>
> Note that v4 is based on tip/master. The first two patches contain
> objtool support for CFI, the remaining patches change function
> declarations to use opaque types, fix type mismatch issues that
> confuse the compiler, and disable CFI where it can't be used.
>
> You can also pull this series from
>
>   https://github.com/samitolvanen/linux.git x86-cfi-v4

I tested this series with near ToT clang with CONFIG_LTO_CLANG_THIN=y
and our small buildroot-based userspace image in QEMU, then again with
CONFIG_LTO_CLANG_FULL=y. No new warnings from objtool, everything
seemed fine.  FWIW,

Tested-by: Nick Desaulniers <ndesaulniers@google.com>

>
> ---
> Changes in v4:
> - Dropped the extable patch after the code was refactored in -tip.
>
> - Switched to __section() instead of open-coding the attribute.
>
> - Added an explicit ifdef for filtering out CC_FLAGS_CFI in
>   purgatory for readability.
>
> - Added a comment to arch_cfi_jump_reloc_offset() in objtool.
>
> Changes in v3:
> - Dropped Clang requirement to >= 13 after the missing compiler
>   fix was backported there.
>
> - Added DEFINE_CFI_IMMEDIATE_RETURN_STUB to address the issue
>   with tp_stub_func in kernel/tracepoint.c.
>
> - Renamed asm_func_t to asm_func_ptr.
>
> - Changed extable handlers to use __cficanonical instead of
>   disabling CFI for fixup_exception.
>
> Changes in v2:
> - Dropped the first objtool patch as the warnings were fixed in
>   separate patches.
>
> - Changed fix_cfi_relocs() in objtool to not rely on jump table
>   symbols, and to return an error if it can't find a relocation.
>
> - Fixed a build issue with ASM_STACK_FRAME_NON_STANDARD().
>
> - Dropped workarounds for inline assembly references to
>   address-taken static functions with CFI as this was fixed in
>   the compiler.
>
> - Changed the C declarations of non-callable functions to use
>   opaque types and dropped the function_nocfi() patches.
>
> - Changed ARCH_SUPPORTS_CFI_CLANG to depend on Clang >=14 for
>   the compiler fixes.
>
>
> Kees Cook (1):
>   x86, relocs: Ignore __typeid__ relocations
>
> Sami Tolvanen (14):
>   objtool: Add CONFIG_CFI_CLANG support
>   objtool: Add ASM_STACK_FRAME_NON_STANDARD
>   linkage: Add DECLARE_ASM_FUNC_SYMBOL
>   cfi: Add DEFINE_CFI_IMMEDIATE_RETURN_STUB
>   tracepoint: Exclude tp_stub_func from CFI checking
>   ftrace: Use an opaque type for functions not callable from C
>   lkdtm: Disable UNSET_SMEP with CFI
>   lkdtm: Use an opaque type for lkdtm_rodata_do_nothing
>   x86: Use an opaque type for functions not callable from C
>   x86/purgatory: Disable CFI
>   x86, module: Ignore __typeid__ relocations
>   x86, cpu: Use LTO for cpu.c with CFI
>   x86, kprobes: Fix optprobe_template_func type mismatch
>   x86, build: Allow CONFIG_CFI_CLANG to be selected
>
>  arch/x86/Kconfig                      |  1 +
>  arch/x86/include/asm/ftrace.h         |  2 +-
>  arch/x86/include/asm/idtentry.h       | 10 +++---
>  arch/x86/include/asm/page_64.h        |  7 ++--
>  arch/x86/include/asm/paravirt_types.h |  3 +-
>  arch/x86/include/asm/processor.h      |  2 +-
>  arch/x86/include/asm/proto.h          | 25 ++++++-------
>  arch/x86/include/asm/uaccess_64.h     |  9 ++---
>  arch/x86/kernel/alternative.c         |  2 +-
>  arch/x86/kernel/ftrace.c              |  2 +-
>  arch/x86/kernel/kprobes/opt.c         |  4 +--
>  arch/x86/kernel/module.c              |  4 +++
>  arch/x86/kernel/paravirt.c            |  4 +--
>  arch/x86/kvm/emulate.c                |  4 +--
>  arch/x86/kvm/kvm_emulate.h            |  9 ++---
>  arch/x86/power/Makefile               |  2 ++
>  arch/x86/purgatory/Makefile           |  4 +++
>  arch/x86/tools/relocs.c               |  7 ++++
>  arch/x86/xen/enlighten_pv.c           |  6 ++--
>  arch/x86/xen/xen-ops.h                | 10 +++---
>  drivers/misc/lkdtm/bugs.c             |  2 +-
>  drivers/misc/lkdtm/lkdtm.h            |  2 +-
>  drivers/misc/lkdtm/perms.c            |  2 +-
>  drivers/misc/lkdtm/rodata.c           |  2 +-
>  include/asm-generic/vmlinux.lds.h     | 11 ++++++
>  include/linux/cfi.h                   | 13 +++++++
>  include/linux/ftrace.h                |  7 ++--
>  include/linux/linkage.h               | 13 +++++++
>  include/linux/objtool.h               |  6 ++++
>  kernel/cfi.c                          | 24 ++++++++++++-
>  kernel/tracepoint.c                   |  5 +--
>  tools/include/linux/objtool.h         |  6 ++++
>  tools/objtool/arch/x86/decode.c       | 17 +++++++++
>  tools/objtool/elf.c                   | 51 +++++++++++++++++++++++++++
>  tools/objtool/include/objtool/arch.h  |  3 ++
>  tools/objtool/include/objtool/elf.h   |  2 +-
>  36 files changed, 217 insertions(+), 66 deletions(-)
>
>
> base-commit: d4bfebd9ef497ee0afb498f6028a5074a6ccf307
> --
> 2.33.0.800.g4c38ced690-goog
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v4 00/15] x86: Add support for Clang CFI

[Josh Poimboeuf]

On Thu, Sep 30, 2021 at 11:05:16AM -0700, Sami Tolvanen wrote:
> This series adds support for Clang's Control-Flow Integrity (CFI)
> checking to x86_64. With CFI, the compiler injects a runtime
> check before each indirect function call to ensure the target is
> a valid function with the correct static type. This restricts
> possible call targets and makes it more difficult for an attacker
> to exploit bugs that allow the modification of stored function
> pointers. For more details, see:
> 
>   https://clang.llvm.org/docs/ControlFlowIntegrity.html
> 
> Note that v4 is based on tip/master. The first two patches contain
> objtool support for CFI, the remaining patches change function
> declarations to use opaque types, fix type mismatch issues that
> confuse the compiler, and disable CFI where it can't be used.
> 
> You can also pull this series from
> 
>   https://github.com/samitolvanen/linux.git x86-cfi-v4

Does this work for indirect calls made from alternatives?

I'm also wondering whether this works on CONFIG_RETPOLINE systems which
disable retpolines at runtime, combined with Peter's patch to use
objtool to replace retpoline thunk calls with indirect branches:

  9bc0bb50727c ("objtool/x86: Rewrite retpoline thunk calls")

Since presumably objtool runs after the CFI stuff is inserted.

-- 
Josh

Re: [PATCH v4 00/15] x86: Add support for Clang CFI

[Sami Tolvanen]

On Tue, Oct 5, 2021 at 1:37 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Thu, Sep 30, 2021 at 11:05:16AM -0700, Sami Tolvanen wrote:
> > This series adds support for Clang's Control-Flow Integrity (CFI)
> > checking to x86_64. With CFI, the compiler injects a runtime
> > check before each indirect function call to ensure the target is
> > a valid function with the correct static type. This restricts
> > possible call targets and makes it more difficult for an attacker
> > to exploit bugs that allow the modification of stored function
> > pointers. For more details, see:
> >
> >   https://clang.llvm.org/docs/ControlFlowIntegrity.html
> >
> > Note that v4 is based on tip/master. The first two patches contain
> > objtool support for CFI, the remaining patches change function
> > declarations to use opaque types, fix type mismatch issues that
> > confuse the compiler, and disable CFI where it can't be used.
> >
> > You can also pull this series from
> >
> >   https://github.com/samitolvanen/linux.git x86-cfi-v4
>
> Does this work for indirect calls made from alternatives?

It works in the sense that indirect calls made from alternatives won't
trip CFI. The compiler doesn't instrument inline assembly.

> I'm also wondering whether this works on CONFIG_RETPOLINE systems which
> disable retpolines at runtime, combined with Peter's patch to use
> objtool to replace retpoline thunk calls with indirect branches:
>
>   9bc0bb50727c ("objtool/x86: Rewrite retpoline thunk calls")
>
> Since presumably objtool runs after the CFI stuff is inserted.

The indirect call checking is before the retpoline thunk call, so
replacing the call with an indirect call isn't a problem.

Sami

Re: [PATCH v4 00/15] x86: Add support for Clang CFI

[Josh Poimboeuf]

On Tue, Oct 05, 2021 at 02:52:46PM -0700, Sami Tolvanen wrote:
> On Tue, Oct 5, 2021 at 1:37 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > On Thu, Sep 30, 2021 at 11:05:16AM -0700, Sami Tolvanen wrote:
> > > This series adds support for Clang's Control-Flow Integrity (CFI)
> > > checking to x86_64. With CFI, the compiler injects a runtime
> > > check before each indirect function call to ensure the target is
> > > a valid function with the correct static type. This restricts
> > > possible call targets and makes it more difficult for an attacker
> > > to exploit bugs that allow the modification of stored function
> > > pointers. For more details, see:
> > >
> > >   https://clang.llvm.org/docs/ControlFlowIntegrity.html
> > >
> > > Note that v4 is based on tip/master. The first two patches contain
> > > objtool support for CFI, the remaining patches change function
> > > declarations to use opaque types, fix type mismatch issues that
> > > confuse the compiler, and disable CFI where it can't be used.
> > >
> > > You can also pull this series from
> > >
> > >   https://github.com/samitolvanen/linux.git x86-cfi-v4
> >
> > Does this work for indirect calls made from alternatives?
> 
> It works in the sense that indirect calls made from alternatives won't
> trip CFI. The compiler doesn't instrument inline assembly.
> 
> > I'm also wondering whether this works on CONFIG_RETPOLINE systems which
> > disable retpolines at runtime, combined with Peter's patch to use
> > objtool to replace retpoline thunk calls with indirect branches:
> >
> >   9bc0bb50727c ("objtool/x86: Rewrite retpoline thunk calls")
> >
> > Since presumably objtool runs after the CFI stuff is inserted.
> 
> The indirect call checking is before the retpoline thunk call, so
> replacing the call with an indirect call isn't a problem.

Ah right.  I managed to forget how this worked and was thinking this
intercepted the indirect call rather than the function pointer.

-- 
Josh