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.
Attachment:
signature.asc
Description: PGP signature
