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
