linux-kernel - Re: [RFC 07/13] objtool: Extract code to validate instruction from the validate branch loop

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <op34sueobpwyzrxm4gmxrkzauhfzcg2wqktbjzrzg2n3gq4uiy@4zlau7mwrycm>
Date: Tue, 10 Jun 2025 16:31:59 -0700
From: Josh Poimboeuf <jpoimboe@...nel.org>
To: Alexandre Chartre <alexandre.chartre@...cle.com>
Cc: linux-kernel@...r.kernel.org, mingo@...nel.org, peterz@...radead.org
Subject: Re: [RFC 07/13] objtool: Extract code to validate instruction from
 the validate branch loop

On Fri, Jun 06, 2025 at 05:34:34PM +0200, Alexandre Chartre wrote:
> The code to validate a branch loops through all instructions of the
> branch and validate each instruction. Move the code to validate an
> instruction to a separated function.
> 
> Signed-off-by: Alexandre Chartre <alexandre.chartre@...cle.com>
> ---
>  tools/objtool/check.c | 375 +++++++++++++++++++++++-------------------
>  1 file changed, 208 insertions(+), 167 deletions(-)
> 
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 2c73c8d3515d..36ec08b8d654 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -3527,6 +3527,11 @@ static bool skip_alt_group(struct instruction *insn)
>  	return alt_insn->type == INSN_CLAC || alt_insn->type == INSN_STAC;
>  }
>  
> +static int validate_insn(struct objtool_file *file, struct symbol *func,
> +			 struct instruction *insn, struct insn_state *statep,
> +			 struct instruction *prev_insn, struct instruction *next_insn,
> +			 bool *validate_nextp);

Since validate_insn() is always called by validate_branch(), put the
validate_insn() implementation here above validate_branch().

>  /*
>   * Follow the branch starting at the given instruction, and recursively follow
>   * any other branches (jumps).  Meanwhile, track the frame pointer state at
> @@ -3536,10 +3541,9 @@ static bool skip_alt_group(struct instruction *insn)
>  static int validate_branch(struct objtool_file *file, struct symbol *func,
>  			   struct instruction *insn, struct insn_state state)
>  {
> -	struct alternative *alt;
>  	struct instruction *next_insn, *prev_insn = NULL;
>  	struct section *sec;
> -	u8 visited;
> +	bool validate_next;

I think it's clearer if we reverse the polarity and call it "dead_end".

> @@ -3566,232 +3570,269 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>  			return 1;
>  		}
>  
> -		visited = VISITED_BRANCH << state.uaccess;
> -		if (insn->visited & VISITED_BRANCH_MASK) {
> -			if (!insn->hint && !insn_cfi_match(insn, &state.cfi))
> -				return 1;
> +		ret = validate_insn(file, func, insn, &state,
> +				    prev_insn, next_insn,
> +				    &validate_next);

This can fit in two lines:

		ret = validate_insn(file, func, insn, &state, prev_insn,
				    next_insn, &dead_end);

> +static int validate_insn(struct objtool_file *file, struct symbol *func,
> +			 struct instruction *insn, struct insn_state *statep,
> +			 struct instruction *prev_insn, struct instruction *next_insn,
> +			 bool *validate_nextp)
> +{
> +	struct alternative *alt;
> +	u8 visited;
> +	int ret;
>  
> -				sym_for_each_insn_continue_reverse(file, func, i) {
> -					if (i->save) {
> -						save_insn = i;
> -						break;
> -					}
> -				}
> +	/*
> +	 * Indicate that, by default, the calling function should not
> +	 * validate the next instruction and validation should be
> +	 * stopped. That way this function can stop validation by just
> +	 * returning at any point before reaching the end of the function.
> +	 *
> +	 * If the end of this function is reached then that means that the
> +	 * validation should continue and the caller should validate the
> +	 * next instruction, so *validate_nextp will be set to true at
> +	 * that point.
> +	 */
> +	*validate_nextp = false;

This can be much more succinct:

	/*
	 * Any returns before the end of this function are effectively dead
	 * ends, i.e. validate_branch() has reached the end of the branch.
	 */
	*dead_end = true;


> +	default:
> +		break;
>  	}
>  
> +	if (insn->dead_end)
> +		return 0;
> +
> +	/*
> +	 * Indicate that the caller should validate the next
> +	 * instruction and continue the validation.
> +	 */
> +	*validate_nextp = true;
> +
>  	return 0;

No comment needed here.  Also this can be condensed:

	*dead_end = insn->dead_end;
	return 0;

-- 
Josh