On Tue, 01 Jul 2025 06:46:14 +0200
Florian Weimer <fweimer@xxxxxxxxxx> wrote:
> * Linus Torvalds:
>
> > On Mon, 30 Jun 2025 at 17:54, Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> >>
> >> + /* stack going in wrong direction? */
> >> + if (cfa <= state->sp)
> >> + goto done;
> >
> > I suspect this should do a lot more testing.
> >
> >> + /* Find the Return Address (RA) */
> >> + if (get_user(ra, (unsigned long *)(cfa + frame->ra_off)))
> >> + goto done;
> >> +
> >> + if (frame->fp_off && get_user(fp, (unsigned long __user *)(cfa + frame->fp_off)))
> >> + goto done;
> >
> > .. and this should check the frame for validity too. At a minimum it
> > should be properly aligned, but things like "it had better be above
> > the current frame" to avoid having some loop would seem to be a good
> > idea.
>
> I don't think SFrame as-is requires stacks to be contiguous. Maybe
> there could be a per-frame flag that indicates whether a stack switch is
> expected?
Looking at the current code of perf, it appears to only check that the
address is valid to read from user space. Perhaps that's the only check
needed here too?
Now this loop will not go into an infinite loop as the code has:
for_each_user_frame(&state) {
trace->entries[trace->nr++] = state.ip;
if (trace->nr >= max_entries)
break;
}
Where
#define for_each_user_frame(state) \
for (unwind_user_start((state)); !(state)->done; unwind_user_next((state)))
It will stop at "max_entries" even if the user space tries to make it go
forever. max_entries is either 511 (on 64 bit) or 1023 (on 32 bit), as it
is defined as:
/* Make the cache fit in a 4K page */
#define UNWIND_MAX_ENTRIES \
((SZ_4K - sizeof(struct unwind_cache)) / sizeof(long))
Now, perhaps we need to verify that the cfa is indeed canonical, but what
other test do we have perform?
In the future, this code will also be handling JIT and possibly interpreted
code. How much do we really want to limit the stack walking due to checks?
-- Steve
