On Fri, Aug 23, 2024 at 10:37:19AM +0100, Catalin Marinas wrote:
On Thu, Aug 22, 2024 at 02:15:28AM +0100, Mark Brown wrote:
- gcs_preserve_current_state();
- gcspr = current->thread.gcspr_el0 - 8;
- __put_user_error(gcspr, &ctx->gcspr, err);
Do we actually need to store the gcspr value after the cap token has been pushed or just the value of the interrupted context? If we at some point get a sigaltshadowstack() syscall, the saved GCS wouldn't point to the new stack but rather the original one. Unwinders should be able to get the actual GCSPR_EL0 register, no need for the sigcontext to point to the new shadow stack.
We could store either the cap token or the interrupted GCSPR_EL0 (the address below the cap token). It felt more joined up to go with the cap token since notionally signal return is consuming the cap token but either way would work, we could just add an offset when looking at the pointer.
Also in gcs_signal_entry() in the previous patch, we seem to subtract 16 rather than 8.
We need to not only place a cap but also a GCS frame for the sigreturn trampoline, the sigreturn trampoline isn't part of the interrupted context so isn't included in the signal frame but it needs to have a record on the GCS so that the signal handler doesn't just generate a GCS fault if it tries to return to the trampoline. This means that the GCSPR_EL0 that is set for the signal handler needs to move two entries, one for the cap token and one for the trampoline.
What I find confusing is that both restore_gcs_context() and gcs_restore_signal() seem to touch current->thread.gcspr_el0 and the sysreg. Which one takes priority? I should probably check the branch out to see the end result.
restore_gcs_context() is loading values from the signal frame in memory (which will only happen if a GCS context is present) then gcs_restore_signal() consumes the token at the top of the stack. The split is because userspace can skip the restore_X_context() functions for the optional signal frame elements by removing them from the context but we want to ensure that we always consume a token.
- /*
* We let userspace set GCSPR_EL0 to anything here, we will* validate later in gcs_restore_signal().*/- current->thread.gcspr_el0 = gcspr;
- write_sysreg_s(current->thread.gcspr_el0, SYS_GCSPR_EL0);
So in preserve_gcs_context(), we subtract 8 from the gcspr_el0 value. Where is it added back?
When we consumed the GCS cap token.
- if (add_all || task_gcs_el0_enabled(current)) {
err = sigframe_alloc(user, &user->gcs_offset,sizeof(struct gcs_context));if (err)return err;- }
I'm still not entirely convinced of this conditional saving and the interaction with unwinders. In a previous thread you mentioned that we need to keep the GCSPR_EL0 sysreg value up to date even after disabling GCS for a thread as not to confuse the unwinders. We could get a signal delivered together with a sigreturn without any context switch. Do we lose any state?
It might help if you describe the scenario, maybe even adding a comment in the code, otherwise I'm sure we'll forget in a few months time.
We should probably just change that back to saving unconditionally - it looks like the decision on worrying about overflowing the default signal frame is that we just shouldn't.