| Message ID | 20210315165800.5948-5-madvenka@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: Implement reliable stack trace | expand |
On Mon, Mar 15, 2021 at 11:57:56AM -0500, madvenka@linux.microsoft.com wrote: > From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> > > EL1 exceptions can happen on any instruction including instructions in > the frame pointer prolog or epilog. Depending on where exactly they happen, > they could render the stack trace unreliable. > > If an EL1 exception frame is found on the stack, mark the stack trace as > unreliable. > > Now, the EL1 exception frame is not at any well-known offset on the stack. > It can be anywhere on the stack. In order to properly detect an EL1 > exception frame the following checks must be done: > > - The frame type must be EL1_FRAME. > > - When the register state is saved in the EL1 pt_regs, the frame > pointer x29 is saved in pt_regs->regs[29] and the return PC > is saved in pt_regs->pc. These must match with the current > frame. Before you can do this, you need to reliably identify that you have a pt_regs on the stack, but this patch uses a heuristic, which is not reliable. However, instead you can identify whether you're trying to unwind through one of the EL1 entry functions, which tells you the same thing without even having to look at the pt_regs. We can do that based on the entry functions all being in .entry.text, which we could further sub-divide to split the EL0 and EL1 entry functions. > > Interrupts encountered in kernel code are also EL1 exceptions. At the end > of an interrupt, the interrupt handler checks if the current task must be > preempted for any reason. If so, it calls the preemption code which takes > the task off the CPU. A stack trace taken on the task after the preemption > will show the EL1 frame and will be considered unreliable. This is correct > behavior as preemption can happen practically at any point in code > including the frame pointer prolog and epilog. > > Breakpoints encountered in kernel code are also EL1 exceptions. The probing > infrastructure uses breakpoints for executing probe code. While in the probe > code, the stack trace will show an EL1 frame and will be considered > unreliable. This is also correct behavior. > > Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com> > --- > arch/arm64/include/asm/stacktrace.h | 2 + > arch/arm64/kernel/stacktrace.c | 57 +++++++++++++++++++++++++++++ > 2 files changed, 59 insertions(+) > > diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h > index eb29b1fe8255..684f65808394 100644 > --- a/arch/arm64/include/asm/stacktrace.h > +++ b/arch/arm64/include/asm/stacktrace.h > @@ -59,6 +59,7 @@ struct stackframe { > #ifdef CONFIG_FUNCTION_GRAPH_TRACER > int graph; > #endif > + bool reliable; > }; > > extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame); > @@ -169,6 +170,7 @@ static inline void start_backtrace(struct stackframe *frame, > bitmap_zero(frame->stacks_done, __NR_STACK_TYPES); > frame->prev_fp = 0; > frame->prev_type = STACK_TYPE_UNKNOWN; > + frame->reliable = true; > } > > #endif /* __ASM_STACKTRACE_H */ > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c > index 504cd161339d..6ae103326f7b 100644 > --- a/arch/arm64/kernel/stacktrace.c > +++ b/arch/arm64/kernel/stacktrace.c > @@ -18,6 +18,58 @@ > #include <asm/stack_pointer.h> > #include <asm/stacktrace.h> > > +static void check_if_reliable(unsigned long fp, struct stackframe *frame, > + struct stack_info *info) > +{ > + struct pt_regs *regs; > + unsigned long regs_start, regs_end; > + > + /* > + * If the stack trace has already been marked unreliable, just > + * return. > + */ > + if (!frame->reliable) > + return; > + > + /* > + * Assume that this is an intermediate marker frame inside a pt_regs > + * structure created on the stack and get the pt_regs pointer. Other > + * checks will be done below to make sure that this is a marker > + * frame. > + */ Sorry, but NAK to this approach specifically. This isn't reliable (since it can be influenced by arbitrary data on the stack), and it's far more complicated than identifying the entry functions specifically. Thanks, Mark. > + regs_start = fp - offsetof(struct pt_regs, stackframe); > + if (regs_start < info->low) > + return; > + regs_end = regs_start + sizeof(*regs); > + if (regs_end > info->high) > + return; > + regs = (struct pt_regs *) regs_start; > + > + /* > + * When an EL1 exception happens, a pt_regs structure is created > + * on the stack and the register state is recorded. Part of the > + * state is the FP and PC at the time of the exception. > + * > + * In addition, the FP and PC are also stored in pt_regs->stackframe > + * and pt_regs->stackframe is chained with other frames on the stack. > + * This is so that the interrupted function shows up in the stack > + * trace. > + * > + * The exception could have happened during the frame pointer > + * prolog or epilog. This could result in a missing frame in > + * the stack trace so that the caller of the interrupted > + * function does not show up in the stack trace. > + * > + * So, mark the stack trace as unreliable if an EL1 frame is > + * detected. > + */ > + if (regs->frame_type == EL1_FRAME && regs->pc == frame->pc && > + regs->regs[29] == frame->fp) { > + frame->reliable = false; > + return; > + } > +} > + > /* > * AArch64 PCS assigns the frame pointer to x29. > * > @@ -114,6 +166,11 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame) > > frame->pc = ptrauth_strip_insn_pac(frame->pc); > > + /* > + * Check for features that render the stack trace unreliable. > + */ > + check_if_reliable(fp, frame, &info); > + > return 0; > } > NOKPROBE_SYMBOL(unwind_frame); > -- > 2.25.1 >
On 3/23/21 5:42 AM, Mark Rutland wrote: > On Mon, Mar 15, 2021 at 11:57:56AM -0500, madvenka@linux.microsoft.com wrote: >> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> >> >> EL1 exceptions can happen on any instruction including instructions in >> the frame pointer prolog or epilog. Depending on where exactly they happen, >> they could render the stack trace unreliable. >> >> If an EL1 exception frame is found on the stack, mark the stack trace as >> unreliable. >> >> Now, the EL1 exception frame is not at any well-known offset on the stack. >> It can be anywhere on the stack. In order to properly detect an EL1 >> exception frame the following checks must be done: >> >> - The frame type must be EL1_FRAME. >> >> - When the register state is saved in the EL1 pt_regs, the frame >> pointer x29 is saved in pt_regs->regs[29] and the return PC >> is saved in pt_regs->pc. These must match with the current >> frame. > > Before you can do this, you need to reliably identify that you have a > pt_regs on the stack, but this patch uses a heuristic, which is not > reliable. > > However, instead you can identify whether you're trying to unwind > through one of the EL1 entry functions, which tells you the same thing > without even having to look at the pt_regs. > > We can do that based on the entry functions all being in .entry.text, > which we could further sub-divide to split the EL0 and EL1 entry > functions. > Yes. I will check the entry functions. But I still think that we should not rely on just one check. The additional checks will make it robust. So, I suggest that the return address be checked first. If that passes, then we can be reasonably sure that there are pt_regs. Then, check the other things in pt_regs. >> >> Interrupts encountered in kernel code are also EL1 exceptions. At the end >> of an interrupt, the interrupt handler checks if the current task must be >> preempted for any reason. If so, it calls the preemption code which takes >> the task off the CPU. A stack trace taken on the task after the preemption >> will show the EL1 frame and will be considered unreliable. This is correct >> behavior as preemption can happen practically at any point in code >> including the frame pointer prolog and epilog. >> >> Breakpoints encountered in kernel code are also EL1 exceptions. The probing >> infrastructure uses breakpoints for executing probe code. While in the probe >> code, the stack trace will show an EL1 frame and will be considered >> unreliable. This is also correct behavior. >> >> Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com> >> --- >> arch/arm64/include/asm/stacktrace.h | 2 + >> arch/arm64/kernel/stacktrace.c | 57 +++++++++++++++++++++++++++++ >> 2 files changed, 59 insertions(+) >> >> diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h >> index eb29b1fe8255..684f65808394 100644 >> --- a/arch/arm64/include/asm/stacktrace.h >> +++ b/arch/arm64/include/asm/stacktrace.h >> @@ -59,6 +59,7 @@ struct stackframe { >> #ifdef CONFIG_FUNCTION_GRAPH_TRACER >> int graph; >> #endif >> + bool reliable; >> }; >> >> extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame); >> @@ -169,6 +170,7 @@ static inline void start_backtrace(struct stackframe *frame, >> bitmap_zero(frame->stacks_done, __NR_STACK_TYPES); >> frame->prev_fp = 0; >> frame->prev_type = STACK_TYPE_UNKNOWN; >> + frame->reliable = true; >> } >> >> #endif /* __ASM_STACKTRACE_H */ >> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c >> index 504cd161339d..6ae103326f7b 100644 >> --- a/arch/arm64/kernel/stacktrace.c >> +++ b/arch/arm64/kernel/stacktrace.c >> @@ -18,6 +18,58 @@ >> #include <asm/stack_pointer.h> >> #include <asm/stacktrace.h> >> >> +static void check_if_reliable(unsigned long fp, struct stackframe *frame, >> + struct stack_info *info) >> +{ >> + struct pt_regs *regs; >> + unsigned long regs_start, regs_end; >> + >> + /* >> + * If the stack trace has already been marked unreliable, just >> + * return. >> + */ >> + if (!frame->reliable) >> + return; >> + >> + /* >> + * Assume that this is an intermediate marker frame inside a pt_regs >> + * structure created on the stack and get the pt_regs pointer. Other >> + * checks will be done below to make sure that this is a marker >> + * frame. >> + */ > > Sorry, but NAK to this approach specifically. This isn't reliable (since > it can be influenced by arbitrary data on the stack), and it's far more > complicated than identifying the entry functions specifically. > As I mentioned above, I agree that we should check the return address. But just as a precaution, I think we should double check the pt_regs. Is that OK with you? It does not take away anything or increase the risk in anyway. I think it makes it more robust. Thanks. Madhavan
On Tue, Mar 23, 2021 at 07:46:10AM -0500, Madhavan T. Venkataraman wrote: > On 3/23/21 5:42 AM, Mark Rutland wrote: > > On Mon, Mar 15, 2021 at 11:57:56AM -0500, madvenka@linux.microsoft.com wrote: > >> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> > >> > >> EL1 exceptions can happen on any instruction including instructions in > >> the frame pointer prolog or epilog. Depending on where exactly they happen, > >> they could render the stack trace unreliable. > >> > >> If an EL1 exception frame is found on the stack, mark the stack trace as > >> unreliable. > >> > >> Now, the EL1 exception frame is not at any well-known offset on the stack. > >> It can be anywhere on the stack. In order to properly detect an EL1 > >> exception frame the following checks must be done: > >> > >> - The frame type must be EL1_FRAME. > >> > >> - When the register state is saved in the EL1 pt_regs, the frame > >> pointer x29 is saved in pt_regs->regs[29] and the return PC > >> is saved in pt_regs->pc. These must match with the current > >> frame. > > > > Before you can do this, you need to reliably identify that you have a > > pt_regs on the stack, but this patch uses a heuristic, which is not > > reliable. > > > > However, instead you can identify whether you're trying to unwind > > through one of the EL1 entry functions, which tells you the same thing > > without even having to look at the pt_regs. > > > > We can do that based on the entry functions all being in .entry.text, > > which we could further sub-divide to split the EL0 and EL1 entry > > functions. > > Yes. I will check the entry functions. But I still think that we should > not rely on just one check. The additional checks will make it robust. > So, I suggest that the return address be checked first. If that passes, > then we can be reasonably sure that there are pt_regs. Then, check > the other things in pt_regs. What do you think this will catch? The only way to correctly identify whether or not we have a pt_regs is to check whether we're in specific portions of the EL1 entry assembly where the regs exist. However, as any EL1<->EL1 transition cannot be safely unwound, we'd mark any trace going through the EL1 entry assembly as unreliable. Given that, I don't think it's useful to check the regs, and I'd prefer to avoid the subtlteties involved in attempting to do so. [...] > >> +static void check_if_reliable(unsigned long fp, struct stackframe *frame, > >> + struct stack_info *info) > >> +{ > >> + struct pt_regs *regs; > >> + unsigned long regs_start, regs_end; > >> + > >> + /* > >> + * If the stack trace has already been marked unreliable, just > >> + * return. > >> + */ > >> + if (!frame->reliable) > >> + return; > >> + > >> + /* > >> + * Assume that this is an intermediate marker frame inside a pt_regs > >> + * structure created on the stack and get the pt_regs pointer. Other > >> + * checks will be done below to make sure that this is a marker > >> + * frame. > >> + */ > > > > Sorry, but NAK to this approach specifically. This isn't reliable (since > > it can be influenced by arbitrary data on the stack), and it's far more > > complicated than identifying the entry functions specifically. > > As I mentioned above, I agree that we should check the return address. But > just as a precaution, I think we should double check the pt_regs. > > Is that OK with you? It does not take away anything or increase the risk in > anyway. I think it makes it more robust. As above, I think that the work necessary to correctly access the regs means that it's not helpful to check the regs themselves. If you have something in mind where checking the regs is helpful I'm happy to consider that, but my general preference would be to stay away from the regs for now. Thanks, Mark.
On 3/23/21 8:04 AM, Mark Rutland wrote: > On Tue, Mar 23, 2021 at 07:46:10AM -0500, Madhavan T. Venkataraman wrote: >> On 3/23/21 5:42 AM, Mark Rutland wrote: >>> On Mon, Mar 15, 2021 at 11:57:56AM -0500, madvenka@linux.microsoft.com wrote: >>>> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> >>>> >>>> EL1 exceptions can happen on any instruction including instructions in >>>> the frame pointer prolog or epilog. Depending on where exactly they happen, >>>> they could render the stack trace unreliable. >>>> >>>> If an EL1 exception frame is found on the stack, mark the stack trace as >>>> unreliable. >>>> >>>> Now, the EL1 exception frame is not at any well-known offset on the stack. >>>> It can be anywhere on the stack. In order to properly detect an EL1 >>>> exception frame the following checks must be done: >>>> >>>> - The frame type must be EL1_FRAME. >>>> >>>> - When the register state is saved in the EL1 pt_regs, the frame >>>> pointer x29 is saved in pt_regs->regs[29] and the return PC >>>> is saved in pt_regs->pc. These must match with the current >>>> frame. >>> >>> Before you can do this, you need to reliably identify that you have a >>> pt_regs on the stack, but this patch uses a heuristic, which is not >>> reliable. >>> >>> However, instead you can identify whether you're trying to unwind >>> through one of the EL1 entry functions, which tells you the same thing >>> without even having to look at the pt_regs. >>> >>> We can do that based on the entry functions all being in .entry.text, >>> which we could further sub-divide to split the EL0 and EL1 entry >>> functions. >> >> Yes. I will check the entry functions. But I still think that we should >> not rely on just one check. The additional checks will make it robust. >> So, I suggest that the return address be checked first. If that passes, >> then we can be reasonably sure that there are pt_regs. Then, check >> the other things in pt_regs. > > What do you think this will catch? > I am not sure that I have an exact example to mention here. But I will attempt one. Let us say that a task has called arch_stack_walk() in the recent past. The unwinder may have copied a stack frame onto some location in the stack with one of the return addresses we check. Let us assume that there is some stack corruption that makes a frame pointer point to that exact record. Now, we will get a match on the return address on the next unwind. Pardon me if the example is somewhat crude. My point is that it is highly unlikely but not impossible for the return address to be on the stack and for the unwinder to get an unfortunate match. > The only way to correctly identify whether or not we have a pt_regs is > to check whether we're in specific portions of the EL1 entry assembly > where the regs exist. However, as any EL1<->EL1 transition cannot be > safely unwound, we'd mark any trace going through the EL1 entry assembly > as unreliable. > > Given that, I don't think it's useful to check the regs, and I'd prefer > to avoid the subtlteties involved in attempting to do so. > I agree that the return address check is a good check. I would like to add extra checks to be absolutely sure. > [...] > >>>> +static void check_if_reliable(unsigned long fp, struct stackframe *frame, >>>> + struct stack_info *info) >>>> +{ >>>> + struct pt_regs *regs; >>>> + unsigned long regs_start, regs_end; >>>> + >>>> + /* >>>> + * If the stack trace has already been marked unreliable, just >>>> + * return. >>>> + */ >>>> + if (!frame->reliable) >>>> + return; >>>> + >>>> + /* >>>> + * Assume that this is an intermediate marker frame inside a pt_regs >>>> + * structure created on the stack and get the pt_regs pointer. Other >>>> + * checks will be done below to make sure that this is a marker >>>> + * frame. >>>> + */ >>> >>> Sorry, but NAK to this approach specifically. This isn't reliable (since >>> it can be influenced by arbitrary data on the stack), and it's far more >>> complicated than identifying the entry functions specifically. >> >> As I mentioned above, I agree that we should check the return address. But >> just as a precaution, I think we should double check the pt_regs. >> >> Is that OK with you? It does not take away anything or increase the risk in >> anyway. I think it makes it more robust. > > As above, I think that the work necessary to correctly access the regs > means that it's not helpful to check the regs themselves. If you have > something in mind where checking the regs is helpful I'm happy to > consider that, but my general preference would be to stay away from the > regs for now. > I have mentioned a possibility above. Please take a look and let me know. Thanks. Madhavan
On Tue, Mar 23, 2021 at 08:31:50AM -0500, Madhavan T. Venkataraman wrote: > On 3/23/21 8:04 AM, Mark Rutland wrote: > > On Tue, Mar 23, 2021 at 07:46:10AM -0500, Madhavan T. Venkataraman wrote: > >> On 3/23/21 5:42 AM, Mark Rutland wrote: > >>> On Mon, Mar 15, 2021 at 11:57:56AM -0500, madvenka@linux.microsoft.com wrote: > >>>> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> > >>>> > >>>> EL1 exceptions can happen on any instruction including instructions in > >>>> the frame pointer prolog or epilog. Depending on where exactly they happen, > >>>> they could render the stack trace unreliable. > >>>> > >>>> If an EL1 exception frame is found on the stack, mark the stack trace as > >>>> unreliable. > >>>> > >>>> Now, the EL1 exception frame is not at any well-known offset on the stack. > >>>> It can be anywhere on the stack. In order to properly detect an EL1 > >>>> exception frame the following checks must be done: > >>>> > >>>> - The frame type must be EL1_FRAME. > >>>> > >>>> - When the register state is saved in the EL1 pt_regs, the frame > >>>> pointer x29 is saved in pt_regs->regs[29] and the return PC > >>>> is saved in pt_regs->pc. These must match with the current > >>>> frame. > >>> > >>> Before you can do this, you need to reliably identify that you have a > >>> pt_regs on the stack, but this patch uses a heuristic, which is not > >>> reliable. > >>> > >>> However, instead you can identify whether you're trying to unwind > >>> through one of the EL1 entry functions, which tells you the same thing > >>> without even having to look at the pt_regs. > >>> > >>> We can do that based on the entry functions all being in .entry.text, > >>> which we could further sub-divide to split the EL0 and EL1 entry > >>> functions. > >> > >> Yes. I will check the entry functions. But I still think that we should > >> not rely on just one check. The additional checks will make it robust. > >> So, I suggest that the return address be checked first. If that passes, > >> then we can be reasonably sure that there are pt_regs. Then, check > >> the other things in pt_regs. > > > > What do you think this will catch? > > I am not sure that I have an exact example to mention here. But I will attempt > one. Let us say that a task has called arch_stack_walk() in the recent past. > The unwinder may have copied a stack frame onto some location in the stack > with one of the return addresses we check. Let us assume that there is some > stack corruption that makes a frame pointer point to that exact record. Now, > we will get a match on the return address on the next unwind. I don't see how this is material to the pt_regs case, as either: * When the unwinder considers this frame, it appears to be in the middle of an EL1 entry function, and the unwinder must mark the unwinding as unreliable regardless of the contents of any regs (so there's no need to look at the regs). * When the unwinder considers this frame, it does not appear to be in the middle of an EL1 entry function, so the unwinder does not think there are any regs to consider, and so we cannot detect this case. ... unless I've misunderstood the example? There's a general problem that it's possible to corrupt any portion of the chain to skip records, e.g. A -> B -> C -> D -> E -> F -> G -> H -> [final] ... could get corrupted to: A -> B -> D -> H -> [final] ... regardless of whether C/E/F/G had associated pt_regs. AFAICT there's no good way to catch this generally unless we have additional metadata to check the unwinding against. The likelihood of this happening without triggering other checks is vanishingly low, and as we don't have a reliable mechanism for detecting this, I don't think it's worthwhile attempting to do so. If and when we try to unwind across EL1 exception boundaries, the potential mismatch between the frame record and regs will be more significant, and I agree at that point thisd will need more thought. > Pardon me if the example is somewhat crude. My point is that it is > highly unlikely but not impossible for the return address to be on the > stack and for the unwinder to get an unfortunate match. I agree that this is possible in theory, but as above I don't think this is a practical concern. Thanks, Mark.
On 3/23/21 9:33 AM, Mark Rutland wrote: > On Tue, Mar 23, 2021 at 08:31:50AM -0500, Madhavan T. Venkataraman wrote: >> On 3/23/21 8:04 AM, Mark Rutland wrote: >>> On Tue, Mar 23, 2021 at 07:46:10AM -0500, Madhavan T. Venkataraman wrote: >>>> On 3/23/21 5:42 AM, Mark Rutland wrote: >>>>> On Mon, Mar 15, 2021 at 11:57:56AM -0500, madvenka@linux.microsoft.com wrote: >>>>>> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> >>>>>> >>>>>> EL1 exceptions can happen on any instruction including instructions in >>>>>> the frame pointer prolog or epilog. Depending on where exactly they happen, >>>>>> they could render the stack trace unreliable. >>>>>> >>>>>> If an EL1 exception frame is found on the stack, mark the stack trace as >>>>>> unreliable. >>>>>> >>>>>> Now, the EL1 exception frame is not at any well-known offset on the stack. >>>>>> It can be anywhere on the stack. In order to properly detect an EL1 >>>>>> exception frame the following checks must be done: >>>>>> >>>>>> - The frame type must be EL1_FRAME. >>>>>> >>>>>> - When the register state is saved in the EL1 pt_regs, the frame >>>>>> pointer x29 is saved in pt_regs->regs[29] and the return PC >>>>>> is saved in pt_regs->pc. These must match with the current >>>>>> frame. >>>>> >>>>> Before you can do this, you need to reliably identify that you have a >>>>> pt_regs on the stack, but this patch uses a heuristic, which is not >>>>> reliable. >>>>> >>>>> However, instead you can identify whether you're trying to unwind >>>>> through one of the EL1 entry functions, which tells you the same thing >>>>> without even having to look at the pt_regs. >>>>> >>>>> We can do that based on the entry functions all being in .entry.text, >>>>> which we could further sub-divide to split the EL0 and EL1 entry >>>>> functions. >>>> >>>> Yes. I will check the entry functions. But I still think that we should >>>> not rely on just one check. The additional checks will make it robust. >>>> So, I suggest that the return address be checked first. If that passes, >>>> then we can be reasonably sure that there are pt_regs. Then, check >>>> the other things in pt_regs. >>> >>> What do you think this will catch? >> >> I am not sure that I have an exact example to mention here. But I will attempt >> one. Let us say that a task has called arch_stack_walk() in the recent past. >> The unwinder may have copied a stack frame onto some location in the stack >> with one of the return addresses we check. Let us assume that there is some >> stack corruption that makes a frame pointer point to that exact record. Now, >> we will get a match on the return address on the next unwind. > > I don't see how this is material to the pt_regs case, as either: > > * When the unwinder considers this frame, it appears to be in the middle > of an EL1 entry function, and the unwinder must mark the unwinding as > unreliable regardless of the contents of any regs (so there's no need > to look at the regs). > > * When the unwinder considers this frame, it does not appear to be in > the middle of an EL1 entry function, so the unwinder does not think > there are any regs to consider, and so we cannot detect this case. > > ... unless I've misunderstood the example? > > There's a general problem that it's possible to corrupt any portion of > the chain to skip records, e.g. > > A -> B -> C -> D -> E -> F -> G -> H -> [final] > > ... could get corrupted to: > > A -> B -> D -> H -> [final] > > ... regardless of whether C/E/F/G had associated pt_regs. AFAICT there's > no good way to catch this generally unless we have additional metadata > to check the unwinding against. > > The likelihood of this happening without triggering other checks is > vanishingly low, and as we don't have a reliable mechanism for detecting > this, I don't think it's worthwhile attempting to do so. > > If and when we try to unwind across EL1 exception boundaries, the > potential mismatch between the frame record and regs will be more > significant, and I agree at that point thisd will need more thought. > >> Pardon me if the example is somewhat crude. My point is that it is >> highly unlikely but not impossible for the return address to be on the >> stack and for the unwinder to get an unfortunate match. > > I agree that this is possible in theory, but as above I don't think this > is a practical concern. > OK. What you say makes sense. Thanks. Madhavan
diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h index eb29b1fe8255..684f65808394 100644 --- a/arch/arm64/include/asm/stacktrace.h +++ b/arch/arm64/include/asm/stacktrace.h @@ -59,6 +59,7 @@ struct stackframe { #ifdef CONFIG_FUNCTION_GRAPH_TRACER int graph; #endif + bool reliable; }; extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame); @@ -169,6 +170,7 @@ static inline void start_backtrace(struct stackframe *frame, bitmap_zero(frame->stacks_done, __NR_STACK_TYPES); frame->prev_fp = 0; frame->prev_type = STACK_TYPE_UNKNOWN; + frame->reliable = true; } #endif /* __ASM_STACKTRACE_H */ diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c index 504cd161339d..6ae103326f7b 100644 --- a/arch/arm64/kernel/stacktrace.c +++ b/arch/arm64/kernel/stacktrace.c @@ -18,6 +18,58 @@ #include <asm/stack_pointer.h> #include <asm/stacktrace.h> +static void check_if_reliable(unsigned long fp, struct stackframe *frame, + struct stack_info *info) +{ + struct pt_regs *regs; + unsigned long regs_start, regs_end; + + /* + * If the stack trace has already been marked unreliable, just + * return. + */ + if (!frame->reliable) + return; + + /* + * Assume that this is an intermediate marker frame inside a pt_regs + * structure created on the stack and get the pt_regs pointer. Other + * checks will be done below to make sure that this is a marker + * frame. + */ + regs_start = fp - offsetof(struct pt_regs, stackframe); + if (regs_start < info->low) + return; + regs_end = regs_start + sizeof(*regs); + if (regs_end > info->high) + return; + regs = (struct pt_regs *) regs_start; + + /* + * When an EL1 exception happens, a pt_regs structure is created + * on the stack and the register state is recorded. Part of the + * state is the FP and PC at the time of the exception. + * + * In addition, the FP and PC are also stored in pt_regs->stackframe + * and pt_regs->stackframe is chained with other frames on the stack. + * This is so that the interrupted function shows up in the stack + * trace. + * + * The exception could have happened during the frame pointer + * prolog or epilog. This could result in a missing frame in + * the stack trace so that the caller of the interrupted + * function does not show up in the stack trace. + * + * So, mark the stack trace as unreliable if an EL1 frame is + * detected. + */ + if (regs->frame_type == EL1_FRAME && regs->pc == frame->pc && + regs->regs[29] == frame->fp) { + frame->reliable = false; + return; + } +} + /* * AArch64 PCS assigns the frame pointer to x29. * @@ -114,6 +166,11 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame) frame->pc = ptrauth_strip_insn_pac(frame->pc); + /* + * Check for features that render the stack trace unreliable. + */ + check_if_reliable(fp, frame, &info); + return 0; } NOKPROBE_SYMBOL(unwind_frame);