On Thu, Aug 22, 2024 at 02:15:28AM +0100, Mark Brown wrote: > +static int preserve_gcs_context(struct gcs_context __user *ctx) > +{ > + int err = 0; > + u64 gcspr; > + > + /* > + * We will add a cap token to the frame, include it in the > + * GCSPR_EL0 we report to support stack switching via > + * sigreturn. > + */ > + gcs_preserve_current_state(); > + gcspr = current->thread.gcspr_el0 - 8; > + > + __put_user_error(GCS_MAGIC, &ctx->head.magic, err); > + __put_user_error(sizeof(*ctx), &ctx->head.size, err); > + __put_user_error(gcspr, &ctx->gcspr, err); > + __put_user_error(0, &ctx->reserved, err); > + __put_user_error(current->thread.gcs_el0_mode, > + &ctx->features_enabled, err); > + > + return 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. Also in gcs_signal_entry() in the previous patch, we seem to subtract 16 rather than 8. I admit I haven't checked the past discussions in this area, so maybe I'm missing something. > +static int restore_gcs_context(struct user_ctxs *user) > +{ > + u64 gcspr, enabled; > + int err = 0; > + > + if (user->gcs_size != sizeof(*user->gcs)) > + return -EINVAL; > + > + __get_user_error(gcspr, &user->gcs->gcspr, err); > + __get_user_error(enabled, &user->gcs->features_enabled, err); > + if (err) > + return err; > + > + /* Don't allow unknown modes */ > + if (enabled & ~PR_SHADOW_STACK_SUPPORTED_STATUS_MASK) > + return -EINVAL; > + > + err = gcs_check_locked(current, enabled); > + if (err != 0) > + return err; > + > + /* Don't allow enabling */ > + if (!task_gcs_el0_enabled(current) && > + (enabled & PR_SHADOW_STACK_ENABLE)) > + return -EINVAL; > + > + /* If we are disabling disable everything */ > + if (!(enabled & PR_SHADOW_STACK_ENABLE)) > + enabled = 0; > + > + current->thread.gcs_el0_mode = enabled; > + > + /* > + * 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? 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. > @@ -977,6 +1079,13 @@ static int setup_sigframe_layout(struct rt_sigframe_user_layout *user, > return err; > } > > + 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. -- Catalin