linux-kernel - Re: [RFC PATCH v2 11/20] objtool: arm64: Walk instructions and compute CFI for each instruction

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, 30 May 2022 09:44:37 +0800
From:   Chen Zhongjin <chenzhongjin@...wei.com>
To:     "Madhavan T. Venkataraman" <madvenka@...ux.microsoft.com>,
        <jpoimboe@...hat.com>, <peterz@...radead.org>,
        <mark.rutland@....com>, <broonie@...nel.org>,
        <nobuta.keiya@...itsu.com>, <sjitindarsingh@...il.com>,
        <catalin.marinas@....com>, <will@...nel.org>,
        <jamorris@...ux.microsoft.com>,
        <linux-arm-kernel@...ts.infradead.org>,
        <live-patching@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH v2 11/20] objtool: arm64: Walk instructions and
 compute CFI for each instruction

Hi,

On 2022/5/29 23:18, Madhavan T. Venkataraman wrote:
> 
> 
> On 5/24/22 08:45, Chen Zhongjin wrote:
>> Hi,
>>
>> On 2022/5/24 8:16, madvenka@...ux.microsoft.com wrote:
>>> From: "Madhavan T. Venkataraman" <madvenka@...ux.microsoft.com>
>>>
>>> Implement arch_initial_func_cfi_state() to initialize the CFI for a
>>> function.
>>>
>>> Add code to fpv_decode() to walk the instructions in every function and
>>> compute the CFI information for each instruction.
>>>
>>> Implement special handling for cases like jump tables.
>>>
>>> Signed-off-by: Madhavan T. Venkataraman <madvenka@...ux.microsoft.com>
>>> ---
>>>  tools/objtool/arch/arm64/decode.c |  15 +++
>>>  tools/objtool/fpv.c               | 204 ++++++++++++++++++++++++++++++
>>>  2 files changed, 219 insertions(+)
>> ...
>>> +static void update_cfi_state(struct cfi_state *cfi, struct stack_op *op)
>>> +{
>>> +	struct cfi_reg *cfa = &cfi->cfa;
>>> +	struct cfi_reg *regs = cfi->regs;
>>> +
>>> +	if (op->src.reg == CFI_SP) {
>>> +		if (op->dest.reg == CFI_SP)
>>> +			cfa->offset -= op->src.offset;
>>> +		else
>>> +			regs[CFI_FP].offset = -cfa->offset + op->src.offset;
>> Seems wrong here, we don't have any op->src.offset for [mov x29, sp] so here we
>> get: fp->offset = -cfa->offset. The dumped info also proves this.
> 
> 
> See the example below.
> 
>>
>>> +	case UNWIND_HINT_TYPE_CALL:
>>> +		/* Normal call */
>>> +		frame->cfa += orc->sp_offset;
>>> +		fp = frame->cfa + orc->fp_offset;
>>> +		break;
>> Obviously this is not conform to the reliability check because we get
>> frame->cfa == fp here.
>>
> 
> See the example below:
> 
>> IIUC your sp_offset equals to stack length, and fp_offset is offset from next
>> x29 to next CFA. So maybe here we should have
>> regs[CFI_FP].offset = regs[CFI_SP].offset for [mov x29, sp].
>>
>> Anyway, in original objtool sp_offset and fp_offset both represents the offset
>> from CFA to REGs. I think it's better not spoiling their original meaning and
>> just extending.
>>
>>
> 
> I am not spoiling anything.
> 
> 
> Let us take an example:
> 
> ffff800008010000 <bcm2835_handle_irq>:
> ffff800008010000:       d503201f        nop
> ffff800008010004:       d503201f        nop
> ffff800008010008:       d503233f        paciasp
> ffff80000801000c:       a9be7bfd        stp     x29, x30, [sp, #-32]!
> ffff800008010010:       910003fd        mov     x29, sp
> ffff800008010014:       f9000bf3        str     x19, [sp, #16]
> 
> 
> The stack pointer is first moved by -32 and the FP and LR are stored there.
> At this point, SP is pointing to the frame. The CFA is:
> 
> 	CFA = SP + 32
> 
> The frame pointer has been stored at the location pointed to by the SP.
> So, FP should be:
> 
> 	FP = CFA - 32
> 
> Therefore, at instruction address ffff800008010014:
> 
> 	frame->cfa = SP + 32;
> 	frame->fp = frame->cfa - 32 = SP;
> 
> So, if a call/interrupt happens after this instruction, the frame pointer computed
> from the above data will match with the actual frame pointer.
> 
> I have verified this using the DWARF data generated by the compiler. It is correct.
> I have also verified that the stack trace through such code passes the reliability
> check. That is, it computes the frame pointer correctly which matches with the
> actual frame pointer
You are right, I think I mixed up frame of x86 and arm64.

Apologize for that and thanks for explaining!

Best,
Chen