linux-kernel - Re: [PATCH] x86/unwind/orc: unwind ftrace trampolines with correct orc

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: <24556707-99b3-ec3c-c176-cb73e1d030d8@huawei.com>
Date:   Thu, 18 Aug 2022 11:42:17 +0800
From:   Chen Zhongjin <chenzhongjin@...wei.com>
To:     Steven Rostedt <rostedt@...dmis.org>
CC:     <linux-kernel@...r.kernel.org>, <jpoimboe@...nel.org>,
        <peterz@...radead.org>, <tglx@...utronix.de>, <mingo@...hat.com>,
        <bp@...en8.de>, <dave.hansen@...ux.intel.com>, <x86@...nel.org>,
        <hpa@...or.com>
Subject: Re: [PATCH] x86/unwind/orc: unwind ftrace trampolines with correct
 orc

Thanks for review!


On 2022/8/18 10:28, Steven Rostedt wrote:
> On Thu, 18 Aug 2022 09:55:25 +0800
> Chen Zhongjin <chenzhongjin@...wei.com> wrote:
>
>
>>   arch/x86/kernel/unwind_orc.c | 13 ++++++++-----
>>   1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/kernel/unwind_orc.c b/arch/x86/kernel/unwind_orc.c
>> index 38185aedf7d1..a938c5d0ed6f 100644
>> --- a/arch/x86/kernel/unwind_orc.c
>> +++ b/arch/x86/kernel/unwind_orc.c
>> @@ -93,22 +93,25 @@ static struct orc_entry *orc_find(unsigned long ip);
>>   static struct orc_entry *orc_ftrace_find(unsigned long ip)
>>   {
>>   	struct ftrace_ops *ops;
>> -	unsigned long caller;
>> +	unsigned long tramp_addr, offset;
>>   
>>   	ops = ftrace_ops_trampoline(ip);
>>   	if (!ops)
>>   		return NULL;
>>   
> Now if this is that unlikely recursion mentioned below then ops->trampoline
> will be NULL, and if we do that offset addition, it will be incorrect.
>
> Perhaps we should add here:
>
> 	if (!ops->trampoline)
> 		return NULL;

I think when this will return NULL and then stop at orc_find:`if (ip == 
0)` and return null_orc_entry.

And in ftrace_ops_trampoline: `if (op->trampoline && 
op->trampoline_size)` which promise !ops->trampoline when !ops.


IIUC the In unlikely recursion below means if orc_find(ftrace_call) 
can't find any orc it will enter orc_ftrace_find(ftrace_call).

If we dont check ip==caller then,

ftrace_ops_trampoline(ftrace_call) causes orc_find(ftrace_call) again 
(I'm not 100% sure it will)

and it will be trapped in recursion


When here is an offset we can still protect this scenario when 
orc_find(ftrace_caller + offset) and check ip == ftrace_caller + offset.

>
> Let's add some comments.

Makes sense.

If the above explanation logic is fine, I'll add this comment and send v2.

>
> 	/* Set tramp_addr to the start of the code copied by the trampoline */
>
>>   	if (ops->flags & FTRACE_OPS_FL_SAVE_REGS)
>> -		caller = (unsigned long)ftrace_regs_call;
>> +		tramp_addr = (unsigned long)ftrace_regs_caller;
>>   	else
>> -		caller = (unsigned long)ftrace_call;
>> +		tramp_addr = (unsigned long)ftrace_caller;
>> +
> 	/* Now place tramp_addr to the location within the trampoline ip is at */
>
>> +	offset = ip - ops->trampoline;
>> +	tramp_addr += offset;
>>   
>>   	/* Prevent unlikely recursion */
>> -	if (ip == caller)
>> +	if (ip == tramp_addr)
>>   		return NULL;
>>   
>> -	return orc_find(caller);
>> +	return orc_find(tramp_addr);
>>   }
>>   #else
>>   static struct orc_entry *orc_ftrace_find(unsigned long ip)

Best,

Chen