amd64 db_trace: Reject unaligned frame pointers

[kib]

I do not object strongly, but two notes:

1. I think that the alignment is more required by hw, which auto-aligns %rsp on traps
2. What if we got a bug and stack becomes misaligned? Then ddb is less useful due to refusal to backtrace.

[jhb]

In D41532#946529, @kib wrote:

I do not object strongly, but two notes:

1. I think that the alignment is more required by hw, which auto-aligns %rsp on traps
2. What if we got a bug and stack becomes misaligned? Then ddb is less useful due to refusal to backtrace.

It is actually 2) that I'm worried about. If we get a frame that is misaligned, it is almost certainly garbage and dereferencing it may trigger a nested fault which is less useful. I think it's more likely if your frame pointer is misaligned that we've encountered a garbage frame that it isn't worth unwinding through due to the risk of a nested fault (but also low likelihood of getting useful information after the garbage).

On non-x86 architectures we have a bit more safety in that we can use kstack_contains, but that doesn't quite work on x86 due to the extra stacks used for double faults and machine checks, etc. We could maybe add an x86-specific helper for kstack validation that checks those additional stacks as well as kstack_contains and replace the INKERNEL checks with some of those for better protection. I still think a misaligned frame is unlikely to be useful to unwind through and is instead evidence of a corrupt stack?

[markj]

sys/amd64/amd64/db_trace.c

208–209

328

[kib]

As I mentioned, I do not object. But perhaps we can be extra verbose in this case and print a line stating that tracing aborted due to mis-alignment, not because we lost the stack.

[jhb]

In D41532#946750, @kib wrote:

As I mentioned, I do not object. But perhaps we can be extra verbose in this case and print a line stating that tracing aborted due to mis-alignment, not because we lost the stack.

Yes, we do that on arm64 and risc-v where we print "invalid" trapframe pointers.

[jhb]

Log invalid trapframe pointers

[kib]

Pedantically, having unaligned pointer to the type is UB. So might be, it is more future-proof against compiler brain-damage, to calculate the address as uintptr_t, then check alignment and INKERNEL (we cast to long anyway), and only then cast to trapframe *.

[jhb]

Use uintptr_t more

[kib]

sys/amd64/amd64/db_trace.c

236

There the uintptr_t is also needed, I believe. Should this function return unaligned frame address at all?

[jhb]

sys/amd64/amd64/db_trace.c

236

I thought about moving the aligned fp check in this function, but it makes it a bit messy. Alternatively, we could maybe return a uintptr_t or register_t value here instead of struct amd64_frame *

[kib]

sys/amd64/amd64/db_trace.c

236

I do not see anything wrong with returning integer type from the function. It should also reduce the number of type casts, it seems.

[jhb]

More db_addr_t

[jhb]

sys/amd64/amd64/db_trace.c

328

I think I could now move this alignment check into db_nextframe at the start (just return 0/0 like we do for an invalid trapframe) and then this entire block can go away as when the previous db_nextframe returns the misaligned frame address we would loop around and print the address and then the db_nextframe call that sees the unaligned frame would trigger the early exit.

[jhb]

sys/amd64/amd64/db_trace.c

217

We are careful to use db_get_value for the regular frames, but not for trapframe. Of course, most other arches don't use db_get_value either relying on kstack_contains checks instead.

[kib]

sys/amd64/amd64/db_trace.c

135

196

197

236

and in the prev line

320

All these casts to db_addr_t are now excessive, but IMO it is fine either way.

[jhb]

sys/amd64/amd64/db_trace.c

320

I do think they are a bit much, yes.