Page MenuHomeFreeBSD

Check that the frame pointer is within the current stack.
ClosedPublic

Authored by jhb on Aug 7 2020, 4:17 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, May 19, 6:37 PM
Unknown Object (File)
Fri, May 17, 7:51 AM
Unknown Object (File)
Thu, May 16, 1:50 PM
Unknown Object (File)
Mon, Apr 29, 8:05 PM
Unknown Object (File)
Mon, Apr 29, 8:03 PM
Unknown Object (File)
Mon, Apr 29, 8:03 PM
Unknown Object (File)
Sun, Apr 28, 6:09 PM
Unknown Object (File)
Apr 19 2024, 9:18 AM
Subscribers

Details

Summary

This same check is used on other architectures. Previously this would
permit a stack frame to unwind into any arbitrary kernel address
(including unmapped addresses).

Test Plan
  • in a CHERI kernel where the stack uses a pointer with bounds, this was faulting when it tried to read off the end of the stack inside ddb

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jhb requested review of this revision.Aug 7 2020, 4:17 PM
jhb created this revision.
mhorne added a subscriber: trasz.

I was investigating a panic recently encountered on the Jenkins run of the test suite, and ended up with an identical patch.

While I haven't had any luck reproducing the issue, the excerpt (P412) shows recursive panics in unwind_frame(), so I strongly suspect this is the cause.

Note that arm64 might benefit from this check as well, but that code has been in widespread use for a while now and has seemingly never encountered this.

This revision is now accepted and ready to land.Aug 8 2020, 9:17 PM

I was investigating a panic recently encountered on the Jenkins run of the test suite, and ended up with an identical patch.

While I haven't had any luck reproducing the issue, the excerpt (P412) shows recursive panics in unwind_frame(), so I strongly suspect this is the cause.

Note that arm64 might benefit from this check as well, but that code has been in widespread use for a while now and has seemingly never encountered this.

arm64 almost certainly needs the same check. Tagging @andrew so he sees this.

On arm64 we have the following in fork_trampoline to stop unwinding past the top of the stack.

mov     fp, #0  /* Stack traceback stops here. */

On arm64 we have the following in fork_trampoline to stop unwinding past the top of the stack.

mov     fp, #0  /* Stack traceback stops here. */

Yes, but you will want to validate that the frame pointer is contained in td_kstack and not random garbage. It is much nicer to have ddb abort the trace when the frame pointer goes off in the weeds due to the stack being trashed than to have it do a nested fault in ddb, or worse, do a fault in stack_capture() that turns into a kernel panic.