Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH V3 4/9] objtool: Handle return instruction with
 intra-function call
To:     Alexandre Chartre <alexandre.chartre@oracle.com>, x86@kernel.org
Cc:     linux-kernel@vger.kernel.org, jpoimboe@redhat.com,
        peterz@infradead.org, tglx@linutronix.de
References: <20200414103618.12657-1-alexandre.chartre@oracle.com>
 <20200414103618.12657-5-alexandre.chartre@oracle.com>
From:   Julien Thierry <jthierry@redhat.com>
Message-ID: <050ba999-5d16-170f-8a70-44d3856eb1ea@redhat.com>
Date:   Tue, 14 Apr 2020 14:44:57 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.4.1
MIME-Version: 1.0
In-Reply-To: <20200414103618.12657-5-alexandre.chartre@oracle.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Alexandre,

On 4/14/20 11:36 AM, Alexandre Chartre wrote:
> Intra-function calls are implemented in objtool like unconditional
> jumps. Keep track of intra-functin calls return addresses so that

intra-function*

> objtool can make a return instruction continues the flow at the
> right location.
> 
> Signed-off-by: Alexandre Chartre <alexandre.chartre@oracle.com>
> ---
>   tools/objtool/arch/x86/decode.c |   7 +++
>   tools/objtool/check.c           | 104 ++++++++++++++++++++++++++++++--
>   tools/objtool/check.h           |   3 +
>   3 files changed, 110 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c
> index f4d70b8835c4..76b593bb2e4f 100644
> --- a/tools/objtool/arch/x86/decode.c
> +++ b/tools/objtool/arch/x86/decode.c
> @@ -427,6 +427,13 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
>   	case 0xc2:
>   	case 0xc3:
>   		*type = INSN_RETURN;
> +		/*
> +		 * For the impact on the stack, a ret behaves like
> +		 * a pop of the return address.
> +		 */
> +		op->src.type = OP_SRC_POP;
> +		op->dest.type = OP_DEST_REG;
> +		op->dest.reg = CFI_RA;
>   		break;
>   
>   	case 0xca: /* retf */
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index ad362c5de281..8b1df659cd68 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -26,9 +26,50 @@ struct alternative {
>   	bool skip_orig;
>   };
>   
> +/*
> + * List to keep track of intra-function call return addresses.
> + * The list is a simple static array because we don't expect
> + * to have a lot of nested intra-function calls.
> + */
> +#define RADDR_COUNT_MAX		32
> +#define RADDR_ALTERED		((void *)-1)
> +
> +static struct instruction *raddr_list[RADDR_COUNT_MAX];
> +static int raddr_count;

Could this be part of the insn_state?

> +
>   const char *objname;
>   struct cfi_state initial_func_cfi;
>   
> +static void raddr_clear(void)
> +{
> +	raddr_count = 0;
> +}
> +
> +static bool raddr_push(struct instruction *insn)
> +{
> +	if (raddr_count == RADDR_COUNT_MAX) {
> +		WARN_FUNC("return address list is full",
> +			  insn->sec, insn->offset);
> +		return false;
> +	}
> +
> +	raddr_list[raddr_count++] = insn;
> +	return true;
> +}
> +
> +static bool raddr_pop(struct instruction **insn)
> +{
> +	if (raddr_count == 0)
> +		return false;
> +
> +	*insn = raddr_list[--raddr_count];
> +	return true;
> +}
> +
> +static int validate_branch(struct objtool_file *file, struct symbol *func,
> +			   struct instruction *from,
> +			   struct instruction *first, struct insn_state state);
> +
>   struct instruction *find_insn(struct objtool_file *file,
>   			      struct section *sec, unsigned long offset)
>   {
> @@ -2039,8 +2080,52 @@ static int validate_sibling_call(struct instruction *insn, struct insn_state *st
>   	return validate_call(insn, state);
>   }
>   
> -static int validate_return(struct symbol *func, struct instruction *insn, struct insn_state *state)
> +static int validate_return_address(struct objtool_file *file,
> +				   struct symbol *func,
> +				   struct instruction *insn,
> +				   struct insn_state *state)
>   {
> +	struct instruction *raddr_insn;
> +	int ret;
> +
> +	while (raddr_pop(&raddr_insn)) {

Why is this a loop? Either there is something on the return address 
stack, it gets validated as a branch and the function returns, or 
nothing is on the stack and the function returns.

There doesn't seem to be a point where the loop gets to a second iteration.

> +		/*
> +		 * We are branching somewhere and so processing
> +		 * a return instruction. So update the stack
> +		 * state for this instruction.
> +		 */
> +		update_insn_state(insn, state);
> +
> +		/*
> +		 * If the return address has no instruction then
> +		 * that's the end of the function.
> +		 */
> +		if (!raddr_insn)

I can't think of a valid case where raddr_pop() returns "true" for a 
NULL return address. If you check for that case, it should probably be a 
warning/error rather than silently returning (especially since you'd 
have updated the state).

> +			break;
> +
> +		/*
> +		 * If we are branching to a defined address then
> +		 * just do an unconditional jump there.
> +		 */
> +		ret = validate_branch(file, func, insn,
> +				      raddr_insn, *state);
> +		if (ret) {
> +			if (backtrace)
> +				BT_FUNC("(ret-branch)", insn);
> +			return ret;
> +		}
> +
> +		return 0;
> +	}
> +
> +	return -1;

Returning "-1" is a bit confusing. One might think this is reporting an 
error, but the only caller actually uses it as "I'll just carry on with 
the rest of my checks".

Maybe validate_return() could call validate_return_address() as follows:

	if (radd_pop(&raddr_insn))
		return validate_return_address(file, func, insn, state, raddr_insn);


> +}
> +
> +static int validate_return(struct objtool_file *file, struct symbol *func,
> +			   struct instruction *insn, struct insn_state *state)
> +{
> +	int ret;
> +
>   	if (state->uaccess && !func_uaccess_safe(func)) {
>   		WARN_FUNC("return with UACCESS enabled",
>   			  insn->sec, insn->offset);
> @@ -2059,6 +2144,11 @@ static int validate_return(struct symbol *func, struct instruction *insn, struct
>   		return 1;
>   	}
>   
> +	/* check if we have return address to branch to */
> +	ret = validate_return_address(file, func, insn, state);
> +	if (ret >= 0)
> +		return ret;
> +
>   	if (func && has_modified_stack_frame(state)) {
>   		WARN_FUNC("return with modified stack frame",
>   			  insn->sec, insn->offset);
> @@ -2200,7 +2290,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>   		switch (insn->type) {
>   
>   		case INSN_RETURN:
> -			return validate_return(func, insn, &state);
> +			return validate_return(file, func, insn, &state);
>   
>   		case INSN_CALL:
>   		case INSN_CALL_DYNAMIC:
> @@ -2223,12 +2313,17 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>   				/*
>   				 * The call instruction can update the stack
>   				 * state. Then make the intra-function call
> -				 * behaves like and unconditional jump.
> +				 * behaves like and unconditional jump. We
> +				 * track the return address to handle any
> +				 * return instruction.
>   				 */
>   				ret = update_insn_state(insn, &state);
>   				if (ret)
>   					return ret;
>   
> +				if (!raddr_push(next_insn))
> +					return 1;
> +
>   				ret = validate_branch(file, func, insn,
>   						      insn->jump_dest, state);
>   				if (ret) {
> @@ -2383,6 +2478,7 @@ static int validate_unwind_hints(struct objtool_file *file)
>   
>   	for_each_insn(file, insn) {
>   		if (insn->hint && !insn->visited) {
> +			raddr_clear();
>   			ret = validate_branch(file, insn->func,
>   					      NULL, insn, state);
>   			if (ret && backtrace)
> @@ -2522,7 +2618,7 @@ static int validate_section(struct objtool_file *file, struct section *sec)
>   			continue;
>   
>   		state.uaccess = func->uaccess_safe;
> -
> +		raddr_clear();
>   		ret = validate_branch(file, func, NULL, insn, state);
>   		if (ret && backtrace)
>   			BT_FUNC("<=== (func)", insn);
> diff --git a/tools/objtool/check.h b/tools/objtool/check.h
> index 6a80903fc4aa..f7dbecd46bed 100644
> --- a/tools/objtool/check.h
> +++ b/tools/objtool/check.h
> @@ -23,6 +23,7 @@ struct insn_state {
>   	unsigned int uaccess_stack;
>   	int drap_reg, drap_offset;
>   	struct cfi_reg vals[CFI_NUM_REGS];
> +	bool stack_altered;

This isn't used in this patch.

>   };
>   
>   struct instruction {
> @@ -39,6 +40,8 @@ struct instruction {
>   	bool intra_function_call;
>   	bool retpoline_safe;
>   	u8 visited;
> +	u8 raddr_delete;
> +	u8 raddr_alter;

These fields (and the RADDR_ALTERED) don't seem to be used elsewhere in 
this patch. I guess they should be part of patch 5.

>   	struct symbol *call_dest;
>   	struct instruction *jump_dest;
>   	struct instruction *first_jump_src;
> 

Cheers,

-- 
Julien Thierry