linux-kernel - Re: [PATCH 4/8] objtool: Add support for intra-function calls

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: <alpine.LSU.2.21.2004231619070.6520@pobox.suse.cz>
Date:   Thu, 23 Apr 2020 16:34:21 +0200 (CEST)
From:   Miroslav Benes <mbenes@...e.cz>
To:     Peter Zijlstra <peterz@...radead.org>
cc:     jpoimboe@...hat.com, alexandre.chartre@...cle.com,
        linux-kernel@...r.kernel.org, jthierry@...hat.com,
        tglx@...utronix.de, x86@...nel.org
Subject: Re: [PATCH 4/8] objtool: Add support for intra-function calls

>  /*
>   * Find the destination instructions for all calls.
>   */
> @@ -715,10 +725,7 @@ static int add_call_destinations(struct
>  				continue;
>  
>  			if (!insn->call_dest) {
> -				WARN_FUNC("unsupported intra-function call",
> -					  insn->sec, insn->offset);
> -				if (retpoline)
> -					WARN("If this is a retpoline, please patch it in with alternatives and annotate it with ANNOTATE_NOSPEC_ALTERNATIVE.");
> +				WARN_FUNC("intra-function call", insn->sec, insn->offset);

"unsupported intra-function call"?

>  				return -1;
>  			}
>  
> @@ -741,6 +748,12 @@ static int add_call_destinations(struct
>  			}
>  		} else
>  			insn->call_dest = rela->sym;
> +
> +		/*
> +		 * Whatever stack impact regular CALLs have, should be
> +		 * undone by the RETURN of the called function.

 * Annotated intra-function CALLs are treated as JMPs with a stack_op.
 * See read_intra_function_calls().

would make it a bit clearer.

> +                */
> +               remove_insn_ops(insn);
>         }
>  
>         return 0;
> @@ -1416,6 +1429,57 @@ static int read_instr_hints(struct objto
>         return 0;
>  }
> 
> +static int read_intra_function_calls(struct objtool_file *file)
> +{
> +       struct instruction *insn;
> +       struct section *sec;
> +       struct rela *rela;
> +
> +       sec = find_section_by_name(file->elf, ".rela.discard.intra_function_calls");
> +       if (!sec)
> +               return 0;
> +
> +       list_for_each_entry(rela, &sec->rela_list, list) {
> +               unsigned long dest_off;
> +
> +               if (rela->sym->type != STT_SECTION) {
> +                       WARN("unexpected relocation symbol type in %s",
> +                            sec->name);
> +                       return -1;
> +               }
> +
> +               insn = find_insn(file, rela->sym->sec, rela->addend);
> +               if (!insn) {
> +                       WARN("bad .discard.intra_function_call entry");
> +                       return -1;
> +               }
> +
> +               if (insn->type != INSN_CALL) {
> +                       WARN_FUNC("intra_function_call not a direct call",
> +                                 insn->sec, insn->offset);
> +                       return -1;
> +               }
> +
> +               /*
> +                * Treat intra-function CALLs as JMPs, but with a stack_op.
> +                * Also see how setup_call_dest() strips stack_ops from normal
> +                * CALLs.

/*
 * Treat annotated intra-function CALLs as JMPs, but with a stack_op.
 * Also see how add_call_destinations() strips stack_ops from normal
 * CALLs.
 */

? (note added "annotated" and s/setup_call_dest/add_call_destinations/)

> +                */
> +               insn->type = INSN_JUMP_UNCONDITIONAL;

[...]

> @@ -2245,6 +2313,9 @@ static int validate_branch(struct objtoo
>  				return 0;
>  		}
>  
> +		if (handle_insn_ops(insn, &state))
> +			return 1;
> +
>  		switch (insn->type) {
>  
>  		case INSN_RETURN:
> @@ -2304,9 +2375,6 @@ static int validate_branch(struct objtoo
>  			break;
>  
>  		case INSN_EXCEPTION_RETURN:
> -			if (handle_insn_ops(insn, &state))
> -				return 1;
> -
>  			/*
>  			 * This handles x86's sync_core() case, where we use an
>  			 * IRET to self. All 'normal' IRET instructions are in
> @@ -2326,8 +2394,6 @@ static int validate_branch(struct objtoo
>  			return 0;
>  
>  		case INSN_STACK:
> -			if (handle_insn_ops(insn, &state))
> -				return 1;
>  			break;

So we could get rid of INSN_STACK now as Julien proposed, couldn't we? If 
I am not missing something. handle_insn_ops() is called unconditionally 
here for all insn types and you remove stack_ops when unneeded.

We could also go ahead with Julien's proposal to remove 
INSN_EXCEPTION_RETURN hack and move it to tools/objtool/arch/x86/decode.c. 

Miroslav