linux-kernel - Re: [PATCH v2 12/39] x86/ibt,ftrace: Search for __fentry_

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:   Thu, 03 Mar 2022 15:15:14 +0530
From:   "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com>
To:     Peter Zijlstra <peterz@...radead.org>,
        Steven Rostedt <rostedt@...dmis.org>
Cc:     alexei.starovoitov@...il.com, alyssa.milburn@...el.com,
        andrew.cooper3@...rix.com, hjl.tools@...il.com,
        joao@...rdrivepizza.com, jpoimboe@...hat.com,
        keescook@...omium.org, linux-kernel@...r.kernel.org,
        mark.rutland@....com, mbenes@...e.cz,
        Masami Hiramatsu <mhiramat@...nel.org>,
        ndesaulniers@...gle.com, samitolvanen@...gle.com, x86@...nel.org
Subject: Re: [PATCH v2 12/39] x86/ibt,ftrace: Search for __fentry__ location

Peter Zijlstra wrote:
> On Wed, Mar 02, 2022 at 02:47:16PM -0500, Steven Rostedt wrote:
>> Note, I just pulled this patch, and I hit this warning:
>> 
>> WARNING: CPU: 0 PID: 6965 at arch/x86/kernel/kprobes/core.c:205 recover_probed_instruction+0x8f/0xa0
>> 
>> static unsigned long
>> __recover_probed_insn(kprobe_opcode_t *buf, unsigned long addr)
>> {
>>         struct kprobe *kp;
>>         unsigned long faddr;
>> 
>>         kp = get_kprobe((void *)addr);
>>         faddr = ftrace_location(addr);
>>         /*
>>          * Addresses inside the ftrace location are refused by
>>          * arch_check_ftrace_location(). Something went terribly wrong
>>          * if such an address is checked here.
>>          */
>>         if (WARN_ON(faddr && faddr != addr))  <<---- HERE
>>                 return 0UL;
> 
> Ha! so a bunch of IRC later I figured out how it is possible you hit
> this with just the patch on and how I legitimately hit this and what to
> do about it.
> 
> Your problem seems to be that we got ftrace_location() wrong in that
> lookup_rec()'s end argument is inclusive, hence we need:
> 
> 	lookup_rec(ip, ip + size - 1)
> 
> Now, the above thing asserts that:
> 
> 	ftrace_location(x) == {0, x}
> 
> and that is genuinely false in my case, I get x+4 as additional possible
> output. So I think I need the below change to go on top of all I have:
> 
> 
> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> index 7f0ce42f8ff9..4c13406e0bc4 100644
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -198,13 +198,14 @@ __recover_probed_insn(kprobe_opcode_t *buf, unsigned long addr)
> 
>  	kp = get_kprobe((void *)addr);
>  	faddr = ftrace_location(addr);
> +
>  	/*
> -	 * Addresses inside the ftrace location are refused by
> -	 * arch_check_ftrace_location(). Something went terribly wrong
> -	 * if such an address is checked here.
> +	 * In case addr maps to sym+0 ftrace_location() might return something
> +	 * other than faddr. In that case consider it the same as !faddr.
>  	 */
> -	if (WARN_ON(faddr && faddr != addr))
> -		return 0UL;
> +	if (faddr && faddr != addr)
> +		faddr = 0;
> +
>  	/*
>  	 * Use the current code if it is not modified by Kprobe
>  	 * and it cannot be modified by ftrace.

I hit this issue yesterday in kprobe generic code in check_ftrace_location(). 
In both these scenarios, we just want to check if a particular instruction is 
reserved by ftrace.  ftrace_location_range() should still work for that 
purpose, so that may be the easier fix:

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 066fa644e9dfa3..ee3cd035403ca2 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1596,7 +1596,7 @@ static int check_ftrace_location(struct kprobe *p)
 {
 	unsigned long ftrace_addr;
 
-	ftrace_addr = ftrace_location((unsigned long)p->addr);
+	ftrace_addr = ftrace_location_range((unsigned long)p->addr, (unsigned long)p->addr);
 	if (ftrace_addr) {
 #ifdef CONFIG_KPROBES_ON_FTRACE
 		/* Given address is not on the instruction boundary */



- Naveen