linux-kernel - Re: [PATCH v2 10/10] objtool: Support multiple stack

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: <3b3f07b3-172b-0560-96c7-d9386e3cab23@redhat.com>
Date:   Fri, 3 Apr 2020 09:01:46 +0100
From:   Julien Thierry <jthierry@...hat.com>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     linux-kernel@...r.kernel.org, peterz@...radead.org,
        raphael.gault@....com
Subject: Re: [PATCH v2 10/10] objtool: Support multiple stack_op per
 instruction



On 4/2/20 6:54 PM, Josh Poimboeuf wrote:
> On Fri, Mar 27, 2020 at 03:28:47PM +0000, Julien Thierry wrote:
>> @@ -127,6 +129,10 @@ int arch_decode_instruction(struct elf *elf, struct section *sec,
>>   	if (insn.sib.nbytes)
>>   		sib = insn.sib.bytes[0];
>>   
>> +	op = calloc(1, sizeof(*op));
>> +	if (!op)
>> +		return -1;
>> +
> 
> Why not malloc()?
> 

It's just that previsously, stack_op was part of the instruction 
structure and was initialized to all 0 in decode_instructions(). Now 
that it's created here, I assumed it would be better to have the same 
thing here and initialized the new stack_op to all 0.

Do you prefer to have an explicit malloc() + memset()?

>> +static int handle_insn_ops(struct instruction *insn, struct insn_state *state)
>> +{
>> +	struct stack_op *op;
>> +
>> +	list_for_each_entry(op, &insn->stack_ops, list) {
>> +		int res;
>> +
>> +		res = update_insn_state(insn, state, op);
>> +		if (res)
>> +			return res;
> 
> This should probably be like:
> 
> 		if (update_insn_state(insn, state, op))
> 			return 1;
> 
> That way the error codes are converted to non-fatal warnings like before
> (which I admit is confusing...)
> 

Right, I'll change this.

>> @@ -2205,29 +2244,8 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
>>   			return 0;
>>   
>>   		case INSN_STACK:
>> -			if (update_insn_state(insn, &state))
>> +			if (handle_insn_ops(insn, &state))
>>   				return 1;
> 
> How about "handle_stack_ops"?
> 

Works for me!

Thanks,

-- 
Julien Thierry