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__()
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 redundant UACCESS disable instruction;
therefore ignore this warning for !STT_FUNC code (exception handlers
are not normal functions).
XXX: users hard-coded list of uaccess-safe functions because I've
failed to come up with a sensible annotation and the list should be
fairly static.
XXX: are we sure we want __memset marked AC-safe?
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
---
scripts/Makefile.build | 3
tools/objtool/arch.h | 4
tools/objtool/arch/x86/decode.c | 14 +++
tools/objtool/builtin-check.c | 3
tools/objtool/builtin.h | 2
tools/objtool/check.c | 164 +++++++++++++++++++++++++++++++++++++---
tools/objtool/check.h | 2
tools/objtool/elf.h | 1
tools/objtool/special.c | 10 ++
9 files changed, 188 insertions(+), 15 deletions(-)
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -223,6 +223,9 @@ endif
ifdef CONFIG_RETPOLINE
objtool_args += --retpoline
endif
+ifdef CONFIG_X86_SMAP
+ objtool_args += --uaccess
+endif
# 'OBJECT_FILES_NON_STANDARD := y': skip objtool checking for a directory
# 'OBJECT_FILES_NON_STANDARD_foo.o := 'y': skip objtool checking for a file
--- 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/builtin-check.c
+++ b/tools/objtool/builtin-check.c
@@ -29,7 +29,7 @@
#include "builtin.h"
#include "check.h"
-bool no_fp, no_unreachable, retpoline, module, backtrace;
+bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess;
static const char * const check_usage[] = {
"objtool check [<options>] file.o",
@@ -42,6 +42,7 @@ const struct option check_options[] = {
OPT_BOOLEAN('r', "retpoline", &retpoline, "Validate retpoline assumptions"),
OPT_BOOLEAN('m', "module", &module, "Indicates the object will be part of a kernel module"),
OPT_BOOLEAN('b', "backtrace", &backtrace, "unwind on error"),
+ OPT_BOOLEAN('a', "uaccess", &uaccess, "enable uaccess checking"),
OPT_END(),
};
--- a/tools/objtool/builtin.h
+++ b/tools/objtool/builtin.h
@@ -20,7 +20,7 @@
#include <subcmd/parse-options.h>
extern const struct option check_options[];
-extern bool no_fp, no_unreachable, retpoline, module, backtrace;
+extern bool no_fp, no_unreachable, retpoline, module, backtrace, uaccess;
extern int cmd_check(int argc, const char **argv);
extern int cmd_orc(int argc, const char **argv);
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -442,6 +442,81 @@ static void add_ignores(struct objtool_f
}
}
+static const char *uaccess_safe_builtin[] = {
+ /* KASAN */
+ "memset_orig", /* XXX why not memset_erms */
+ "__memset",
+ "kasan_poison_shadow",
+ "kasan_unpoison_shadow",
+ "__asan_poison_stack_memory",
+ "__asan_unpoison_stack_memory",
+ "kasan_report",
+ "check_memory_region",
+ /* KASAN out-of-line */
+ "__asan_loadN_noabort",
+ "__asan_load1_noabort",
+ "__asan_load2_noabort",
+ "__asan_load4_noabort",
+ "__asan_load8_noabort",
+ "__asan_load16_noabort",
+ "__asan_storeN_noabort",
+ "__asan_store1_noabort",
+ "__asan_store2_noabort",
+ "__asan_store4_noabort",
+ "__asan_store8_noabort",
+ "__asan_store16_noabort",
+ /* KASAN in-line */
+ "__asan_report_load_n_noabort",
+ "__asan_report_load1_noabort",
+ "__asan_report_load2_noabort",
+ "__asan_report_load4_noabort",
+ "__asan_report_load8_noabort",
+ "__asan_report_load16_noabort",
+ "__asan_report_store_n_noabort",
+ "__asan_report_store1_noabort",
+ "__asan_report_store2_noabort",
+ "__asan_report_store4_noabort",
+ "__asan_report_store8_noabort",
+ "__asan_report_store16_noabort",
+ /* KCOV */
+ "write_comp_data",
+ "__sanitizer_cov_trace_pc",
+ "__sanitizer_cov_trace_const_cmp1",
+ "__sanitizer_cov_trace_const_cmp2",
+ "__sanitizer_cov_trace_const_cmp4",
+ "__sanitizer_cov_trace_const_cmp8",
+ "__sanitizer_cov_trace_cmp1",
+ "__sanitizer_cov_trace_cmp2",
+ "__sanitizer_cov_trace_cmp4",
+ "__sanitizer_cov_trace_cmp8",
+ /* UBSAN */
+ "ubsan_type_mismatch_common",
+ "__ubsan_handle_type_mismatch",
+ "__ubsan_handle_type_mismatch_v1",
+ /* misc */
+ "csum_partial_copy_generic",
+ "__memcpy_mcsafe",
+ "ftrace_likely_update", /* CONFIG_TRACE_BRANCH_PROFILING */
+ NULL
+};
+
+static void add_uaccess_safe(struct objtool_file *file)
+{
+ struct symbol *func;
+ const char **name;
+
+ if (!uaccess)
+ return;
+
+ for (name = uaccess_safe_builtin; *name; name++) {
+ func = find_symbol_by_name(file->elf, *name);
+ if (!func)
+ continue;
+
+ func->alias->uaccess_safe = true;
+ }
+}
+
/*
* 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.
@@ -1239,6 +1314,7 @@ static int decode_sections(struct objtoo
return ret;
add_ignores(file);
+ add_uaccess_safe(file);
ret = add_nospec_ignores(file);
if (ret)
@@ -1799,6 +1875,22 @@ static bool insn_state_match(struct inst
return false;
}
+static inline bool func_uaccess_safe(struct symbol *func)
+{
+ if (func)
+ return func->alias->uaccess_safe;
+
+ return false;
+}
+
+static inline const char *insn_dest_name(struct instruction *insn)
+{
+ if (insn->call_dest)
+ return insn->call_dest->name;
+
+ return "{dynamic}";
+}
+
/*
* Follow the branch starting at the given instruction, and recursively follow
* any other branches (jumps). Meanwhile, track the frame pointer state at
@@ -1844,7 +1936,9 @@ static int validate_branch(struct objtoo
if (!insn->hint && !insn_state_match(insn, &state))
return 1;
- return 0;
+ /* If we were here with AC=0, but now have AC=1, go again */
+ if (insn->state.uaccess || !state.uaccess)
+ return 0;
}
if (insn->hint) {
@@ -1914,6 +2008,16 @@ static int validate_branch(struct objtoo
switch (insn->type) {
case INSN_RETURN:
+ if (state.uaccess && !func_uaccess_safe(func)) {
+ WARN_FUNC("return with UACCESS enabled", sec, insn->offset);
+ return 1;
+ }
+
+ if (!state.uaccess && func_uaccess_safe(func)) {
+ WARN_FUNC("return with UACCESS disabled from a UACCESS-safe function", sec, insn->offset);
+ return 1;
+ }
+
if (func && has_modified_stack_frame(&state)) {
WARN_FUNC("return with modified stack frame",
sec, insn->offset);
@@ -1929,17 +2033,32 @@ static int validate_branch(struct objtoo
return 0;
case INSN_CALL:
- if (is_fentry_call(insn))
- break;
+ case INSN_CALL_DYNAMIC:
+do_call:
+ if (state.uaccess && !func_uaccess_safe(insn->call_dest)) {
+ WARN_FUNC("call to %s() with UACCESS enabled",
+ sec, insn->offset, insn_dest_name(insn));
+ return 1;
+ }
- ret = dead_end_function(file, insn->call_dest);
- if (ret == 1)
+ if (insn->type == INSN_JUMP_UNCONDITIONAL ||
+ insn->type == INSN_JUMP_DYNAMIC)
return 0;
- if (ret == -1)
- return 1;
- /* fallthrough */
- case INSN_CALL_DYNAMIC:
+ if (insn->type == INSN_JUMP_CONDITIONAL)
+ break;
+
+ 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;
+ }
+
if (!no_fp && func && !has_valid_stack_frame(&state)) {
WARN_FUNC("call without frame pointer save/setup",
sec, insn->offset);
@@ -1956,6 +2075,8 @@ static int validate_branch(struct objtoo
sec, insn->offset);
return 1;
}
+ goto do_call;
+
} else if (insn->jump_dest &&
(!func || !insn->jump_dest->func ||
insn->jump_dest->func->pfunc == func)) {
@@ -1994,6 +2115,29 @@ static int validate_branch(struct objtoo
break;
+ case INSN_STAC:
+ if (state.uaccess) {
+ WARN_FUNC("recursive UACCESS enable", sec, insn->offset);
+ return 1;
+ }
+
+ state.uaccess = true;
+ break;
+
+ case INSN_CLAC:
+ if (!state.uaccess && insn->func) {
+ WARN_FUNC("redundant UACCESS disable", sec, insn->offset);
+ return 1;
+ }
+
+ if (func_uaccess_safe(func)) {
+ WARN_FUNC("UACCESS-safe disables UACCESS", sec, insn->offset);
+ return 1;
+ }
+
+ state.uaccess = false;
+ break;
+
default:
break;
}
@@ -2157,6 +2301,8 @@ static int validate_functions(struct obj
if (!insn || insn->ignore)
continue;
+ state.uaccess = func->alias->uaccess_safe;
+
ret = validate_branch(file, insn, state);
if (ret && backtrace)
BT_FUNC("<=== (func)", insn);
--- 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;
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, *alias;
+ bool uaccess_safe;
};
struct rela {
--- a/tools/objtool/special.c
+++ b/tools/objtool/special.c
@@ -42,6 +42,7 @@
#define ALT_NEW_LEN_OFFSET 11
#define X86_FEATURE_POPCNT (4*32+23)
+#define X86_FEATURE_SMAP (9*32+20)
struct special_entry {
const char *sec;
@@ -107,8 +108,15 @@ static int get_alt_entry(struct elf *elf
* It has been requested that we don't validate the !POPCNT
* feature path which is a "very very small percentage of
* machines".
+ *
+ * Also, unconditionally enable SMAP; this avoids seeing paths
+ * that pass through the STAC alternative and through the CLAC
+ * NOPs.
+ *
+ * XXX: We could do this for all binary NOP/single-INSN
+ * alternatives.
*/
- if (feature == X86_FEATURE_POPCNT)
+ if (feature == X86_FEATURE_POPCNT || feature == X86_FEATURE_SMAP)
alt->skip_orig = true;
}
On Thu, Mar 7, 2019 at 3:52 AM Peter Zijlstra <[email protected]> wrote:
>
> XXX: are we sure we want __memset marked AC-safe?
It's certainly one of the safer functions to call with AC set, but it
sounds wrong anyway. It's not like it's likely to leak kernel data
(most memset's are with 0, and even the non-zero ones I can't imagine
are sensitive - more like poison values etc).
What's the call site that made you go "just add __memset() to the list"?
Linus
On March 7, 2019 8:33:26 AM PST, Linus Torvalds <[email protected]> wrote:
>On Thu, Mar 7, 2019 at 3:52 AM Peter Zijlstra <[email protected]>
>wrote:
>>
>> XXX: are we sure we want __memset marked AC-safe?
>
>It's certainly one of the safer functions to call with AC set, but it
>sounds wrong anyway. It's not like it's likely to leak kernel data
>(most memset's are with 0, and even the non-zero ones I can't imagine
>are sensitive - more like poison values etc).
>
>What's the call site that made you go "just add __memset() to the
>list"?
>
> Linus
Agreed... it seems fishy at least.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
On Thu, Mar 07, 2019 at 08:33:26AM -0800, Linus Torvalds wrote:
> On Thu, Mar 7, 2019 at 3:52 AM Peter Zijlstra <[email protected]> wrote:
> >
> > XXX: are we sure we want __memset marked AC-safe?
>
> It's certainly one of the safer functions to call with AC set, but it
> sounds wrong anyway. It's not like it's likely to leak kernel data
> (most memset's are with 0, and even the non-zero ones I can't imagine
> are sensitive - more like poison values etc).
>
> What's the call site that made you go "just add __memset() to the list"?
__asan_{,un}poinson_stack_memory()
kasan_{,un}poison_shadow()
__memset()
On Thu, Mar 7, 2019 at 9:41 AM Peter Zijlstra <[email protected]> wrote:
> >
> > What's the call site that made you go "just add __memset() to the list"?
>
> __asan_{,un}poinson_stack_memory()
> kasan_{,un}poison_shadow()
> __memset()
Ugh. I think I almost just agree with your decision to just let that
memset go unchecked.
I'm not saying it's right, but it doesn't seem to be a fight worth fighting.
Again, maybe we could avoid the static checking entirely for the
complex configs, and just make preempt_schedule() not do it for AC
regions.
Because AC vs KASAN in general ends up smelling like "not a fight
worth fighting" to me. You've done a herculean job, but..
Linus
On Thu, Mar 07, 2019 at 09:54:14AM -0800, Linus Torvalds wrote:
> On Thu, Mar 7, 2019 at 9:41 AM Peter Zijlstra <[email protected]> wrote:
> > >
> > > What's the call site that made you go "just add __memset() to the list"?
> >
> > __asan_{,un}poinson_stack_memory()
> > kasan_{,un}poison_shadow()
> > __memset()
>
> Ugh. I think I almost just agree with your decision to just let that
> memset go unchecked.
>
> I'm not saying it's right, but it doesn't seem to be a fight worth fighting.
One think I could do; is add a filter to each function and only allow
__memset from the kasan code, and not from anywhere else.
Another thing I need to look at is why objtool only found memset_orig
(from __memset) but not memset_erms, which if I read the code right, is
a possible alternative there.
> Because AC vs KASAN in general ends up smelling like "not a fight
> worth fighting" to me. You've done a herculean job, but..
I know,.. I've been so close to doing that so many times, but it
seems like defeat, esp. since I'm so close now :-)
On March 7, 2019 10:48:13 AM PST, Peter Zijlstra <[email protected]> wrote:
>On Thu, Mar 07, 2019 at 09:54:14AM -0800, Linus Torvalds wrote:
>> On Thu, Mar 7, 2019 at 9:41 AM Peter Zijlstra <[email protected]>
>wrote:
>> > >
>> > > What's the call site that made you go "just add __memset() to the
>list"?
>> >
>> > __asan_{,un}poinson_stack_memory()
>> > kasan_{,un}poison_shadow()
>> > __memset()
>>
>> Ugh. I think I almost just agree with your decision to just let that
>> memset go unchecked.
>>
>> I'm not saying it's right, but it doesn't seem to be a fight worth
>fighting.
>
>One think I could do; is add a filter to each function and only allow
>__memset from the kasan code, and not from anywhere else.
>
>Another thing I need to look at is why objtool only found memset_orig
>(from __memset) but not memset_erms, which if I read the code right, is
>a possible alternative there.
>
>> Because AC vs KASAN in general ends up smelling like "not a fight
>> worth fighting" to me. You've done a herculean job, but..
>
>I know,.. I've been so close to doing that so many times, but it
>seems like defeat, esp. since I'm so close now :-)
___memset_kasan()?
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
On Thu, Mar 07, 2019 at 07:48:13PM +0100, Peter Zijlstra wrote:
> On Thu, Mar 07, 2019 at 09:54:14AM -0800, Linus Torvalds wrote:
> > On Thu, Mar 7, 2019 at 9:41 AM Peter Zijlstra <[email protected]> wrote:
> > > >
> > > > What's the call site that made you go "just add __memset() to the list"?
> > >
> > > __asan_{,un}poinson_stack_memory()
> > > kasan_{,un}poison_shadow()
> > > __memset()
> >
> > Ugh. I think I almost just agree with your decision to just let that
> > memset go unchecked.
> >
> > I'm not saying it's right, but it doesn't seem to be a fight worth fighting.
>
> One think I could do; is add a filter to each function and only allow
> __memset from the kasan code, and not from anywhere else.
Ah.. how about I feed objtool a text file with all these symbol names;
and I have Makefile compose file that from fragments.
Then only KASAN builds will have memset whitelisted, and any other build
will still flag memset abuse.
Now I only have to figure out how to make Makefile do something like
that :-)
On 3/7/19 8:41 PM, Peter Zijlstra wrote:
> On Thu, Mar 07, 2019 at 08:33:26AM -0800, Linus Torvalds wrote:
>> On Thu, Mar 7, 2019 at 3:52 AM Peter Zijlstra <[email protected]> wrote:
>>>
>>> XXX: are we sure we want __memset marked AC-safe?
>>
>> It's certainly one of the safer functions to call with AC set, but it
>> sounds wrong anyway. It's not like it's likely to leak kernel data
>> (most memset's are with 0, and even the non-zero ones I can't imagine
>> are sensitive - more like poison values etc).
>>
>> What's the call site that made you go "just add __memset() to the list"?
>
> __asan_{,un}poinson_stack_memory()
These two can be called only with CONFIG_KASAN_EXTRA=y which
was removed very recently, so it should be safe to delete these functions.
> kasan_{,un}poison_shadow()
> __memset()
>
>
On Thu, Mar 07, 2019 at 07:48:13PM +0100, Peter Zijlstra wrote:
> Another thing I need to look at is why objtool only found memset_orig
> (from __memset) but not memset_erms, which if I read the code right, is
> a possible alternative there.
Turns out we only look for sibling calls in the original instruction
stream, not in any alternatives; which in general seems like a fair
enough assumption.
On Thu, Mar 07, 2019 at 11:15:42PM +0300, Andrey Ryabinin wrote:
>
>
> On 3/7/19 8:41 PM, Peter Zijlstra wrote:
> > On Thu, Mar 07, 2019 at 08:33:26AM -0800, Linus Torvalds wrote:
> >> On Thu, Mar 7, 2019 at 3:52 AM Peter Zijlstra <[email protected]> wrote:
> >>>
> >>> XXX: are we sure we want __memset marked AC-safe?
> >>
> >> It's certainly one of the safer functions to call with AC set, but it
> >> sounds wrong anyway. It's not like it's likely to leak kernel data
> >> (most memset's are with 0, and even the non-zero ones I can't imagine
> >> are sensitive - more like poison values etc).
> >>
> >> What's the call site that made you go "just add __memset() to the list"?
> >
> > __asan_{,un}poinson_stack_memory()
>
> These two can be called only with CONFIG_KASAN_EXTRA=y which
> was removed very recently, so it should be safe to delete these functions.
Ooh shiny. Clearly my tree still has them; what commit do I need to look
for?
On Thu, Mar 07, 2019 at 09:23:19PM +0100, Peter Zijlstra wrote:
> On Thu, Mar 07, 2019 at 07:48:13PM +0100, Peter Zijlstra wrote:
> > Another thing I need to look at is why objtool only found memset_orig
> > (from __memset) but not memset_erms, which if I read the code right, is
> > a possible alternative there.
>
> Turns out we only look for sibling calls in the original instruction
> stream, not in any alternatives; which in general seems like a fair
> enough assumption.
And while I'm looking at memset_64.S, why are memset_erms and
memset_orig global functions? At the very least they should be local,
and ideally not even functions.
On 3/7/19 11:33 PM, Peter Zijlstra wrote:
> On Thu, Mar 07, 2019 at 11:15:42PM +0300, Andrey Ryabinin wrote:
>>
>>
>> On 3/7/19 8:41 PM, Peter Zijlstra wrote:
>>> On Thu, Mar 07, 2019 at 08:33:26AM -0800, Linus Torvalds wrote:
>>>> On Thu, Mar 7, 2019 at 3:52 AM Peter Zijlstra <[email protected]> wrote:
>>>>>
>>>>> XXX: are we sure we want __memset marked AC-safe?
>>>>
>>>> It's certainly one of the safer functions to call with AC set, but it
>>>> sounds wrong anyway. It's not like it's likely to leak kernel data
>>>> (most memset's are with 0, and even the non-zero ones I can't imagine
>>>> are sensitive - more like poison values etc).
>>>>
>>>> What's the call site that made you go "just add __memset() to the list"?
>>>
>>> __asan_{,un}poinson_stack_memory()
>>
>> These two can be called only with CONFIG_KASAN_EXTRA=y which
>> was removed very recently, so it should be safe to delete these functions.
>
> Ooh shiny. Clearly my tree still has them; what commit do I need to look
> for?
>
commit 7771bdbbfd3d6f204631b6fd9e1bbc30cd15918e
On Thu, Mar 07, 2019 at 09:33:18PM +0100, Peter Zijlstra wrote:
> Ooh shiny. Clearly my tree still has them; what commit do I need to look
> for?
30be39d1e1dc ("kasan: remove use after scope bugs detection.")
On Thu, Mar 07, 2019 at 08:03:13PM +0100, Peter Zijlstra wrote:
> On Thu, Mar 07, 2019 at 07:48:13PM +0100, Peter Zijlstra wrote:
> > On Thu, Mar 07, 2019 at 09:54:14AM -0800, Linus Torvalds wrote:
> > > On Thu, Mar 7, 2019 at 9:41 AM Peter Zijlstra <[email protected]> wrote:
> > > > >
> > > > > What's the call site that made you go "just add __memset() to the list"?
> > > >
> > > > __asan_{,un}poinson_stack_memory()
> > > > kasan_{,un}poison_shadow()
> > > > __memset()
> > >
> > > Ugh. I think I almost just agree with your decision to just let that
> > > memset go unchecked.
> > >
> > > I'm not saying it's right, but it doesn't seem to be a fight worth fighting.
> >
> > One think I could do; is add a filter to each function and only allow
> > __memset from the kasan code, and not from anywhere else.
>
> Ah.. how about I feed objtool a text file with all these symbol names;
> and I have Makefile compose file that from fragments.
>
> Then only KASAN builds will have memset whitelisted, and any other build
> will still flag memset abuse.
>
> Now I only have to figure out how to make Makefile do something like
> that :-)
Instead of adding all those additional moving parts, I would much rather
either:
a) have kasan call a special whitelisted version of memset (like hpa
suggested); or
b) just don't use the objtool --uaccess flag for KASAN builds.
--
Josh
On Fri, Mar 08, 2019 at 09:01:11AM -0600, Josh Poimboeuf wrote:
> On Thu, Mar 07, 2019 at 08:03:13PM +0100, Peter Zijlstra wrote:
> > Ah.. how about I feed objtool a text file with all these symbol names;
> > and I have Makefile compose file that from fragments.
> >
> > Then only KASAN builds will have memset whitelisted, and any other build
> > will still flag memset abuse.
> >
> > Now I only have to figure out how to make Makefile do something like
> > that :-)
>
> Instead of adding all those additional moving parts, I would much rather
> either:
The problem went away. That is, the problematic part of KASAN just got
removed in this merge window.
Also; I just about had hpa's __memset_kasan implemented when I got that
email.
On Thu, Mar 07, 2019 at 09:40:21PM +0100, Peter Zijlstra wrote:
> On Thu, Mar 07, 2019 at 09:23:19PM +0100, Peter Zijlstra wrote:
> > On Thu, Mar 07, 2019 at 07:48:13PM +0100, Peter Zijlstra wrote:
> > > Another thing I need to look at is why objtool only found memset_orig
> > > (from __memset) but not memset_erms, which if I read the code right, is
> > > a possible alternative there.
> >
> > Turns out we only look for sibling calls in the original instruction
> > stream, not in any alternatives; which in general seems like a fair
> > enough assumption.
>
> And while I'm looking at memset_64.S, why are memset_erms and
> memset_orig global functions? At the very least they should be local,
> and ideally not even functions.
I think the only benefit is that they would show up better on stack
traces, but that could also be solved by just making them local labels
inside memset. Which is what I think they should be.
The general rule is that ENDPROC is only used for callable functions, so
yeah, I think the current setup isn't ideal, and also prevents objtool
from properly doing the AC analysis as you pointed out earlier.
--
Josh
On Fri, Mar 08, 2019 at 09:07:03AM -0600, Josh Poimboeuf wrote:
> On Thu, Mar 07, 2019 at 09:40:21PM +0100, Peter Zijlstra wrote:
> > On Thu, Mar 07, 2019 at 09:23:19PM +0100, Peter Zijlstra wrote:
> > > On Thu, Mar 07, 2019 at 07:48:13PM +0100, Peter Zijlstra wrote:
> > > > Another thing I need to look at is why objtool only found memset_orig
> > > > (from __memset) but not memset_erms, which if I read the code right, is
> > > > a possible alternative there.
> > >
> > > Turns out we only look for sibling calls in the original instruction
> > > stream, not in any alternatives; which in general seems like a fair
> > > enough assumption.
> >
> > And while I'm looking at memset_64.S, why are memset_erms and
> > memset_orig global functions? At the very least they should be local,
> > and ideally not even functions.
>
> I think the only benefit is that they would show up better on stack
> traces, but that could also be solved by just making them local labels
> inside memset. Which is what I think they should be.
Boris wanted to use alternative_call_2, just like copy_user_generic().
Which makes more sense to me still.
On Thu, Mar 07, 2019 at 12:45:29PM +0100, Peter Zijlstra wrote:
> --- 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;
> +
> + }
Style nit, no need for all those brackets and newlines.
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -442,6 +442,81 @@ static void add_ignores(struct objtool_f
> }
> }
>
> +static const char *uaccess_safe_builtin[] = {
> + /* KASAN */
A short comment would be good here, something describing why a function
might be added to the list.
> + "memset_orig", /* XXX why not memset_erms */
> + "__memset",
> + "kasan_poison_shadow",
> + "kasan_unpoison_shadow",
> + "__asan_poison_stack_memory",
> + "__asan_unpoison_stack_memory",
> + "kasan_report",
> + "check_memory_region",
> + /* KASAN out-of-line */
> + "__asan_loadN_noabort",
> + "__asan_load1_noabort",
> + "__asan_load2_noabort",
> + "__asan_load4_noabort",
> + "__asan_load8_noabort",
> + "__asan_load16_noabort",
> + "__asan_storeN_noabort",
> + "__asan_store1_noabort",
> + "__asan_store2_noabort",
> + "__asan_store4_noabort",
> + "__asan_store8_noabort",
> + "__asan_store16_noabort",
> + /* KASAN in-line */
> + "__asan_report_load_n_noabort",
> + "__asan_report_load1_noabort",
> + "__asan_report_load2_noabort",
> + "__asan_report_load4_noabort",
> + "__asan_report_load8_noabort",
> + "__asan_report_load16_noabort",
> + "__asan_report_store_n_noabort",
> + "__asan_report_store1_noabort",
> + "__asan_report_store2_noabort",
> + "__asan_report_store4_noabort",
> + "__asan_report_store8_noabort",
> + "__asan_report_store16_noabort",
> + /* KCOV */
> + "write_comp_data",
> + "__sanitizer_cov_trace_pc",
> + "__sanitizer_cov_trace_const_cmp1",
> + "__sanitizer_cov_trace_const_cmp2",
> + "__sanitizer_cov_trace_const_cmp4",
> + "__sanitizer_cov_trace_const_cmp8",
> + "__sanitizer_cov_trace_cmp1",
> + "__sanitizer_cov_trace_cmp2",
> + "__sanitizer_cov_trace_cmp4",
> + "__sanitizer_cov_trace_cmp8",
> + /* UBSAN */
> + "ubsan_type_mismatch_common",
> + "__ubsan_handle_type_mismatch",
> + "__ubsan_handle_type_mismatch_v1",
> + /* misc */
> + "csum_partial_copy_generic",
> + "__memcpy_mcsafe",
> + "ftrace_likely_update", /* CONFIG_TRACE_BRANCH_PROFILING */
> + NULL
> +};
> +
> +static void add_uaccess_safe(struct objtool_file *file)
> +{
> + struct symbol *func;
> + const char **name;
> +
> + if (!uaccess)
> + return;
> +
> + for (name = uaccess_safe_builtin; *name; name++) {
> + func = find_symbol_by_name(file->elf, *name);
> + if (!func)
> + continue;
This won't work if the function name changes due to IPA optimizations.
I assume these are all global functions so maybe it's fine?
> @@ -1914,6 +2008,16 @@ static int validate_branch(struct objtoo
> switch (insn->type) {
>
> case INSN_RETURN:
> + if (state.uaccess && !func_uaccess_safe(func)) {
> + WARN_FUNC("return with UACCESS enabled", sec, insn->offset);
> + return 1;
> + }
> +
> + if (!state.uaccess && func_uaccess_safe(func)) {
> + WARN_FUNC("return with UACCESS disabled from a UACCESS-safe function", sec, insn->offset);
> + return 1;
> + }
> +
> if (func && has_modified_stack_frame(&state)) {
> WARN_FUNC("return with modified stack frame",
> sec, insn->offset);
> @@ -1929,17 +2033,32 @@ static int validate_branch(struct objtoo
> return 0;
>
> case INSN_CALL:
> - if (is_fentry_call(insn))
> - break;
> + case INSN_CALL_DYNAMIC:
> +do_call:
> + if (state.uaccess && !func_uaccess_safe(insn->call_dest)) {
> + WARN_FUNC("call to %s() with UACCESS enabled",
> + sec, insn->offset, insn_dest_name(insn));
> + return 1;
> + }
>
> - ret = dead_end_function(file, insn->call_dest);
> - if (ret == 1)
> + if (insn->type == INSN_JUMP_UNCONDITIONAL ||
> + insn->type == INSN_JUMP_DYNAMIC)
> return 0;
> - if (ret == -1)
> - return 1;
>
> - /* fallthrough */
> - case INSN_CALL_DYNAMIC:
> + if (insn->type == INSN_JUMP_CONDITIONAL)
> + break;
> +
> + 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;
> + }
> +
> if (!no_fp && func && !has_valid_stack_frame(&state)) {
> WARN_FUNC("call without frame pointer save/setup",
> sec, insn->offset);
> @@ -1956,6 +2075,8 @@ static int validate_branch(struct objtoo
> sec, insn->offset);
> return 1;
> }
> + goto do_call;
> +
These gotos make my head spin. Again I would much prefer a small amount
of code duplication over this.
> +++ b/tools/objtool/special.c
> @@ -42,6 +42,7 @@
> #define ALT_NEW_LEN_OFFSET 11
>
> #define X86_FEATURE_POPCNT (4*32+23)
> +#define X86_FEATURE_SMAP (9*32+20)
>
> struct special_entry {
> const char *sec;
> @@ -107,8 +108,15 @@ static int get_alt_entry(struct elf *elf
> * It has been requested that we don't validate the !POPCNT
> * feature path which is a "very very small percentage of
> * machines".
> + *
> + * Also, unconditionally enable SMAP; this avoids seeing paths
> + * that pass through the STAC alternative and through the CLAC
> + * NOPs.
Why is this a problem?
> + *
> + * XXX: We could do this for all binary NOP/single-INSN
> + * alternatives.
Same question here.
> */
> - if (feature == X86_FEATURE_POPCNT)
> + if (feature == X86_FEATURE_POPCNT || feature == X86_FEATURE_SMAP)
> alt->skip_orig = true;
> }
>
>
>
--
Josh
On Fri, Mar 08, 2019 at 03:02:09PM -0600, Josh Poimboeuf wrote:
> > +static const char *uaccess_safe_builtin[] = {
> > + /* KASAN */
>
> A short comment would be good here, something describing why a function
> might be added to the list.
There is; but I'm thinking it might be too short?
> > + "memset_orig", /* XXX why not memset_erms */
> > + "__memset",
> > + "kasan_poison_shadow",
> > + "kasan_unpoison_shadow",
> > + "__asan_poison_stack_memory",
> > + "__asan_unpoison_stack_memory",
Those are gone.
> > + "kasan_report",
That is the main kasan_report function, and is for, as the comment says:
kasan :-)
> > + "check_memory_region",
for __asan_{load,store}N
> > + /* KASAN out-of-line */
> > + "__asan_loadN_noabort",
> > + "__asan_load1_noabort",
> > + "__asan_load2_noabort",
> > + "__asan_load4_noabort",
> > + "__asan_load8_noabort",
> > + "__asan_load16_noabort",
> > + "__asan_storeN_noabort",
> > + "__asan_store1_noabort",
> > + "__asan_store2_noabort",
> > + "__asan_store4_noabort",
> > + "__asan_store8_noabort",
> > + "__asan_store16_noabort",
These, are, again as the comment suggests, the out-of-line KASAN ABI
calls.
> > + /* KASAN in-line */
> > + "__asan_report_load_n_noabort",
> > + "__asan_report_load1_noabort",
> > + "__asan_report_load2_noabort",
> > + "__asan_report_load4_noabort",
> > + "__asan_report_load8_noabort",
> > + "__asan_report_load16_noabort",
> > + "__asan_report_store_n_noabort",
> > + "__asan_report_store1_noabort",
> > + "__asan_report_store2_noabort",
> > + "__asan_report_store4_noabort",
> > + "__asan_report_store8_noabort",
> > + "__asan_report_store16_noabort",
The in-line KASAN ABI
Also, can I just say that {load,store}N vs {load,store}_n bugs the
hell out of me?
> > + /* KCOV */
> > + "write_comp_data",
the logger function
> > + "__sanitizer_cov_trace_pc",
> > + "__sanitizer_cov_trace_const_cmp1",
> > + "__sanitizer_cov_trace_const_cmp2",
> > + "__sanitizer_cov_trace_const_cmp4",
> > + "__sanitizer_cov_trace_const_cmp8",
> > + "__sanitizer_cov_trace_cmp1",
> > + "__sanitizer_cov_trace_cmp2",
> > + "__sanitizer_cov_trace_cmp4",
> > + "__sanitizer_cov_trace_cmp8",
KCOV ABI
> > + /* UBSAN */
> > + "ubsan_type_mismatch_common",
implementation
> > + "__ubsan_handle_type_mismatch",
> > + "__ubsan_handle_type_mismatch_v1",
UBSAN ABI
> > + /* misc */
> > + "csum_partial_copy_generic",
> > + "__memcpy_mcsafe",
> > + "ftrace_likely_update", /* CONFIG_TRACE_BRANCH_PROFILING */
> > + NULL
> > +};
> > + func = find_symbol_by_name(file->elf, *name);
>
> This won't work if the function name changes due to IPA optimizations.
> I assume these are all global functions so maybe it's fine?
With one or two exceptions, yep.
> These gotos make my head spin. Again I would much prefer a small amount
> of code duplication over this.
I didn't think the code was that bad once you see the end result, but
sure, I can try something else.
> > +++ b/tools/objtool/special.c
> > @@ -42,6 +42,7 @@
> > #define ALT_NEW_LEN_OFFSET 11
> >
> > #define X86_FEATURE_POPCNT (4*32+23)
> > +#define X86_FEATURE_SMAP (9*32+20)
> >
> > struct special_entry {
> > const char *sec;
> > @@ -107,8 +108,15 @@ static int get_alt_entry(struct elf *elf
> > * It has been requested that we don't validate the !POPCNT
> > * feature path which is a "very very small percentage of
> > * machines".
> > + *
> > + * Also, unconditionally enable SMAP; this avoids seeing paths
> > + * that pass through the STAC alternative and through the CLAC
> > + * NOPs.
>
> Why is this a problem?
'obvious' violation?
STAC; .... RET; # an AC=1 leak
.... CLAC; # spurious CLAC
If we do the STAC we must also do the CLAC. If we don't do the STAC we
must also not do the CLAC.
> > + *
> > + * XXX: We could do this for all binary NOP/single-INSN
> > + * alternatives.
>
> Same question here.
In general, validating NOPs isn't too interesting, so all NOP/INSN
binary alternatives could be forced on.
> > */
> > - if (feature == X86_FEATURE_POPCNT)
> > + if (feature == X86_FEATURE_POPCNT || feature == X86_FEATURE_SMAP)
> > alt->skip_orig = true;
> > }
I've actually changed this to depend on --uaccess, when set we force on
FEATURE_SMAP, otherwise we force off.
On Fri, Mar 08, 2019 at 10:31:56PM +0100, Peter Zijlstra wrote:
> On Fri, Mar 08, 2019 at 03:02:09PM -0600, Josh Poimboeuf wrote:
> > > +static const char *uaccess_safe_builtin[] = {
> > > + /* KASAN */
> >
> > A short comment would be good here, something describing why a function
> > might be added to the list.
>
> There is; but I'm thinking it might be too short?
I actually just meant a comment above uaccess_safe_builtin describing
what the purpose of the list is and what the expectations are for the
listed functions. i.e. these are functions which are allowed to be
called with the AC flag on, and they should not clear it unless they're
saving/restoring.
> > > +++ b/tools/objtool/special.c
> > > @@ -42,6 +42,7 @@
> > > #define ALT_NEW_LEN_OFFSET 11
> > >
> > > #define X86_FEATURE_POPCNT (4*32+23)
> > > +#define X86_FEATURE_SMAP (9*32+20)
> > >
> > > struct special_entry {
> > > const char *sec;
> > > @@ -107,8 +108,15 @@ static int get_alt_entry(struct elf *elf
> > > * It has been requested that we don't validate the !POPCNT
> > > * feature path which is a "very very small percentage of
> > > * machines".
> > > + *
> > > + * Also, unconditionally enable SMAP; this avoids seeing paths
> > > + * that pass through the STAC alternative and through the CLAC
> > > + * NOPs.
> >
> > Why is this a problem?
>
> 'obvious' violation?
>
> STAC; .... RET; # an AC=1 leak
>
> .... CLAC; # spurious CLAC
>
> If we do the STAC we must also do the CLAC. If we don't do the STAC we
> must also not do the CLAC.
Makes sense now, can you add that last sentence to the paragraph?
> > > + *
> > > + * XXX: We could do this for all binary NOP/single-INSN
> > > + * alternatives.
> >
> > Same question here.
>
> In general, validating NOPs isn't too interesting, so all NOP/INSN
> binary alternatives could be forced on.
Right, but it doesn't sound like there's any real benefit to adding
extra logic.
> > > */
> > > - if (feature == X86_FEATURE_POPCNT)
> > > + if (feature == X86_FEATURE_POPCNT || feature == X86_FEATURE_SMAP)
> > > alt->skip_orig = true;
> > > }
>
> I've actually changed this to depend on --uaccess, when set we force on
> FEATURE_SMAP, otherwise we force off.
Ok.
--
Josh
On Fri, Mar 08, 2019 at 03:02:09PM -0600, Josh Poimboeuf wrote:
> These gotos make my head spin. Again I would much prefer a small amount
> of code duplication over this.
something like so then?
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1888,6 +1888,23 @@ static inline const char *insn_dest_name
return "{dynamic}";
}
+static int validate_call(struct instruction *insn, struct insn_state *state)
+{
+ if (state->uaccess && !func_uaccess_safe(insn->call_dest)) {
+ WARN_FUNC("call to %s() with UACCESS enabled",
+ insn->sec, insn->offset, insn_dest_name(insn));
+ return 1;
+ }
+
+ if (state->df) {
+ WARN_FUNC("call to %s() with DF set",
+ insn->sec, insn->offset, insn_dest_name(insn));
+ return 1;
+ }
+
+ return 0;
+}
+
/*
* Follow the branch starting at the given instruction, and recursively follow
* any other branches (jumps). Meanwhile, track the frame pointer state at
@@ -2036,25 +2053,9 @@ static int validate_branch(struct objtoo
case INSN_CALL:
case INSN_CALL_DYNAMIC:
-do_call:
- if (state.uaccess && !func_uaccess_safe(insn->call_dest)) {
- WARN_FUNC("call to %s() with UACCESS enabled",
- sec, insn->offset, insn_dest_name(insn));
- return 1;
- }
-
- if (state.df) {
- WARN_FUNC("call to %s() with DF set",
- sec, insn->offset, insn_dest_name(insn));
- return 1;
- }
-
- if (insn->type == INSN_JUMP_UNCONDITIONAL ||
- insn->type == INSN_JUMP_DYNAMIC)
- return 0;
-
- if (insn->type == INSN_JUMP_CONDITIONAL)
- break;
+ ret = validate_call(insn, &state);
+ if (ret)
+ return ret;
if (insn->type == INSN_CALL) {
if (is_fentry_call(insn))
@@ -2077,13 +2078,15 @@ static int validate_branch(struct objtoo
case INSN_JUMP_CONDITIONAL:
case INSN_JUMP_UNCONDITIONAL:
if (func && !insn->jump_dest) {
-do_sibling_call:
+ /* sibling call */
if (has_modified_stack_frame(&state)) {
WARN_FUNC("sibling call from callable instruction with modified stack frame",
sec, insn->offset);
return 1;
}
- goto do_call;
+ ret = validate_call(insn, &state);
+ if (ret)
+ return ret;
} else if (insn->jump_dest &&
(!func || !insn->jump_dest->func ||
@@ -2104,8 +2107,17 @@ static int validate_branch(struct objtoo
break;
case INSN_JUMP_DYNAMIC:
- if (func && list_empty(&insn->alts))
- goto do_sibling_call;
+ if (func && list_empty(&insn->alts)) {
+ /* sibling call */
+ if (has_modified_stack_frame(&state)) {
+ WARN_FUNC("sibling call from callable instruction with modified stack frame",
+ sec, insn->offset);
+ return 1;
+ }
+ ret = validate_call(insn, &state);
+ if (ret)
+ return ret;
+ }
return 0;
On Sun, Mar 10, 2019 at 02:10:46PM +0100, Peter Zijlstra wrote:
> On Fri, Mar 08, 2019 at 03:02:09PM -0600, Josh Poimboeuf wrote:
>
> > These gotos make my head spin. Again I would much prefer a small amount
> > of code duplication over this.
>
> something like so then?
Yeah, that looks a lot nicer to me. Thanks.
>
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -1888,6 +1888,23 @@ static inline const char *insn_dest_name
> return "{dynamic}";
> }
>
> +static int validate_call(struct instruction *insn, struct insn_state *state)
> +{
> + if (state->uaccess && !func_uaccess_safe(insn->call_dest)) {
> + WARN_FUNC("call to %s() with UACCESS enabled",
> + insn->sec, insn->offset, insn_dest_name(insn));
> + return 1;
> + }
> +
> + if (state->df) {
> + WARN_FUNC("call to %s() with DF set",
> + insn->sec, insn->offset, insn_dest_name(insn));
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> /*
> * Follow the branch starting at the given instruction, and recursively follow
> * any other branches (jumps). Meanwhile, track the frame pointer state at
> @@ -2036,25 +2053,9 @@ static int validate_branch(struct objtoo
>
> case INSN_CALL:
> case INSN_CALL_DYNAMIC:
> -do_call:
> - if (state.uaccess && !func_uaccess_safe(insn->call_dest)) {
> - WARN_FUNC("call to %s() with UACCESS enabled",
> - sec, insn->offset, insn_dest_name(insn));
> - return 1;
> - }
> -
> - if (state.df) {
> - WARN_FUNC("call to %s() with DF set",
> - sec, insn->offset, insn_dest_name(insn));
> - return 1;
> - }
> -
> - if (insn->type == INSN_JUMP_UNCONDITIONAL ||
> - insn->type == INSN_JUMP_DYNAMIC)
> - return 0;
> -
> - if (insn->type == INSN_JUMP_CONDITIONAL)
> - break;
> + ret = validate_call(insn, &state);
> + if (ret)
> + return ret;
>
> if (insn->type == INSN_CALL) {
> if (is_fentry_call(insn))
> @@ -2077,13 +2078,15 @@ static int validate_branch(struct objtoo
> case INSN_JUMP_CONDITIONAL:
> case INSN_JUMP_UNCONDITIONAL:
> if (func && !insn->jump_dest) {
> -do_sibling_call:
> + /* sibling call */
> if (has_modified_stack_frame(&state)) {
> WARN_FUNC("sibling call from callable instruction with modified stack frame",
> sec, insn->offset);
> return 1;
> }
> - goto do_call;
> + ret = validate_call(insn, &state);
> + if (ret)
> + return ret;
>
> } else if (insn->jump_dest &&
> (!func || !insn->jump_dest->func ||
> @@ -2104,8 +2107,17 @@ static int validate_branch(struct objtoo
> break;
>
> case INSN_JUMP_DYNAMIC:
> - if (func && list_empty(&insn->alts))
> - goto do_sibling_call;
> + if (func && list_empty(&insn->alts)) {
> + /* sibling call */
> + if (has_modified_stack_frame(&state)) {
> + WARN_FUNC("sibling call from callable instruction with modified stack frame",
> + sec, insn->offset);
> + return 1;
> + }
> + ret = validate_call(insn, &state);
> + if (ret)
> + return ret;
> + }
>
> return 0;
>
>
--
Josh