Messages in this thread | | | Date | Thu, 2 Jul 2020 10:05:10 +0200 | From | Peter Zijlstra <> | Subject | Re: objtool clac/stac handling change.. |
| |
On Wed, Jul 01, 2020 at 07:00:41PM -0500, Josh Poimboeuf wrote: > On Wed, Jul 01, 2020 at 02:02:42PM -0700, Linus Torvalds wrote: > > So the objtool rule might be: > > > > - in a STAC region, no exception handlers at all except for that > > ex_handler_uaccess case > > > > - and that case will clear AC if it triggers. > > > > and maybe such an objtool check would show some case where I'm wrong, > > and we do some MSR read other other fault thing within a STAC region. > > That _sounds_ wrong to me, but maybe we have reason to do so that I > > just can't think or right now? > > Here's an attempt at implementing this, in case anybody wants to play > with it. Usual disclaimers apply...
Looks about right, two niggles below.
> @@ -2335,6 +2340,35 @@ static void fill_alternative_cfi(struct objtool_file *file, struct instruction * > } > } > > +static int handle_stac(struct symbol *func, struct instruction *insn, > + struct insn_state *state) > +{ > + if (state->uaccess) { > + WARN_FUNC("recursive UACCESS enable", insn->sec, insn->offset); > + return -1; > + } > + > + state->uaccess = true; > + return 0; > +} > + > +static int handle_clac(struct symbol *func, struct instruction *insn, > + struct insn_state *state) > +{ > + if (!state->uaccess && func) { > + WARN_FUNC("redundant UACCESS disable", insn->sec, insn->offset); > + return -1; > + } > + > + if (func_uaccess_safe(func) && !state->uaccess_stack) { > + WARN_FUNC("UACCESS-safe disables UACCESS", insn->sec, insn->offset); > + return -1; > + } > + > + state->uaccess = false; > + return 0; > +}
For both these we return -1 on error and then all callers convert it to 1. So why not have this return 1 and pass any !0 value through?
> /* > * Follow the branch starting at the given instruction, and recursively follow > * any other branches (jumps). Meanwhile, track the frame pointer state at > @@ -2393,6 +2427,17 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, > if (alt->skip_orig) > skip_orig = true; > > + if (alt->exception) { > + if (!alt->uaccess && state.uaccess) { > + WARN_FUNC("non-user-access exception with uaccess enabled", > + sec, insn->offset); > + return 1; > + }
This is Linus' new rule that AC code should not get any exceptions except ex_handler_uaccess.
> + > + if (alt->uaccess && handle_clac(func, insn, &state)) > + return 1;
And this is ex_handler_uaccess() mucking with regs->flags, right? Might want a comment.
> + } > + > ret = validate_branch(file, func, alt->insn, state); > if (ret) { > if (backtrace)
| |