Re: [RFC PATCH v3 2/4] arm64: Check the return PC against unreliable code sections
From: Mark Brown <broonie@kernel.org>
Date: 2021-05-04 16:05:46
Also in:
live-patching, lkml
On Mon, May 03, 2021 at 12:36:13PM -0500, madvenka@linux.microsoft.com wrote:
From: "Madhavan T. Venkataraman" <redacted> 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.
+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().
+{+ 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.
quoted hunk ↗ jump to hunk
+ 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.