It is important that UACCESS regions are as small as possible;
furthermore the UACCESS state is not scheduled, so doing anything that
might directly call into the scheduler will cause random code to be
ran with UACCESS enabled.
Teach objtool too track UACCESS state and warn about any CALL made
while UACCESS is enabled. This very much includes the __fentry__()
tracing calls and __preempt_schedule() calls.
Note that exceptions _do_ save/restore the UACCESS state, and therefore
they can drive preemption. This also means that all exception handlers
must have an otherwise dedundant UACCESS disable instruction;
therefore ignore this warning for !STT_FUNC code (exception handlers
are not normal functions).
It also provides a UACCESS_SAFE() annotation which allows explicit
annotation. This is meant to be used for future things like:
unsafe_copy_{to,from}_user().
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
include/linux/frame.h | 30 +++++++++++-
tools/objtool/arch.h | 4 +
tools/objtool/arch/x86/decode.c | 14 +++++
tools/objtool/check.c | 100 ++++++++++++++++++++++++++++++++++++----
tools/objtool/check.h | 2
tools/objtool/elf.h | 1
6 files changed, 138 insertions(+), 13 deletions(-)
--- a/include/linux/frame.h
+++ b/include/linux/frame.h
@@ -28,10 +28,38 @@ asm (".pushsection .discard.nonstd_frame
".byte 0\n\t"
".popsection\n\t");
+/*
+ * This macro marks functions as UACCESS-safe, that is, it is safe to call from an
+ * UACCESS enabled region (typically user_access_begin() /
+ * user_access_end()).
+ *
+ * These functions in turn will only call UACCESS-safe functions themselves (which
+ * precludes tracing, including __fentry__ and scheduling, including
+ * preempt_enable).
+ *
+ * UACCESS-safe functions will obviously also not change UACCESS themselves.
+ */
+#define UACCESS_SAFE(func) \
+ asm (".pushsection .discard.uaccess_safe_strtab, \"S\", @3\n\t" \
+ "999: .ascii \"" #func "\"\n\t" \
+ " .byte 0\n\t" \
+ ".popsection\n\t" \
+ ".pushsection .discard.uaccess_safe\n\t" \
+ ".long 999b - .\n\t" \
+ ".popsection")
+
+/*
+ * SHT_STRTAB(@3) sections should start with a \0 byte such that the 0 offset
+ * encodes the NULL string.
+ */
+asm(".pushsection .discard.uaccess_safe_strtab, \"S\", @3\n\t"
+ ".byte 0\n\t"
+ ".popsection\n\t");
+
#else /* !CONFIG_STACK_VALIDATION */
#define STACK_FRAME_NON_STANDARD(func)
+#define AC_SAFE(func)
#endif /* CONFIG_STACK_VALIDATION */
-
#endif /* _LINUX_FRAME_H */
--- a/tools/objtool/arch.h
+++ b/tools/objtool/arch.h
@@ -33,7 +33,9 @@
#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_OTHER 13
#define INSN_LAST INSN_OTHER
enum op_dest_type {
--- a/tools/objtool/arch/x86/decode.c
+++ b/tools/objtool/arch/x86/decode.c
@@ -369,7 +369,19 @@ int arch_decode_instruction(struct elf *
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;
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -444,6 +444,40 @@ static void add_ignores(struct objtool_f
return;
}
+static void add_uaccess_safe(struct objtool_file *file)
+{
+ struct section *strtab_sec, *sec;
+ struct symbol *func;
+ struct rela *rela;
+ const char *name;
+
+ sec = find_section_by_name(file->elf, ".rela.discard.uaccess_safe");
+ if (!sec)
+ return;
+
+ strtab_sec = find_section_by_name(file->elf, ".discard.uaccess_safe_strtab");
+ if (!strtab_sec) {
+ WARN("missing uaccess_safe strtab");
+ return;
+ }
+
+ 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;
+ }
+
+ name = elf_strptr(file->elf->elf, strtab_sec->idx, rela->addend);
+ func = find_symbol_by_name(file->elf, name);
+ if (!func)
+ continue;
+
+ func->uaccess_safe = true;
+ }
+
+ return;
+}
+
/*
* 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.
@@ -1232,6 +1266,7 @@ static int decode_sections(struct objtoo
return ret;
add_ignores(file);
+ add_uaccess_safe(file);
ret = add_nospec_ignores(file);
if (ret)
@@ -1792,6 +1827,22 @@ static bool insn_state_match(struct inst
return false;
}
+static inline bool insn_dest_uaccess_safe(struct instruction *insn)
+{
+ if (!insn->call_dest)
+ return false;
+
+ return insn->call_dest->uaccess_safe;
+}
+
+static inline const char *insn_dest_name(struct instruction *insn)
+{
+ if (!insn->call_dest)
+ return "{dynamic}";
+
+ return insn->call_dest->name;
+}
+
/*
* Follow the branch starting at the given instruction, and recursively follow
* any other branches (jumps). Meanwhile, track the frame pointer state at
@@ -1896,6 +1947,9 @@ static int validate_branch(struct objtoo
switch (insn->type) {
case INSN_RETURN:
+ if (state.uaccess)
+ WARN_FUNC("return with UACCESS enabled", sec, insn->offset);
+
if (func && has_modified_stack_frame(&state)) {
WARN_FUNC("return with modified stack frame",
sec, insn->offset);
@@ -1911,17 +1965,24 @@ static int validate_branch(struct objtoo
return 0;
case INSN_CALL:
- if (is_fentry_call(insn))
- break;
+ case INSN_CALL_DYNAMIC:
+ if ((state.uaccess_safe || state.uaccess) &&
+ !insn_dest_uaccess_safe(insn)) {
+ WARN_FUNC("call to %s() with UACCESS enabled",
+ sec, insn->offset, insn_dest_name(insn));
+ }
- ret = dead_end_function(file, insn->call_dest);
- if (ret == 1)
- return 0;
- if (ret == -1)
- return 1;
+ if (insn->type == INSN_CALL) {
+ if (is_fentry_call(insn))
+ break;
+
+ ret = dead_end_function(file, insn->call_dest);
+ if (ret == 1)
+ return 0;
+ if (ret == -1)
+ return 1;
+ }
- /* fallthrough */
- case INSN_CALL_DYNAMIC:
if (!no_fp && func && !has_valid_stack_frame(&state)) {
WARN_FUNC("call without frame pointer save/setup",
sec, insn->offset);
@@ -1974,6 +2035,25 @@ static int validate_branch(struct objtoo
break;
+ case INSN_STAC:
+ if (state.uaccess_safe || state.uaccess)
+ WARN_FUNC("recursive UACCESS enable", sec, insn->offset);
+
+ state.uaccess = true;
+ break;
+
+ case INSN_CLAC:
+ if (!state.uaccess && insn->func)
+ WARN_FUNC("redundant UACCESS disable", sec, insn->offset);
+
+ if (state.uaccess_safe) {
+ WARN_FUNC("UACCESS-safe disables UACCESS", sec, insn->offset);
+ break;
+ }
+
+ state.uaccess = false;
+ break;
+
default:
break;
}
@@ -2135,6 +2215,8 @@ static int validate_functions(struct obj
if (!insn || insn->ignore)
continue;
+ state.uaccess_safe = func->uaccess_safe;
+
ret = validate_branch(file, insn, state);
warnings += ret;
}
--- 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, uaccess, uaccess_safe;
int drap_reg, drap_offset;
struct cfi_reg vals[CFI_NUM_REGS];
};
--- 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 uaccess_safe;
};
struct rela {
On Mon, Feb 25, 2019 at 4:53 AM Peter Zijlstra <[email protected]> wrote:
>
> It is important that UACCESS regions are as small as possible;
> furthermore the UACCESS state is not scheduled, so doing anything that
> might directly call into the scheduler will cause random code to be
> ran with UACCESS enabled.
>
> Teach objtool too track UACCESS state and warn about any CALL made
> while UACCESS is enabled. This very much includes the __fentry__()
> tracing calls and __preempt_schedule() calls.
>
> Note that exceptions _do_ save/restore the UACCESS state, and therefore
> they can drive preemption. This also means that all exception handlers
> must have an otherwise dedundant UACCESS disable instruction;
> therefore ignore this warning for !STT_FUNC code (exception handlers
> are not normal functions).
>
> It also provides a UACCESS_SAFE() annotation which allows explicit
> annotation. This is meant to be used for future things like:
> unsafe_copy_{to,from}_user().
>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> ---
> include/linux/frame.h | 30 +++++++++++-
> tools/objtool/arch.h | 4 +
> tools/objtool/arch/x86/decode.c | 14 +++++
> tools/objtool/check.c | 100 ++++++++++++++++++++++++++++++++++++----
> tools/objtool/check.h | 2
> tools/objtool/elf.h | 1
> 6 files changed, 138 insertions(+), 13 deletions(-)
>
> --- a/include/linux/frame.h
> +++ b/include/linux/frame.h
> @@ -28,10 +28,38 @@ asm (".pushsection .discard.nonstd_frame
> ".byte 0\n\t"
> ".popsection\n\t");
>
> +/*
> + * This macro marks functions as UACCESS-safe, that is, it is safe to call from an
> + * UACCESS enabled region (typically user_access_begin() /
> + * user_access_end()).
> + *
> + * These functions in turn will only call UACCESS-safe functions themselves (which
> + * precludes tracing, including __fentry__ and scheduling, including
> + * preempt_enable).
> + *
> + * UACCESS-safe functions will obviously also not change UACCESS themselves.
> + */
> +#define UACCESS_SAFE(func) \
> + asm (".pushsection .discard.uaccess_safe_strtab, \"S\", @3\n\t" \
> + "999: .ascii \"" #func "\"\n\t" \
> + " .byte 0\n\t" \
> + ".popsection\n\t" \
> + ".pushsection .discard.uaccess_safe\n\t" \
> + ".long 999b - .\n\t" \
> + ".popsection")
Minor nit: using big numbers like 999: like this always bugs me. It
relies on there not being a macro nested inside or outside this that
uses the same number. My general preference is to do something like
.Ldescription_\@ instead.
Otherwise this looks conceptually good :)
On Mon, Feb 25, 2019 at 07:53:01AM -0800, Andy Lutomirski wrote:
> On Mon, Feb 25, 2019 at 4:53 AM Peter Zijlstra <[email protected]> wrote:
> > +#define UACCESS_SAFE(func) \
> > + asm (".pushsection .discard.uaccess_safe_strtab, \"S\", @3\n\t" \
> > + "999: .ascii \"" #func "\"\n\t" \
> > + " .byte 0\n\t" \
> > + ".popsection\n\t" \
> > + ".pushsection .discard.uaccess_safe\n\t" \
> > + ".long 999b - .\n\t" \
> > + ".popsection")
>
> Minor nit: using big numbers like 999: like this always bugs me. It
> relies on there not being a macro nested inside or outside this that
> uses the same number. My general preference is to do something like
> .Ldescription_\@ instead.
>
> Otherwise this looks conceptually good :)
I seem to remember here being an issue with the \@ thing. Notably we're
not using it in nospsec-branch.h.
I'll see if I can dig up why we decided not to use it there.
On Mon, Feb 25, 2019 at 05:12:34PM +0100, Peter Zijlstra wrote:
> On Mon, Feb 25, 2019 at 07:53:01AM -0800, Andy Lutomirski wrote:
> > On Mon, Feb 25, 2019 at 4:53 AM Peter Zijlstra <[email protected]> wrote:
>
> > > +#define UACCESS_SAFE(func) \
> > > + asm (".pushsection .discard.uaccess_safe_strtab, \"S\", @3\n\t" \
> > > + "999: .ascii \"" #func "\"\n\t" \
> > > + " .byte 0\n\t" \
> > > + ".popsection\n\t" \
> > > + ".pushsection .discard.uaccess_safe\n\t" \
> > > + ".long 999b - .\n\t" \
> > > + ".popsection")
> >
> > Minor nit: using big numbers like 999: like this always bugs me. It
> > relies on there not being a macro nested inside or outside this that
> > uses the same number. My general preference is to do something like
> > .Ldescription_\@ instead.
> >
> > Otherwise this looks conceptually good :)
>
> I seem to remember here being an issue with the \@ thing. Notably we're
> not using it in nospsec-branch.h.
>
> I'll see if I can dig up why we decided not to use it there.
Ah, so the thing we need for inline-asm is %=, but Linus didn't like it:
https://lkml.kernel.org/r/[email protected]
Also, I think at the time we didn't use it because backporting efforts.
I can't really find what happened between:
https://lkml.kernel.org/r/[email protected]
and
https://lkml.kernel.org/r/[email protected]
But the %= thing went away.
On Mon, Feb 25, 2019 at 9:15 AM Peter Zijlstra <[email protected]> wrote:
>
> Ah, so the thing we need for inline-asm is %=, but Linus didn't like it:
Well, to be fair, I also didn't like the random big numbers.
I think that inline asm that has local labels should just use simple
enumeration, and always use the direction suffix to make those local
labels be unambiguous.
Now, there are cases where we may not be able to do that, because we
have internal macros within the asm that _also_ wants to do its own
numbers. This isn't such a case.
For that case, you might actually want to use a fancy "get me a unique
number and prefix", but it should be seen as the exception rather than
the first solution, because the code starts looking really random.
Linus
On Mon, Feb 25, 2019 at 06:15:31PM +0100, Peter Zijlstra wrote:
> Also, I think at the time we didn't use it because backporting efforts.
This part may have been my fault. I spread a vicious rumor (based on
reading docs instead of code) that old versions of GCC don't support
'%='. But it's actually been around since GCC 2.8.
--
Josh
On Mon, Feb 25, 2019 at 01:43:35PM +0100, Peter Zijlstra wrote:
> It is important that UACCESS regions are as small as possible;
> furthermore the UACCESS state is not scheduled, so doing anything that
> might directly call into the scheduler will cause random code to be
> ran with UACCESS enabled.
>
> Teach objtool too track UACCESS state and warn about any CALL made
> while UACCESS is enabled. This very much includes the __fentry__()
> tracing calls and __preempt_schedule() calls.
>
> Note that exceptions _do_ save/restore the UACCESS state, and therefore
> they can drive preemption. This also means that all exception handlers
> must have an otherwise dedundant UACCESS disable instruction;
> therefore ignore this warning for !STT_FUNC code (exception handlers
> are not normal functions).
>
> It also provides a UACCESS_SAFE() annotation which allows explicit
> annotation. This is meant to be used for future things like:
> unsafe_copy_{to,from}_user().
>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
So KASAN is wildly unhappy..
I can't actually find any definitions of those functions, so I can't
very well mark the safe, even if we wanted to.
---
>> arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x59: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x6a: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x7b: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x8f: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0xa3: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0xb4: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0xc5: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0xdc: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0xf0: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x101: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x112: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x123: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x134: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x145: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x156: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x167: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x177: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x192: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x1bc: call to __asan_store8_noabort() with UACCESS enabled
>> arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x1f2: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x212: call to __asan_store8_noabort() with UACCESS enabled
>> arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x1d8: call to force_valid_ss.isra.0() with UACCESS enabled
>> arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x3f: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x50: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x61: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x75: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x89: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x9a: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0xab: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0xbc: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0xcd: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0xde: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0xef: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x100: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x111: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x122: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x133: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x143: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x157: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x173: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x18e: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x1a8: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x1c2: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x1ee: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: setup_sigcontext()+0x217: call to __asan_load8_noabort() with UACCESS enabled
>> arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x618: call to __asan_load8_noabort() with UACCESS enabled
>> arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x630: call to __asan_loadN_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x681: call to __asan_load8_noabort() with UACCESS enabled
>> arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x6a0: call to __asan_load4_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x6bd: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x7cb: call to __asan_load8_noabort() with UACCESS enabled
>> arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x6dd: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x6f0: call to __asan_store8_noabort() with UACCESS enabled
>> arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x707: call to __asan_store4_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x2c6: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x2e3: call to __asan_loadN_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x334: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x352: call to __asan_load4_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x374: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x49d: call to __asan_load8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x393: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x3aa: call to __asan_store8_noabort() with UACCESS enabled
arch/x86/kernel/signal.o: warning: objtool: do_signal()+0x3c1: call to __asan_store4_noabort() with UACCESS enabled
On Wed, Feb 27, 2019 at 05:17:58PM +0300, Andrey Ryabinin wrote:
>
>
> On 2/27/19 5:08 PM, Peter Zijlstra wrote:
> > I can't actually find any definitions of those functions, so I can't
> > very well mark the safe, even if we wanted to.
> >
>
> They are macro-generated. Use 'git grep DEFINE_ASAN'
Ah, indeed! I'll go have a look.
On 2/27/19 5:08 PM, Peter Zijlstra wrote:
> On Mon, Feb 25, 2019 at 01:43:35PM +0100, Peter Zijlstra wrote:
>> It is important that UACCESS regions are as small as possible;
>> furthermore the UACCESS state is not scheduled, so doing anything that
>> might directly call into the scheduler will cause random code to be
>> ran with UACCESS enabled.
>>
>> Teach objtool too track UACCESS state and warn about any CALL made
>> while UACCESS is enabled. This very much includes the __fentry__()
>> tracing calls and __preempt_schedule() calls.
>>
>> Note that exceptions _do_ save/restore the UACCESS state, and therefore
>> they can drive preemption. This also means that all exception handlers
>> must have an otherwise dedundant UACCESS disable instruction;
>> therefore ignore this warning for !STT_FUNC code (exception handlers
>> are not normal functions).
>>
>> It also provides a UACCESS_SAFE() annotation which allows explicit
>> annotation. This is meant to be used for future things like:
>> unsafe_copy_{to,from}_user().
>>
>> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
>
> So KASAN is wildly unhappy..
>
> I can't actually find any definitions of those functions, so I can't
> very well mark the safe, even if we wanted to.
>
They are macro-generated. Use 'git grep DEFINE_ASAN'
> ---
>
>>> arch/x86/kernel/signal.o: warning: objtool: restore_sigcontext()+0x59: call to __asan_store8_noabort() with UACCESS enabled
On Wed, Feb 27, 2019 at 03:26:23PM +0100, Peter Zijlstra wrote:
> On Wed, Feb 27, 2019 at 05:17:58PM +0300, Andrey Ryabinin wrote:
> >
> >
> > On 2/27/19 5:08 PM, Peter Zijlstra wrote:
>
> > > I can't actually find any definitions of those functions, so I can't
> > > very well mark the safe, even if we wanted to.
> > >
> >
> > They are macro-generated. Use 'git grep DEFINE_ASAN'
>
> Ah, indeed! I'll go have a look.
Urgh, kasan_report() is definitely unsafe. Now, admitedly we should
'never' hit that, but it does leave us up a creek without a paddle.
Not sure what to do here; best I can come up with atm. is kill SMAP on
KASAN builds.
On Wed, Feb 27, 2019 at 3:33 PM Peter Zijlstra <[email protected]> wrote:
>
> On Wed, Feb 27, 2019 at 03:26:23PM +0100, Peter Zijlstra wrote:
> > On Wed, Feb 27, 2019 at 05:17:58PM +0300, Andrey Ryabinin wrote:
> > >
> > >
> > > On 2/27/19 5:08 PM, Peter Zijlstra wrote:
> >
> > > > I can't actually find any definitions of those functions, so I can't
> > > > very well mark the safe, even if we wanted to.
> > > >
> > >
> > > They are macro-generated. Use 'git grep DEFINE_ASAN'
> >
> > Ah, indeed! I'll go have a look.
>
> Urgh, kasan_report() is definitely unsafe. Now, admitedly we should
> 'never' hit that, but it does leave us up a creek without a paddle.
>
> Not sure what to do here; best I can come up with atm. is kill SMAP on
> KASAN builds.
If SMAP detects additional bugs, then it would be pity to disable it
with KASAN (detect bugs in production but not during testing).
You mentioned that exception save/restore the UACCESS state. Is it
possible to do the same in kasan_report? At the very least we need to
survive report printing, what happens after that does not matter much
(we've corrupted memory by now anyway).
On Wed, Feb 27, 2019 at 6:08 AM Peter Zijlstra <[email protected]> wrote:
>
> So KASAN is wildly unhappy..
Yeah, well, with KASAN you definitely end up doing lots and lots for
calls for just regular memory accesses.
Which we obviously need to do for most uaccess cases.
I think you should just say "ok, kasan reporting will possibly run with AC on".
Again, having AC on isn't fatal. It just makes the window where you
can incorrectly access user space through a wild pointer bigger.
So the whole "run with AC on" thing isn't a big deal, apart from the
scheduling case.
And we know how to fix the scheduling case by just doing the
save/restore thing..
Linus
On Wed, Feb 27, 2019 at 04:40:28PM +0100, Dmitry Vyukov wrote:
> On Wed, Feb 27, 2019 at 3:33 PM Peter Zijlstra <[email protected]> wrote:
> > Urgh, kasan_report() is definitely unsafe. Now, admitedly we should
> > 'never' hit that, but it does leave us up a creek without a paddle.
> If SMAP detects additional bugs, then it would be pity to disable it
> with KASAN (detect bugs in production but not during testing).
>
> You mentioned that exception save/restore the UACCESS state. Is it
> possible to do the same in kasan_report? At the very least we need to
> survive report printing, what happens after that does not matter much
> (we've corrupted memory by now anyway).
Ideally we'll put all of kasan_report() in an exception, much like we do
for WARN. But there's a distinct lack of arch hooks there to play with.
I suppose I can try and create some.
On top of that we'll have to mark these __asan functions with notrace.
Maybe a little something horrible like so... completely untested.
---
arch/x86/include/asm/bug.h | 28 ++++++++++++++--------------
arch/x86/include/asm/kasan.h | 11 +++++++++++
include/asm-generic/bug.h | 1 +
include/linux/kasan.h | 6 +++++-
lib/bug.c | 8 +++++++-
mm/kasan/generic.c | 4 ++--
mm/kasan/kasan.h | 2 +-
mm/kasan/report.c | 2 +-
8 files changed, 42 insertions(+), 20 deletions(-)
diff --git a/arch/x86/include/asm/bug.h b/arch/x86/include/asm/bug.h
index 6804d6642767..ead3e6ad4eb6 100644
--- a/arch/x86/include/asm/bug.h
+++ b/arch/x86/include/asm/bug.h
@@ -30,33 +30,33 @@
#ifdef CONFIG_DEBUG_BUGVERBOSE
-#define _BUG_FLAGS(ins, flags) \
+#define _BUG_FLAGS(ins, flags, ...) \
do { \
asm volatile("1:\t" ins "\n" \
".pushsection __bug_table,\"aw\"\n" \
- "2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n" \
- "\t" __BUG_REL(%c0) "\t# bug_entry::file\n" \
- "\t.word %c1" "\t# bug_entry::line\n" \
- "\t.word %c2" "\t# bug_entry::flags\n" \
- "\t.org 2b+%c3\n" \
+ "2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n" \
+ "\t" __BUG_REL(%c[file]) "\t# bug_entry::file\n" \
+ "\t.word %c[line]" "\t# bug_entry::line\n" \
+ "\t.word %c[flags]" "\t# bug_entry::flags\n" \
+ "\t.org 2b+%c[size]\n" \
".popsection" \
- : : "i" (__FILE__), "i" (__LINE__), \
- "i" (flags), \
- "i" (sizeof(struct bug_entry))); \
+ : : [file] "i" (__FILE__), [line] "i" (__LINE__), \
+ [flags] "i" (flags), \
+ [size] "i" (sizeof(struct bug_entry)), ##__VA_ARGS__); \
} while (0)
#else /* !CONFIG_DEBUG_BUGVERBOSE */
-#define _BUG_FLAGS(ins, flags) \
+#define _BUG_FLAGS(ins, flags, ...) \
do { \
asm volatile("1:\t" ins "\n" \
".pushsection __bug_table,\"aw\"\n" \
"2:\t" __BUG_REL(1b) "\t# bug_entry::bug_addr\n" \
- "\t.word %c0" "\t# bug_entry::flags\n" \
- "\t.org 2b+%c1\n" \
+ "\t.word %c[flags]" "\t# bug_entry::flags\n" \
+ "\t.org 2b+%c[size]\n" \
".popsection" \
- : : "i" (flags), \
- "i" (sizeof(struct bug_entry))); \
+ : : [flags] "i" (flags), \
+ [size] "i" (sizeof(struct bug_entry)), ##__VA_ARGS__); \
} while (0)
#endif /* CONFIG_DEBUG_BUGVERBOSE */
diff --git a/arch/x86/include/asm/kasan.h b/arch/x86/include/asm/kasan.h
index 13e70da38bed..b7d563965b4d 100644
--- a/arch/x86/include/asm/kasan.h
+++ b/arch/x86/include/asm/kasan.h
@@ -2,6 +2,8 @@
#ifndef _ASM_X86_KASAN_H
#define _ASM_X86_KASAN_H
+#include <asm/bug.h>
+
#include <linux/const.h>
#define KASAN_SHADOW_OFFSET _AC(CONFIG_KASAN_SHADOW_OFFSET, UL)
#define KASAN_SHADOW_SCALE_SHIFT 3
@@ -26,8 +28,17 @@
#ifndef __ASSEMBLY__
#ifdef CONFIG_KASAN
+
void __init kasan_early_init(void);
void __init kasan_init(void);
+
+static __always_inline void kasan_report(unsigned long addr, size_t size, bool is_write, unsigned long ip)
+{
+ unsigned long rdi = addr, rsi = size, rdx = is_write, rdx = ip;
+
+ _BUG_FLAGS(ASM_UD2, BUGFLAG_KASAN, "D" (rdi), "S" (rsi), "d" (rdx), "c" (rcx));
+}
+
#else
static inline void kasan_early_init(void) { }
static inline void kasan_init(void) { }
diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 0e9bd9c83870..6c829e80ea2a 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -10,6 +10,7 @@
#define BUGFLAG_WARNING (1 << 0)
#define BUGFLAG_ONCE (1 << 1)
#define BUGFLAG_DONE (1 << 2)
+#define BUGFLAG_KASAN (1 << 3)
#define BUGFLAG_TAINT(taint) ((taint) << 8)
#define BUG_GET_TAINT(bug) ((bug)->flags >> 8)
#endif
diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index b40ea104dd36..0ab50faedc9e 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -177,7 +177,7 @@ void kasan_init_tags(void);
void *kasan_reset_tag(const void *addr);
-void kasan_report(unsigned long addr, size_t size,
+void __kasan_report(unsigned long addr, size_t size,
bool is_write, unsigned long ip);
#else /* CONFIG_KASAN_SW_TAGS */
@@ -191,4 +191,8 @@ static inline void *kasan_reset_tag(const void *addr)
#endif /* CONFIG_KASAN_SW_TAGS */
+#ifndef kasan_report
+#define kasan_report(addr, size, is_write, ip) __kasan_report(addr, size, is_write, ip)
+#endif
+
#endif /* LINUX_KASAN_H */
diff --git a/lib/bug.c b/lib/bug.c
index 1077366f496b..c9c005a35b76 100644
--- a/lib/bug.c
+++ b/lib/bug.c
@@ -144,7 +144,7 @@ enum bug_trap_type report_bug(unsigned long bugaddr, struct pt_regs *regs)
{
struct bug_entry *bug;
const char *file;
- unsigned line, warning, once, done;
+ unsigned line, warning, once, done, kasan;
if (!is_valid_bugaddr(bugaddr))
return BUG_TRAP_TYPE_NONE;
@@ -167,6 +167,7 @@ enum bug_trap_type report_bug(unsigned long bugaddr, struct pt_regs *regs)
line = bug->line;
#endif
warning = (bug->flags & BUGFLAG_WARNING) != 0;
+ kasan = (bug->flags & BUGFLAG_KASAN) != 0;
once = (bug->flags & BUGFLAG_ONCE) != 0;
done = (bug->flags & BUGFLAG_DONE) != 0;
@@ -188,6 +189,11 @@ enum bug_trap_type report_bug(unsigned long bugaddr, struct pt_regs *regs)
return BUG_TRAP_TYPE_WARN;
}
+ if (kasan) {
+ __kasan_report(regs->di, regs->si, regs->dx, regs->cx);
+ return BUG_TRAP_TYPE_WARN;
+ }
+
printk(KERN_DEFAULT CUT_HERE);
if (file)
diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
index ccb6207276e3..84be578e2591 100644
--- a/mm/kasan/generic.c
+++ b/mm/kasan/generic.c
@@ -228,7 +228,7 @@ void __asan_unregister_globals(struct kasan_global *globals, size_t size)
EXPORT_SYMBOL(__asan_unregister_globals);
#define DEFINE_ASAN_LOAD_STORE(size) \
- void __asan_load##size(unsigned long addr) \
+ notrace void __asan_load##size(unsigned long addr) \
{ \
check_memory_region_inline(addr, size, false, _RET_IP_);\
} \
@@ -236,7 +236,7 @@ EXPORT_SYMBOL(__asan_unregister_globals);
__alias(__asan_load##size) \
void __asan_load##size##_noabort(unsigned long); \
EXPORT_SYMBOL(__asan_load##size##_noabort); \
- void __asan_store##size(unsigned long addr) \
+ notrace void __asan_store##size(unsigned long addr) \
{ \
check_memory_region_inline(addr, size, true, _RET_IP_); \
} \
diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
index ea51b2d898ec..7bc954b4fc2d 100644
--- a/mm/kasan/kasan.h
+++ b/mm/kasan/kasan.h
@@ -130,7 +130,7 @@ void check_memory_region(unsigned long addr, size_t size, bool write,
void *find_first_bad_addr(void *addr, size_t size);
const char *get_bug_type(struct kasan_access_info *info);
-void kasan_report(unsigned long addr, size_t size,
+void __kasan_report(unsigned long addr, size_t size,
bool is_write, unsigned long ip);
void kasan_report_invalid_free(void *object, unsigned long ip);
diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index ca9418fe9232..1d5050659388 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -281,7 +281,7 @@ void kasan_report_invalid_free(void *object, unsigned long ip)
end_report(&flags);
}
-void kasan_report(unsigned long addr, size_t size,
+void __kasan_report(unsigned long addr, size_t size,
bool is_write, unsigned long ip)
{
struct kasan_access_info info;
On Wed, Feb 27, 2019 at 08:18:21AM -0800, Linus Torvalds wrote:
> So the whole "run with AC on" thing isn't a big deal, apart from the
> scheduling case.
>
> And we know how to fix the scheduling case by just doing the
> save/restore thing..
Right, but I was hoping to avoid having to do that; and I think we can
do that if we push all of kasan_report into an exception, like that
'patch' I just posted.
If I'm not mistaken the regular kasan house-keeping crud just prods at
the shadow memory with simple code and should be perfectly safe.
On Wed, Feb 27, 2019 at 9:30 AM Peter Zijlstra <[email protected]> wrote:
>
> Right, but I was hoping to avoid having to do that; and I think we can
> do that if we push all of kasan_report into an exception, like that
> 'patch' I just posted.
>
> If I'm not mistaken the regular kasan house-keeping crud just prods at
> the shadow memory with simple code and should be perfectly safe.
Fair enough. I do agree it would be nice to not have the save/restore
simply because "it cannot matter" and we have the static tool to prove
that.
Linus
On Wed, Feb 27, 2019 at 06:28:16PM +0100, Peter Zijlstra wrote:
> On Wed, Feb 27, 2019 at 04:40:28PM +0100, Dmitry Vyukov wrote:
> > On Wed, Feb 27, 2019 at 3:33 PM Peter Zijlstra <[email protected]> wrote:
>
> > > Urgh, kasan_report() is definitely unsafe. Now, admitedly we should
> > > 'never' hit that, but it does leave us up a creek without a paddle.
>
> > If SMAP detects additional bugs, then it would be pity to disable it
> > with KASAN (detect bugs in production but not during testing).
> >
> > You mentioned that exception save/restore the UACCESS state. Is it
> > possible to do the same in kasan_report? At the very least we need to
> > survive report printing, what happens after that does not matter much
> > (we've corrupted memory by now anyway).
>
> Ideally we'll put all of kasan_report() in an exception, much like we do
> for WARN. But there's a distinct lack of arch hooks there to play with.
> I suppose I can try and create some.
>
> On top of that we'll have to mark these __asan functions with notrace.
>
> Maybe a little something horrible like so... completely untested.
OK, I got that to compile; the next problem is:
../include/linux/kasan.h:90:1: error: built-in function ‘__asan_loadN_noabort’ must be directly called
UACCESS_SAFE(__asan_loadN_noabort);
Which doesn't make any sense; since we actually generated that symbol,
it clearly is not built-in. What gives?
On Thu, Feb 28, 2019 at 10:59 AM Dmitry Vyukov <[email protected]> wrote:
>
> On Thu, Feb 28, 2019 at 10:40 AM Peter Zijlstra <[email protected]> wrote:
> >
> > On Wed, Feb 27, 2019 at 06:28:16PM +0100, Peter Zijlstra wrote:
> > > On Wed, Feb 27, 2019 at 04:40:28PM +0100, Dmitry Vyukov wrote:
> > > > On Wed, Feb 27, 2019 at 3:33 PM Peter Zijlstra <[email protected]> wrote:
> > >
> > > > > Urgh, kasan_report() is definitely unsafe. Now, admitedly we should
> > > > > 'never' hit that, but it does leave us up a creek without a paddle.
> > >
> > > > If SMAP detects additional bugs, then it would be pity to disable it
> > > > with KASAN (detect bugs in production but not during testing).
> > > >
> > > > You mentioned that exception save/restore the UACCESS state. Is it
> > > > possible to do the same in kasan_report? At the very least we need to
> > > > survive report printing, what happens after that does not matter much
> > > > (we've corrupted memory by now anyway).
> > >
> > > Ideally we'll put all of kasan_report() in an exception, much like we do
> > > for WARN. But there's a distinct lack of arch hooks there to play with.
> > > I suppose I can try and create some.
> > >
> > > On top of that we'll have to mark these __asan functions with notrace.
> > >
> > > Maybe a little something horrible like so... completely untested.
> >
> > OK, I got that to compile; the next problem is:
> >
> > ../include/linux/kasan.h:90:1: error: built-in function ‘__asan_loadN_noabort’ must be directly called
> > UACCESS_SAFE(__asan_loadN_noabort);
> >
> > Which doesn't make any sense; since we actually generated that symbol,
> > it clearly is not built-in. What gives?
>
> I guess this warning originated for user-space where programmer does
> not define them and does not generally know about them and signature
> is not a public contract for user. And then for kernel it just stayed
> the same because not doing this warning would require somebody to
> proactively think about this potential difference and add an
> additional code to skip this check and even then it wasn't obvious why
> one will want to do this with these functions. So that's where we are
> now.
Maybe asm directive will help to trick the compiler?
On Thu, Feb 28, 2019 at 10:40 AM Peter Zijlstra <[email protected]> wrote:
>
> On Wed, Feb 27, 2019 at 06:28:16PM +0100, Peter Zijlstra wrote:
> > On Wed, Feb 27, 2019 at 04:40:28PM +0100, Dmitry Vyukov wrote:
> > > On Wed, Feb 27, 2019 at 3:33 PM Peter Zijlstra <[email protected]> wrote:
> >
> > > > Urgh, kasan_report() is definitely unsafe. Now, admitedly we should
> > > > 'never' hit that, but it does leave us up a creek without a paddle.
> >
> > > If SMAP detects additional bugs, then it would be pity to disable it
> > > with KASAN (detect bugs in production but not during testing).
> > >
> > > You mentioned that exception save/restore the UACCESS state. Is it
> > > possible to do the same in kasan_report? At the very least we need to
> > > survive report printing, what happens after that does not matter much
> > > (we've corrupted memory by now anyway).
> >
> > Ideally we'll put all of kasan_report() in an exception, much like we do
> > for WARN. But there's a distinct lack of arch hooks there to play with.
> > I suppose I can try and create some.
> >
> > On top of that we'll have to mark these __asan functions with notrace.
> >
> > Maybe a little something horrible like so... completely untested.
>
> OK, I got that to compile; the next problem is:
>
> ../include/linux/kasan.h:90:1: error: built-in function ‘__asan_loadN_noabort’ must be directly called
> UACCESS_SAFE(__asan_loadN_noabort);
>
> Which doesn't make any sense; since we actually generated that symbol,
> it clearly is not built-in. What gives?
I guess this warning originated for user-space where programmer does
not define them and does not generally know about them and signature
is not a public contract for user. And then for kernel it just stayed
the same because not doing this warning would require somebody to
proactively think about this potential difference and add an
additional code to skip this check and even then it wasn't obvious why
one will want to do this with these functions. So that's where we are
now.
On Thu, Feb 28, 2019 at 11:05:10AM +0100, Dmitry Vyukov wrote:
> On Thu, Feb 28, 2019 at 10:59 AM Dmitry Vyukov <[email protected]> wrote:
> > I guess this warning originated for user-space where programmer does
> > not define them and does not generally know about them and signature
> > is not a public contract for user. And then for kernel it just stayed
> > the same because not doing this warning would require somebody to
> > proactively think about this potential difference and add an
> > additional code to skip this check and even then it wasn't obvious why
> > one will want to do this with these functions. So that's where we are
> > now.
>
> Maybe asm directive will help to trick the compiler?
So I went back and forth on the annotation; and we're back to the same
we use for STACK_FRAME_NON_STANDARD() because that forces GCC to
actually generate the symbol.
Without that GCC IPA will go and wreck things by either making the
entire symbol go away or generating partial functions.
I'm currently doing a hard-coded list of names in objtool for this :/
But I'm having trouble with that alias crud.