On Thu, Aug 01, 2024 at 01:06:51PM +0100, Mark Brown wrote:
> @@ -636,6 +639,81 @@ extern int restore_zt_context(struct user_ctxs *user);
>
> #endif /* ! CONFIG_ARM64_SME */
>
> +#ifdef CONFIG_ARM64_GCS
> +
> +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;
We discussed briefly offline. Not a problem in this patch but
gcs_preserve_current_state() only saves it conditionally on GCS being
enabled for the task. However, you mentioned that the register is always
available to the user, so I'd rather change the preserving function to
save it unconditionally.
> +
> + __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;
> +}
> +
> +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;
We don't allow enabling and that's fine but we don't restore gcspr
either with this early return.
> +
> + /* 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);
I think you should move this further up unconditionally.
--
Catalin
