linux-kernel - Re: [RFC v5 12/57] objtool: check: Allow jumps from an alternative group to itself

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: <37aaea62-4737-d7e7-8107-4e8b05fc849c@redhat.com>
Date:   Thu, 23 Jan 2020 13:42:03 +0000
From:   Julien Thierry <jthierry@...hat.com>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        peterz@...radead.org, raphael.gault@....com,
        catalin.marinas@....com, will@...nel.org
Subject: Re: [RFC v5 12/57] objtool: check: Allow jumps from an alternative
 group to itself



On 1/21/20 5:33 PM, Josh Poimboeuf wrote:
> On Thu, Jan 09, 2020 at 04:02:15PM +0000, Julien Thierry wrote:
>> Alternatives can contain instructions that jump to another instruction
>> in the same alternative group. This is actually a common pattern on
>> arm64.
>>
>> Keep track of instructions jumping within their own alternative group
>> and carry on validating such branches.
>>
>> Signed-off-by: Julien Thierry <jthierry@...hat.com>
>> ---
>>   tools/objtool/check.c | 48 ++++++++++++++++++++++++++++++++++---------
>>   tools/objtool/check.h |  1 +
>>   2 files changed, 39 insertions(+), 10 deletions(-)
>>
>> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
>> index 8f2ff030936d..c7b3f1e2a628 100644
>> --- a/tools/objtool/check.c
>> +++ b/tools/objtool/check.c
>> @@ -722,6 +722,14 @@ static int handle_group_alt(struct objtool_file *file,
>>   	sec_for_each_insn_from(file, insn) {
>>   		if (insn->offset >= special_alt->orig_off + special_alt->orig_len)
>>   			break;
>> +		/* Is insn a jump to an instruction within the alt_group */
>> +		if (insn->jump_dest && insn->sec == insn->jump_dest->sec &&
>> +		    (insn->type == INSN_JUMP_CONDITIONAL ||
>> +		     insn->type == INSN_JUMP_UNCONDITIONAL)) {
>> +			dest_off = insn->jump_dest->offset;
>> +			insn->intra_group_jump = special_alt->orig_off <= dest_off &&
>> +				dest_off < special_alt->orig_off + special_alt->orig_len;
>> +		}
> 
> This patch adds some complexity, just so we can keep the
> 
>    "don't know how to handle branch to middle of alternative instruction group"
> 
> warning for the case where code from outside an alternative insn group
> is branching to inside the group.  But I've never actually seen that
> case in practice, and I get the feeling that warning isn't very useful
> or realistic.
> 
> How about we just remove the warning?
> 

I'm happy to remove it.

I was trying to look for a less intrusive place to only check for 
instructions jumping into alternatives. add_jump_destinations() could be 
an option, but it would require to duplicate some stuff done in 
add_special_section_alts(). So maybe just ignoring for now can be fine.

Thanks,

-- 
Julien Thierry