Re: [RFC][PATCH] objtool: STAC/CLAC validation

From: Peter Zijlstra
Date: Sat Feb 23 2019 - 05:52:54 EST

On Sat, Feb 23, 2019 at 09:37:06AM +0100, Peter Zijlstra wrote:
> On Fri, Feb 22, 2019 at 03:55:25PM -0800, Andy Lutomirski wrote:
> > On Fri, Feb 22, 2019 at 2:26 PM Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>
> > > arch/x86/entry/entry_64.o: warning: objtool: .altinstr_replacement+0xb1: redundant CLAC
> >
> > Does objtool understand altinstr?
>
> Yep, otherwise it would've never found any of the STAC/CLAC instructions
> to begin with, they're all alternatives. The emitted code is all NOPs.
>
> > If it understands that this is an
> > altinstr associated with a not-actually-a-fuction (i.e. END not
> > ENDPROC) piece of code, it could suppress this warning.
>
> Using readelf on entry_64.o the symbol type appears to be STT_NOTYPE for
> these 'functions', so yes, I can try and ignore this warning for those.
>
> > > +#define AC_SAFE(func) \
> > > + static void __used __section(.discard.ac_safe) \
> > > + *__func_ac_safe_##func = func
> >
> > Are we okay with annotating function *declarations* in a way that
> > their addresses get taken whenever the declaration is processed? It
> > would be nice if we could avoid this.
> >
> > I'm wondering if we can just change the code that does getreg() and
> > load_gs_index() so it doesn't do it with AC set. Also, what about
> > paravirt kernels? They'll call into PV code for load_gs_index() with
> > AC set.
>
> Fixing that code would be fine; but we need that annotation regardless,
> read Linus' email a little further back, he wants things like
> copy_{to,from}_user_unsafe().
>
> Those really would need to be marked AC-safe, there's no inlining that.
>
> That said, yes these annotations _suck_. But it's what we had and works,
> I'm just copying it (from STACK_FRAME_NON_STANDARD).
>
> That said; maybe we can do something like:
>
> #define AC_SAFE(func) \
> asm (".pushsection .discard.ac_safe_sym\n\t" \
> "999: .ascii \"" #func "\"\n\t" \
> ".popsection\n\t" \
> ".pushsection .discard.ac_safe\n\t" \
> _ASM_PTR " 999b\n\t" \
> ".popsection")
>
> I just don't have a clue on how to read that in objtool; weak elf foo.
> I'll see if I can make it work.

Fixed all that. Should probably also convert that NON_STANDARD stuff to
this new shiny thing.

Retained the ptrace muck because it's a nice test case, your patch is
obviously a better fix there.

Haven't bothered looking at alternatives yet, this is a
defconfig+tracing build.

Weekend now.

---
arch/x86/include/asm/special_insns.h | 2 +
arch/x86/kernel/ptrace.c | 3 +-
include/linux/frame.h | 38 ++++++++++++
tools/objtool/Makefile | 2 +-
tools/objtool/arch.h | 6 +-
tools/objtool/arch/x86/decode.c | 22 ++++++-
tools/objtool/check.c | 117 ++++++++++++++++++++++++++++++++++-
tools/objtool/check.h | 2 +-
tools/objtool/elf.h | 1 +
9 files changed, 187 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
index 43c029cdc3fe..cd31e4433f4c 100644
--- a/arch/x86/include/asm/special_insns.h
+++ b/arch/x86/include/asm/special_insns.h
@@ -5,6 +5,7 @@

#ifdef __KERNEL__

+#include <linux/frame.h>
#include <asm/nops.h>

/*
@@ -135,6 +136,7 @@ static inline void native_wbinvd(void)
}

extern asmlinkage void native_load_gs_index(unsigned);
+AC_SAFE(native_load_gs_index);

static inline unsigned long __read_cr4(void)
{
diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 4b8ee05dd6ad..e278b4055a8b 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -420,7 +420,7 @@ static int putreg(struct task_struct *child,
return 0;
}

-static unsigned long getreg(struct task_struct *task, unsigned long offset)
+static notrace unsigned long getreg(struct task_struct *task, unsigned long offset)
{
switch (offset) {
case offsetof(struct user_regs_struct, cs):
@@ -444,6 +444,7 @@ static unsigned long getreg(struct task_struct *task, unsigned long offset)

return *pt_regs_access(task_pt_regs(task), offset);
}
+AC_SAFE(getreg);

static int genregs_get(struct task_struct *target,
const struct user_regset *regset,
diff --git a/include/linux/frame.h b/include/linux/frame.h
index 02d3ca2d9598..870a894bc586 100644
--- a/include/linux/frame.h
+++ b/include/linux/frame.h
@@ -21,4 +21,42 @@

#endif /* CONFIG_STACK_VALIDATION */

+#if defined(CONFIG_STACK_VALIDATION)
+/*
+ * This macro marks functions as AC-safe, that is, it is safe to call from an
+ * EFLAGS.AC enabled region (typically user_access_begin() /
+ * user_access_end()).
+ *
+ * These functions in turn will only call AC-safe functions themselves (which
+ * precludes tracing, including __fentry__ and scheduling, including
+ * preempt_enable).
+ *
+ * AC-safe functions will obviously also not change EFLAGS.AC themselves.
+ */
+#define AC_SAFE(func) \
+ asm (".pushsection .discard.ac_safe_sym, \"S\", @3\n\t" \
+ "999: .ascii \"" #func "\"\n\t" \
+ " .byte 0\n\t" \
+ ".popsection\n\t" \
+ ".pushsection .discard.ac_safe\n\t" \
+ ".long 999b - .\n\t" \
+ ".popsection")
+
+/*
+ * SHT_STRTAB sayeth:
+ *
+ * - The initial byte of a string table is \0. This allows an string offset
+ * value of zero to denote the NULL string.
+ *
+ * Works just fine without this, but since we set the proper section type, lets
+ * not confuse anybody reading this.
+ */
+asm(".pushsection .discard.ac_safe_sym, \"S\", @3\n\t"
+ ".byte 0\n\t"
+ ".popsection\n\t");
+
+#else
+#define AC_SAFE(func)
+#endif
+
#endif /* _LINUX_FRAME_H */
diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
index c9d038f91af6..5a64e5fb9d7a 100644
--- a/tools/objtool/Makefile
+++ b/tools/objtool/Makefile
@@ -31,7 +31,7 @@ INCLUDES := -I$(srctree)/tools/include \
-I$(srctree)/tools/arch/$(HOSTARCH)/include/uapi \
-I$(srctree)/tools/objtool/arch/$(ARCH)/include
WARNINGS := $(EXTRA_WARNINGS) -Wno-switch-default -Wno-switch-enum -Wno-packed
-CFLAGS += -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES)
+CFLAGS += -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) -ggdb3
LDFLAGS += -lelf $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS)

# Allow old libelf to be used:
diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h
index b0d7dc3d71b5..9795d83cbc01 100644
--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -33,7 +33,11 @@
#define INSN_STACK 8
#define INSN_BUG 9
#define INSN_NOP 10
-#define INSN_OTHER 11
+#define INSN_STAC 11
+#define INSN_CLAC 12
+#define INSN_STD 13
+#define INSN_CLD 14
+#define INSN_OTHER 15
#define INSN_LAST INSN_OTHER

enum op_dest_type {
diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
index 540a209b78ab..bee32ad53ecf 100644
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -369,7 +369,19 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,

case 0x0f:

- if (op2 >= 0x80 && op2 <= 0x8f) {
+ if (op2 == 0x01) {
+
+ if (modrm == 0xca) {
+
+ *type = INSN_CLAC;
+
+ } else if (modrm == 0xcb) {
+
+ *type = INSN_STAC;
+
+ }
+
+ } else if (op2 >= 0x80 && op2 <= 0x8f) {

*type = INSN_JUMP_CONDITIONAL;

@@ -444,6 +456,14 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
*type = INSN_CALL;
break;

+ case 0xfc:
+ *type = INSN_CLD;
+ break;
+
+ case 0xfd:
+ *type = INSN_STD;
+ break;
+
case 0xff:
if (modrm_reg == 2 || modrm_reg == 3)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 0414a0d52262..3dedfa19cb09 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -451,6 +451,41 @@ static void add_ignores(struct objtool_file *file)
}
}

+static int add_ac_safe(struct objtool_file *file)
+{
+ struct section *sec_sym, *sec;
+ struct symbol *func;
+ struct rela *rela;
+ const char *name;
+
+ sec = find_section_by_name(file->elf, ".rela.discard.ac_safe");
+ if (!sec)
+ return 0;
+
+ sec_sym = find_section_by_name(file->elf, ".discard.ac_safe_sym");
+ if (!sec_sym) {
+ WARN("missing AC-safe symbols");
+ return -1;
+ }
+
+ list_for_each_entry(rela, &sec->rela_list, list) {
+ if (rela->sym->type != STT_SECTION) {
+ WARN("unexpected relocation symbol type in %s", sec->name);
+ return -1;
+ }
+
+ name = elf_strptr(file->elf->elf, sec_sym->idx, rela->addend);
+
+ func = find_symbol_by_name(file->elf, name);
+ if (!func)
+ continue;
+
+ func->ac_safe = true;
+ }
+
+ return 0;
+}
+
/*
* FIXME: For now, just ignore any alternatives which add retpolines. This is
* a temporary hack, as it doesn't allow ORC to unwind from inside a retpoline.
@@ -695,6 +730,7 @@ static int handle_group_alt(struct objtool_file *file,
last_new_insn = insn;

insn->ignore = orig_insn->ignore_alts;
+ insn->func = orig_insn->func;

if (insn->type != INSN_JUMP_CONDITIONAL &&
insn->type != INSN_JUMP_UNCONDITIONAL)
@@ -1239,6 +1275,10 @@ static int decode_sections(struct objtool_file *file)

add_ignores(file);

+ ret = add_ac_safe(file);
+ if (ret)
+ return ret;
+
ret = add_nospec_ignores(file);
if (ret)
return ret;
@@ -1902,6 +1942,15 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,
switch (insn->type) {

case INSN_RETURN:
+ if (state.ac) {
+ WARN_FUNC("return with AC set", sec, insn->offset);
+ return 1;
+ }
+ if (state.df) {
+ WARN_FUNC("return with DF set", sec, insn->offset);
+ return 1;
+ }
+
if (func && has_modified_stack_frame(&state)) {
WARN_FUNC("return with modified stack frame",
sec, insn->offset);
@@ -1917,6 +1966,17 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,
return 0;

case INSN_CALL:
+ if ((state.ac_safe || state.ac) && !insn->call_dest->ac_safe) {
+ WARN_FUNC("call to %s() with AC set", sec, insn->offset,
+ insn->call_dest->name);
+ return 1;
+ }
+ if (state.df) {
+ WARN_FUNC("call to %s() with DF set", sec, insn->offset,
+ insn->call_dest->name);
+ return 1;
+ }
+
if (is_fentry_call(insn))
break;

@@ -1926,8 +1986,25 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,
if (ret == -1)
return 1;

- /* fallthrough */
+ if (!no_fp && func && !has_valid_stack_frame(&state)) {
+ WARN_FUNC("call without frame pointer save/setup",
+ sec, insn->offset);
+ return 1;
+ }
+ break;
+
case INSN_CALL_DYNAMIC:
+ if ((state.ac_safe || state.ac) && !insn->call_dest->ac_safe) {
+ WARN_FUNC("call to %s() with AC set", sec, insn->offset,
+ insn->call_dest->name);
+ return 1;
+ }
+ if (state.df) {
+ WARN_FUNC("call to %s() with DF set", sec, insn->offset,
+ insn->call_dest->name);
+ return 1;
+ }
+
if (!no_fp && func && !has_valid_stack_frame(&state)) {
WARN_FUNC("call without frame pointer save/setup",
sec, insn->offset);
@@ -1980,6 +2057,42 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,

break;

+ case INSN_STAC:
+ if (state.ac_safe || state.ac) {
+ WARN_FUNC("recursive STAC", sec, insn->offset);
+ return 1;
+ }
+ state.ac = true;
+ break;
+
+ case INSN_CLAC:
+ if (!state.ac && insn->func) {
+ WARN_FUNC("redundant CLAC", sec, insn->offset);
+ return 1;
+ }
+ if (state.ac_safe) {
+ WARN_FUNC("AC-safe clears AC", sec, insn->offset);
+ return 1;
+ }
+ state.ac = false;
+ break;
+
+ case INSN_STD:
+ if (state.df) {
+ WARN_FUNC("recursive STD", sec, insn->offset);
+ return 1;
+ }
+ state.df = true;
+ break;
+
+ case INSN_CLD:
+ if (!state.df && insn->func) {
+ WARN_FUNC("redundant CLD", sec, insn->offset);
+ return 1;
+ }
+ state.df = false;
+ break;
+
default:
break;
}
@@ -2141,6 +2254,8 @@ static int validate_functions(struct objtool_file *file)
if (!insn || insn->ignore)
continue;

+ state.ac_safe = func->ac_safe;
+
ret = validate_branch(file, insn, state);
warnings += ret;
}
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index e6e8a655b556..602634792151 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -31,7 +31,7 @@ struct insn_state {
int stack_size;
unsigned char type;
bool bp_scratch;
- bool drap, end;
+ bool drap, end, ac, ac_safe, df;
int drap_reg, drap_offset;
struct cfi_reg vals[CFI_NUM_REGS];
};
diff --git a/tools/objtool/elf.h b/tools/objtool/elf.h
index bc97ed86b9cd..064c3df31e40 100644
--- a/tools/objtool/elf.h
+++ b/tools/objtool/elf.h
@@ -62,6 +62,7 @@ struct symbol {
unsigned long offset;
unsigned int len;
struct symbol *pfunc, *cfunc;
+ bool ac_safe;
};

struct rela {