Messages in this thread | | | Subject | Re: [PATCH V3 6/9] objtool: Report inconsistent stack changes in alternative | From | Julien Thierry <> | Date | Tue, 14 Apr 2020 16:35:56 +0100 |
| |
Hi Alexandre,
On 4/14/20 11:36 AM, Alexandre Chartre wrote: > To allow a valid stack unwinding, an alternative should have code > where the same stack changes happens at the same places as in the > original code. Add a check in objtool to validate that stack changes > in alternative are effectively consitent with the original code. > > Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com> > --- > tools/objtool/check.c | 155 ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 155 insertions(+) > > diff --git a/tools/objtool/check.c b/tools/objtool/check.c > index 0574ce8e232d..9488a9c7be24 100644 > --- a/tools/objtool/check.c > +++ b/tools/objtool/check.c > @@ -2724,6 +2724,156 @@ static int validate_reachable_instructions(struct objtool_file *file) > return 0; > } > > +static int validate_alternative_state(struct objtool_file *file, > + struct instruction *first, > + struct instruction *prev, > + struct instruction *current, > + bool include_current) > +{ > + struct instruction *alt_insn, *alt_first; > + unsigned long roff_prev, roff_curr, roff; > + int count, err = 0, err_alt, alt_num; > + struct alternative *alt; > + const char *err_str; > + > + /* > + * Check that instructions in alternatives between the corresponding > + * previous and current instructions, have the same stack state. > + */ > + > + /* use relative offset to find corresponding alt instructions */ > + roff_prev = prev->offset - first->offset; > + roff_curr = current->offset - first->offset; > + alt_num = 0; > + > + list_for_each_entry(alt, &first->alts, list) { > + if (!alt->insn->alt_group) > + continue; > + > + alt_num++; > + alt_first = alt->insn; > + count = 0; > + err_alt = 0;
Unless I'm missing something, err_alt is wither 1 or 0, so perhaps a boolean would be more appropriate.
> + > + for (alt_insn = alt_first; > + alt_insn && alt_insn->alt_group == alt_first->alt_group; > + alt_insn = next_insn_same_sec(file, alt_insn)) { > + > + roff = alt_insn->offset - alt_first->offset; > + if (roff < roff_prev) > + continue; > + > + if (roff > roff_curr || > + (roff == roff_curr && !include_current)) > + break; > + > + count++; > + /* > + * The first instruction we check must be aligned with > + * the corresponding "prev" instruction because stack > + * change should happen at the same offset. > + */ > + if (count == 1 && roff != roff_prev) { > + err_str = "misaligned alternative state change"; > + err_alt++; > + } > + > + if (!err_alt && memcmp(&prev->state, &alt_insn->state, > + sizeof(struct insn_state))) {
In insn_state_match(), not the whole of the insn_state is taken into account. In particular, uaccess_stack, uaccess, df and end are not compared. Also, stack_size (but maybe that should be included in insn_state_match() ) and vals are not checked.
Is there a reason we'd check those for alternatives but not in the normal case? And does your alternative validation work with uaccess check?
> + err_str = "invalid alternative state"; > + err_alt++; > + } > + > + /* > + * On error, report the error and stop checking > + * this alternative but continue checking other > + * alternatives. > + */ > + if (err_alt) { > + if (!err) { > + WARN_FUNC("error in alternative", > + first->sec, first->offset); > + } > + WARN_FUNC("in alternative %d", > + alt_first->sec, alt_first->offset, > + alt_num); > + WARN_FUNC("%s", alt_insn->sec, alt_insn->offset, > + err_str); > + > + err += err_alt; > + break; > + } > + } > + } > + > + return err; > +} > + > +static int validate_alternative(struct objtool_file *file) > +{ > + struct instruction *first, *prev, *next, *insn; > + bool in_alternative; > + int err; > + > + err = 0; > + first = prev = NULL; > + in_alternative = false; > + for_each_insn(file, insn) { > + if (insn->ignore || !insn->alt_group || insn->ignore_alts) > + continue; > + > + if (!in_alternative) { > + if (list_empty(&insn->alts)) > + continue; > + > + /* > + * Setup variables to start the processing of a > + * new alternative. > + */ > + first = insn; > + prev = insn; > + err = 0; > + in_alternative = true; > + > + } else if (!err && memcmp(&prev->state, &insn->state, > + sizeof(struct insn_state))) { > + /* > + * The stack state has changed and no error was > + * reported for this alternative. Check that the > + * stack state is the same in all alternatives > + * between the last check and up to this instruction. > + * > + * Once we have an error, stop checking the > + * alternative because once the stack state is > + * inconsistent, it will likely be inconsistent > + * for other instructions as well. > + */ > + err = validate_alternative_state(file, first, > + prev, insn, false); > + prev = insn; > + } > + > + /* > + * If the next instruction is in the same alternative then > + * continue with processing this alternative. Otherwise > + * that's the end of this alternative so check there is no > + * stack state changes in remaining instructions (if no > + * error was reported yet). > + */ > + next = list_next_entry(insn, list); > + if (next && !next->ignore && > + next->alt_group == first->alt_group) > + continue; > + > + if (!err) > + err = validate_alternative_state(file, first, > + prev, insn, true); > + in_alternative = false; > + } > + > + return 0; > +} > + > static struct objtool_file file; > > int check(const char *_objname, bool orc) > @@ -2769,6 +2919,11 @@ int check(const char *_objname, bool orc) > goto out; > warnings += ret; > > + ret = validate_alternative(&file); > + if (ret < 0) > + goto out; > + warnings += ret; > + > if (!warnings) { > ret = validate_reachable_instructions(&file); > if (ret < 0) >
Cheers,
-- Julien Thierry
| |