linux-kernel - Re: [PATCH v10 08/11] arm64: Rename unwinder functions, prevent them from being traced and kprobed

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:   Wed, 27 Oct 2021 15:07:09 -0500
From:   "Madhavan T. Venkataraman" <madvenka@...ux.microsoft.com>
To:     Mark Rutland <mark.rutland@....com>
Cc:     broonie@...nel.org, jpoimboe@...hat.com, ardb@...nel.org,
        nobuta.keiya@...itsu.com, sjitindarsingh@...il.com,
        catalin.marinas@....com, will@...nel.org, jmorris@...ei.org,
        linux-arm-kernel@...ts.infradead.org,
        live-patching@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v10 08/11] arm64: Rename unwinder functions, prevent them
 from being traced and kprobed



On 10/27/21 12:53 PM, Mark Rutland wrote:
> On Thu, Oct 14, 2021 at 09:58:44PM -0500, madvenka@...ux.microsoft.com wrote:
>> From: "Madhavan T. Venkataraman" <madvenka@...ux.microsoft.com>
>>
>> Rename unwinder functions for consistency and better naming.
>>
>> 	- Rename start_backtrace() to unwind_start().
>> 	- Rename unwind_frame() to unwind_next().
>> 	- Rename walk_stackframe() to unwind().
> 
> This looks good to me.
> 

Thanks.

> Could we split this from the krpbes/tracing changes? I think this stands
> on it's own, and (as below) the kprobes/tracing changes need some more
> explanation, and would make sense as a separate patch.
> 

OK. I will split the patches.

>> Prevent the following unwinder functions from being traced:
>>
>> 	- unwind_start()
>> 	- unwind_next()
>>
>> 	unwind() is already prevented from being traced.
> 
> This could do with an explanation in the commis message as to why we
> need to do this. If this is fixing a latent issue, it should be in a
> preparatory patch that we can backport.
> 
> I dug into this a bit, and from taking a look, we prohibited ftrace in commit:
> 
>   0c32706dac1b0a72 ("arm64: stacktrace: avoid tracing arch_stack_walk()")
> 
> ... which is just one special case of graph return stack unbalancing,
> and should be addressed by using HAVE_FUNCTION_GRAPH_RET_ADDR_PTR, so
> with the patch making us use HAVE_FUNCTION_GRAPH_RET_ADDR_PTR, that's
> no longer necessary.
> 
> So we no longer seem to have a specific reason to prohibit ftrace
> here.
> 

OK, I will think about this and add a comment.

>> Prevent the following unwinder functions from being kprobed:
>>
>> 	- unwind_start()
>>
>> 	unwind_next() and unwind() are already prevented from being kprobed.
> 
> Likewise, I think this needs some explanation. From diggin, we
> prohibited kprobes in commit:
> 
>   ee07b93e7721ccd5 ("arm64: unwind: Prohibit probing on return_address()")
> 
> ... and the commit message says we need to do this because this is
> (transitively) called by trace_hardirqs_off(), which is kprobes
> blacklisted, but doesn't explain the actual problem this results in.
> 

OK. I will think about this and add a comment.

> AFAICT x86 directly uses __builtin_return_address() here, but that won't
> recover rewritten addresses, which seems like a bug (or at least a
> limitation) on x86, assuming I've read that correctly.
> 

OK.

Thanks,

Madhavan