| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <bc4bfade-6080-72da-5181-b97cd21076ff@redhat.com>
Date: Tue, 14 Apr 2020 16:35:56 +0100
From: Julien Thierry <jthierry@...hat.com>
To: Alexandre Chartre <alexandre.chartre@...cle.com>, x86@...nel.org
Cc: linux-kernel@...r.kernel.org, jpoimboe@...hat.com,
peterz@...radead.org, tglx@...utronix.de
Subject: Re: [PATCH V3 6/9] objtool: Report inconsistent stack changes in
alternative
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@...cle.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
Powered by blists - more mailing lists