linux-kernel - Re: [PATCH v2 1/3] objtool: check: Fully validate the stack frame

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: <5540c2a9-db13-e05e-713c-22ec00f21aa6@redhat.com>
Date:   Mon, 12 Oct 2020 11:21:49 +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 v2 1/3] objtool: check: Fully validate the stack frame



On 9/29/20 8:18 PM, Josh Poimboeuf wrote:
> On Mon, Sep 28, 2020 at 10:36:29AM +0100, Julien Thierry wrote:
>> +++ b/tools/objtool/arch/x86/include/cfi_regs.h
>> @@ -22,4 +22,7 @@
>>   #define CFI_RA			16
>>   #define CFI_NUM_REGS		17
> 
> A few more naming nitpicks:
> 
>> +#define STACKFRAME_BP_OFFSET	-16
>> +#define STACKFRAME_RA_OFFSET	-8
> 
> "Stack frame" has more than one meaning now, I suppose.  i.e. it could
> also include the callee-saved registers and any other stack space
> allocated by the function.
> 
> Would "call frame" be clearer?
> 
>    CALL_FRAME_BP_OFFSET
>    CALL_FRAME_RA_OFFSET
> 
> ?

I would've thought that the call-frame could include the stackframe + 
other callee saved regs. Whereas stackframe tends to used for the 
caller's frame pointer + return address (i.e. what allows unwinding). 
Unless I'm getting lost with things.

And if call frame is associated with the region starting from the stack 
pointer at the parent call point (since this is what CFA is), then it 
shouldn't be associated with the framepointer + return address structure 
since this could be anywhere on the call frame (not at a fixed offset) 
as long as the new frame pointer points to the structure.

> 
>> +++ b/tools/objtool/cfi.h
>> @@ -35,4 +35,6 @@ struct cfi_state {
>>   	bool end;
>>   };
>>   
>> +#define STACKFRAME_SIZE	16
> 
> CALL_FRAME_SIZE ?
> 
> I'm sort of contradicting my previous comment here, but even though this
> value may be generic, it's also very much intertwined with the
> CALL_FRAME_{BP|RA}_OFFSET values.  So I get the feeling it really
> belongs in the arch-specific cfi_regs.h next to the other defines after
> all.
> 

Agreed.

-- 
Julien Thierry