linux-kernel - Re: [PATCH 3/3] objtool: check: Make SP memory operation match PUSH/POP semantics

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]

Date:   Mon, 21 Sep 2020 11:31:28 +0100
From:   Julien Thierry <jthierry@...hat.com>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     linux-kernel@...r.kernel.org, peterz@...radead.org, mbenes@...e.cz,
        raphael.gault@....com, benh@...nel.crashing.org
Subject: Re: [PATCH 3/3] objtool: check: Make SP memory operation match
 PUSH/POP semantics



On 9/18/20 10:43 PM, Josh Poimboeuf wrote:
> On Tue, Sep 15, 2020 at 09:12:04AM +0100, Julien Thierry wrote:
>> Architectures without PUSH/POP instructions will always access the stack
>> though memory operations (SRC/DEST_INDIRECT). Make those operations have
>> the same effect on the CFA as PUSH/POP, with no stack pointer
>> modification.
>>
>> Signed-off-by: Julien Thierry <jthierry@...hat.com>
>> ---
>>   tools/objtool/check.c | 17 +++++++++++++++++
>>   1 file changed, 17 insertions(+)
>>
>> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
>> index f45991c2db41..7ff87fa3caec 100644
>> --- a/tools/objtool/check.c
>> +++ b/tools/objtool/check.c
>> @@ -2005,6 +2005,13 @@ static int update_cfi_state(struct instruction *insn, struct cfi_state *cfi,
>>   			break;
>>   
>>   		case OP_SRC_REG_INDIRECT:
>> +			if (!cfi->drap && op->dest.reg == cfa->base) {
> 
> && op->dest.reg == CFI_BP ?
> 

Does it matter? My unstandig was that the register used to point to the 
CFA is getting overwritten, so we need to fallback to something known 
which is the offset from the stack pointer.

Was that not the case?

>> +
>> +				/* mov disp(%rsp), %rbp */
>> +				cfa->base = CFI_SP;
>> +				cfa->offset = cfi->stack_size;
>> +			}
>> +
>>   			if (cfi->drap && op->src.reg == CFI_BP &&
>>   			    op->src.offset == cfi->drap_offset) {
>>   
>> @@ -2026,6 +2033,11 @@ static int update_cfi_state(struct instruction *insn, struct cfi_state *cfi,
>>   				/* mov disp(%rbp), %reg */
>>   				/* mov disp(%rsp), %reg */
>>   				restore_reg(cfi, op->dest.reg);
>> +			} else if (op->src.reg == CFI_SP &&
> 
> An empty line above the else would help readability.
> 
>> +				   op->src.offset == regs[op->dest.reg].offset + cfi->stack_size) {
>> +
>> +				/* mov disp(%rsp), %reg */
>> +				restore_reg(cfi, op->dest.reg);
> 
>>   			}
>>   
>>   			break;
>> @@ -2103,6 +2115,11 @@ static int update_cfi_state(struct instruction *insn, struct cfi_state *cfi,
>>   			/* mov reg, disp(%rsp) */
>>   			save_reg(cfi, op->src.reg, CFI_CFA,
>>   				 op->dest.offset - cfi->cfa.offset);
>> +		} else if (op->dest.reg == CFI_SP) {
> 
> Same here.
> 

I'll add those.

>> +
>> +			/* mov reg, disp(%rsp) */
>> +			save_reg(cfi, op->src.reg, CFI_CFA,
>> +				 op->dest.offset - cfi->stack_size);
>>   		}
>>   
>>   		break;
>> -- 
>> 2.21.3
>>
> 

-- 
Julien Thierry