live-patching.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
To: Mark Brown <broonie@kernel.org>
Cc: jpoimboe@redhat.com, mark.rutland@arm.com, jthierry@redhat.com,
	catalin.marinas@arm.com, will@kernel.org, jmorris@namei.org,
	pasha.tatashin@soleen.com, linux-arm-kernel@lists.infradead.org,
	live-patching@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH v3 2/4] arm64: Check the return PC against unreliable code sections
Date: Tue, 4 May 2021 14:32:35 -0500	[thread overview]
Message-ID: <0f72c4cb-25ef-ee23-49e4-986542be8673@linux.microsoft.com> (raw)
In-Reply-To: <1bd2b177-509a-21d9-e349-9b2388db45eb@linux.microsoft.com>



On 5/4/21 2:03 PM, Madhavan T. Venkataraman wrote:
> 
> 
> On 5/4/21 11:05 AM, Mark Brown wrote:
>> On Mon, May 03, 2021 at 12:36:13PM -0500, madvenka@linux.microsoft.com wrote:
>>> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
>>>
>>> Create a sym_code_ranges[] array to cover the following text sections that
>>> contain functions defined as SYM_CODE_*(). These functions are low-level
>>
>> This makes sense to me - a few of bikesheddy comments below but nothing
>> really substantive.
>>
> 
> OK.
> 
>>> +static struct code_range *lookup_range(unsigned long pc)
>>
>> This feels like it should have a prefix on the name (eg, unwinder_)
>> since it looks collision prone.  Or lookup_code_range() rather than just
>> plain lookup_range().
>>
> 
> I will add the prefix.
> 
>>> +{
>> +       struct code_range *range;
>> +         
>> +       for (range = sym_code_ranges; range->start; range++) {
>>
>> It seems more idiomatic to use ARRAY_SIZE() rather than a sentinel here,
>> the array can't be empty.
>>
> 
> If there is a match, I return the matched range. Else, I return the sentinel.
> This is just so I don't have to check for range == NULL after calling
> lookup_range().
> 
> I will change it to what you have suggested and check for NULL explicitly.
> It is not a problem.
> 
>>> +	range = lookup_range(frame->pc);
>>> +
>>>  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
>>>  	if (tsk->ret_stack &&
>>>  		frame->pc == (unsigned long)return_to_handler) {
>>> @@ -118,9 +160,21 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
>>>  			return -EINVAL;
>>>  		frame->pc = ret_stack->ret;
>>>  		frame->pc = ptrauth_strip_insn_pac(frame->pc);
>>> +		return 0;
>>>  	}
>>
>> Do we not need to look up the range of the restored pc and validate
>> what's being pointed to here?  It's not immediately obvious why we do
>> the lookup before handling the function graph tracer, especially given
>> that we never look at the result and there's now a return added skipping
>> further reliability checks.  At the very least I think this needs some
>> additional comments so the code is more obvious.
> I want sym_code_ranges[] to contain both unwindable and non-unwindable ranges.
> Unwindable ranges will be special ranges such as the return_to_handler() and
> kretprobe_trampoline() functions for which the unwinder has (or will have)
> special code to unwind. So, the lookup_range() has to happen before the
> function graph code. Please look at the last patch in the series for
> the fix for the above function graph code.
> 
> On the question of "should the original return address be checked against
> sym_code_ranges[]?" - I assumed that if there is a function graph trace on a
> function, it had to be an ftraceable function. It would not be a part
> of sym_code_ranges[]. Is that a wrong assumption on my part?
> 

If you prefer, I could do something like this:

check_pc:
	if (!__kernel_text_address(frame->pc))
		frame->reliable = false;

	range = lookup_range(frame->pc);

#ifdef CONFIG_FUNCTION_GRAPH_TRACER
	if (tsk->ret_stack &&
		frame->pc == (unsigned long)return_to_handler) {
		...
		frame->pc = ret_stack->ret;
		frame->pc = ptrauth_strip_insn_pac(frame->pc);
		goto check_pc;
	}
#endif /* CONFIG_FUNCTION_GRAPH_TRACER */

Is that acceptable?

Madhavan

  reply	other threads:[~2021-05-04 19:32 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <65cf4dfbc439b010b50a0c46ec500432acde86d6>
2021-05-03 17:36 ` [RFC PATCH v3 0/4] arm64: Stack trace reliability checks in the unwinder madvenka
2021-05-03 17:36   ` [RFC PATCH v3 1/4] arm64: Introduce stack " madvenka
2021-05-04 15:50     ` Mark Brown
2021-05-04 19:14       ` Madhavan T. Venkataraman
2021-05-04 21:52     ` Josh Poimboeuf
2021-05-04 23:13       ` Madhavan T. Venkataraman
2021-05-05  0:07         ` Josh Poimboeuf
2021-05-05  0:21           ` Madhavan T. Venkataraman
2021-05-03 17:36   ` [RFC PATCH v3 2/4] arm64: Check the return PC against unreliable code sections madvenka
2021-05-04 16:05     ` Mark Brown
2021-05-04 19:03       ` Madhavan T. Venkataraman
2021-05-04 19:32         ` Madhavan T. Venkataraman [this message]
2021-05-05 16:46           ` Mark Brown
2021-05-05 18:48             ` Madhavan T. Venkataraman
2021-05-05 18:50               ` Madhavan T. Venkataraman
2021-05-06 13:45               ` Mark Brown
2021-05-06 15:21                 ` Madhavan T. Venkataraman
2021-05-05 16:34         ` Mark Brown
2021-05-05 17:51           ` Madhavan T. Venkataraman
2021-05-05 19:30     ` Ard Biesheuvel
2021-05-05 20:00       ` Madhavan T. Venkataraman
2021-05-03 17:36   ` [RFC PATCH v3 3/4] arm64: Handle miscellaneous functions in .text and .init.text madvenka
2021-05-06 14:12     ` Mark Brown
2021-05-06 15:30       ` Madhavan T. Venkataraman
2021-05-06 15:32         ` Madhavan T. Venkataraman
2021-05-06 15:44           ` Mark Brown
2021-05-06 15:56             ` Madhavan T. Venkataraman
2021-05-06 15:37         ` Mark Brown
2021-05-06 15:57           ` Madhavan T. Venkataraman
2021-05-03 17:36   ` [RFC PATCH v3 4/4] arm64: Handle funtion graph tracer better in the unwinder madvenka
2021-05-06 14:43     ` Mark Brown
2021-05-06 15:20       ` Madhavan T. Venkataraman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0f72c4cb-25ef-ee23-49e4-986542be8673@linux.microsoft.com \
    --to=madvenka@linux.microsoft.com \
    --cc=broonie@kernel.org \
    --cc=catalin.marinas@arm.com \
    --cc=jmorris@namei.org \
    --cc=jpoimboe@redhat.com \
    --cc=jthierry@redhat.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=live-patching@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=pasha.tatashin@soleen.com \
    --cc=will@kernel.org \
    --subject='Re: [RFC PATCH v3 2/4] arm64: Check the return PC against unreliable code sections' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).