From: Mark Brown <broonie@kernel.org> To: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> 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: Wed, 5 May 2021 17:34:06 +0100 [thread overview] Message-ID: <20210505163406.GB4541@sirena.org.uk> (raw) In-Reply-To: <1bd2b177-509a-21d9-e349-9b2388db45eb@linux.microsoft.com> [-- Attachment #1: Type: text/plain, Size: 1818 bytes --] On Tue, May 04, 2021 at 02:03:14PM -0500, Madhavan T. Venkataraman wrote: > On 5/4/21 11:05 AM, Mark Brown wrote: > >> @@ -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. That sounds reasonable but like I say should probably be called out in the code so it's clear to people working with it. > 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? I can't think of any cases where it wouldn't be right now, but it seems easier to just do a redundant check than to have the assumption in the code and have to think about if it's missing. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: Mark Brown <broonie@kernel.org> To: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> 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: Wed, 5 May 2021 17:34:06 +0100 [thread overview] Message-ID: <20210505163406.GB4541@sirena.org.uk> (raw) In-Reply-To: <1bd2b177-509a-21d9-e349-9b2388db45eb@linux.microsoft.com> [-- Attachment #1.1: Type: text/plain, Size: 1818 bytes --] On Tue, May 04, 2021 at 02:03:14PM -0500, Madhavan T. Venkataraman wrote: > On 5/4/21 11:05 AM, Mark Brown wrote: > >> @@ -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. That sounds reasonable but like I say should probably be called out in the code so it's clear to people working with it. > 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? I can't think of any cases where it wouldn't be right now, but it seems easier to just do a redundant check than to have the assumption in the code and have to think about if it's missing. [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-05-05 16:46 UTC|newest] Thread overview: 64+ 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 ` madvenka 2021-05-03 17:36 ` [RFC PATCH v3 1/4] arm64: Introduce stack " madvenka 2021-05-03 17:36 ` madvenka 2021-05-04 15:50 ` Mark Brown 2021-05-04 15:50 ` Mark Brown 2021-05-04 19:14 ` Madhavan T. Venkataraman 2021-05-04 19:14 ` Madhavan T. Venkataraman 2021-05-04 21:52 ` Josh Poimboeuf 2021-05-04 21:52 ` Josh Poimboeuf 2021-05-04 23:13 ` Madhavan T. Venkataraman 2021-05-04 23:13 ` Madhavan T. Venkataraman 2021-05-05 0:07 ` Josh Poimboeuf 2021-05-05 0:07 ` Josh Poimboeuf 2021-05-05 0:21 ` Madhavan T. Venkataraman 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-03 17:36 ` madvenka 2021-05-04 16:05 ` Mark Brown 2021-05-04 16:05 ` Mark Brown 2021-05-04 19:03 ` Madhavan T. Venkataraman 2021-05-04 19:03 ` Madhavan T. Venkataraman 2021-05-04 19:32 ` Madhavan T. Venkataraman 2021-05-04 19:32 ` Madhavan T. Venkataraman 2021-05-05 16:46 ` Mark Brown 2021-05-05 16:46 ` Mark Brown 2021-05-05 18:48 ` Madhavan T. Venkataraman 2021-05-05 18:48 ` Madhavan T. Venkataraman 2021-05-05 18:50 ` Madhavan T. Venkataraman 2021-05-05 18:50 ` Madhavan T. Venkataraman 2021-05-06 13:45 ` Mark Brown 2021-05-06 13:45 ` Mark Brown 2021-05-06 15:21 ` Madhavan T. Venkataraman 2021-05-06 15:21 ` Madhavan T. Venkataraman 2021-05-05 16:34 ` Mark Brown [this message] 2021-05-05 16:34 ` Mark Brown 2021-05-05 17:51 ` Madhavan T. Venkataraman 2021-05-05 17:51 ` Madhavan T. Venkataraman 2021-05-05 19:30 ` Ard Biesheuvel 2021-05-05 19:30 ` Ard Biesheuvel 2021-05-05 20:00 ` Madhavan T. Venkataraman 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-03 17:36 ` madvenka 2021-05-06 14:12 ` Mark Brown 2021-05-06 14:12 ` Mark Brown 2021-05-06 15:30 ` Madhavan T. Venkataraman 2021-05-06 15:30 ` Madhavan T. Venkataraman 2021-05-06 15:32 ` Madhavan T. Venkataraman 2021-05-06 15:32 ` Madhavan T. Venkataraman 2021-05-06 15:44 ` Mark Brown 2021-05-06 15:44 ` Mark Brown 2021-05-06 15:56 ` Madhavan T. Venkataraman 2021-05-06 15:56 ` Madhavan T. Venkataraman 2021-05-06 15:37 ` Mark Brown 2021-05-06 15:37 ` Mark Brown 2021-05-06 15:57 ` Madhavan T. Venkataraman 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-03 17:36 ` madvenka 2021-05-06 14:43 ` Mark Brown 2021-05-06 14:43 ` Mark Brown 2021-05-06 15:20 ` Madhavan T. Venkataraman 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=20210505163406.GB4541@sirena.org.uk \ --to=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=madvenka@linux.microsoft.com \ --cc=mark.rutland@arm.com \ --cc=pasha.tatashin@soleen.com \ --cc=will@kernel.org \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.