Re: [PATCH v7 26/39] arm64/ptrace: Expose GCS via ptrace and core files

Mark Brown broonie@kernel.org writes:

Provide a new register type NT_ARM_GCS reporting the current GCS mode and pointer for EL0. Due to the interactions with allocation and deallocation of Guarded Control Stacks we do not permit any changes to the GCS mode via ptrace, only GCSPR_EL0 may be changed.

The code allows disabling GCS. Is that unintended?

Signed-off-by: Mark Brown broonie@kernel.org

arch/arm64/include/uapi/asm/ptrace.h | 8 +++++ arch/arm64/kernel/ptrace.c | 59 ++++++++++++++++++++++++++++++++++++ include/uapi/linux/elf.h | 1 + 3 files changed, 68 insertions(+)

diff --git a/arch/arm64/include/uapi/asm/ptrace.h b/arch/arm64/include/uapi/asm/ptrace.h index 7fa2f7036aa7..0f39ba4f3efd 100644 --- a/arch/arm64/include/uapi/asm/ptrace.h +++ b/arch/arm64/include/uapi/asm/ptrace.h @@ -324,6 +324,14 @@ struct user_za_header { #define ZA_PT_SIZE(vq) \ (ZA_PT_ZA_OFFSET + ZA_PT_ZA_SIZE(vq)) +/* GCS state (NT_ARM_GCS) */

+struct user_gcs {

• __u64 features_enabled;
• __u64 features_locked;
• __u64 gcspr_el0;

+};

If there's a reserved field in sigframe's gcs_context, isn't it worth it to have a reserved field here as well?

#endif /* __ASSEMBLY__ */ #endif /* _UAPI__ASM_PTRACE_H */ diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c index 20d7ef82de90..f15b8e33561e 100644 --- a/arch/arm64/kernel/ptrace.c +++ b/arch/arm64/kernel/ptrace.c @@ -33,6 +33,7 @@ #include <asm/cpufeature.h> #include <asm/debug-monitors.h> #include <asm/fpsimd.h> +#include <asm/gcs.h> #include <asm/mte.h> #include <asm/pointer_auth.h> #include <asm/stacktrace.h> @@ -1409,6 +1410,51 @@ static int tagged_addr_ctrl_set(struct task_struct *target, const struct } #endif +#ifdef CONFIG_ARM64_GCS +static int gcs_get(struct task_struct *target,

•    const struct user_regset *regset,
  

•    struct membuf to)
  

+{

• struct user_gcs user_gcs;
• if (target == current)

• gcs_preserve_current_state();
  

• user_gcs.features_enabled = target->thread.gcs_el0_mode;
• user_gcs.features_locked = target->thread.gcs_el0_locked;
• user_gcs.gcspr_el0 = target->thread.gcspr_el0;
• return membuf_write(&to, &user_gcs, sizeof(user_gcs));

+}

+static int gcs_set(struct task_struct *target, const struct

•    user_regset *regset, unsigned int pos,
  

•    unsigned int count, const void *kbuf, const
  

•    void __user *ubuf)
  

+{

• int ret;
• struct user_gcs user_gcs;
• ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &user_gcs, 0, -1);
• if (ret)

• return ret;
  

• if (user_gcs.features_enabled & ~PR_SHADOW_STACK_SUPPORTED_STATUS_MASK)

• return -EINVAL;
  

• /* Do not allow enable via ptrace */
• if ((user_gcs.features_enabled & PR_SHADOW_STACK_ENABLE) &&

•    !!(target->thread.gcs_el0_mode & PR_SHADOW_STACK_ENABLE))
  

There should be only one '!' above.

Though contrary to the patch description, this code allows disabling GCS. Shouldn't we require that

(user_gcs.features_enabled & PR_SHADOW_STACK_ENABLE) == (target->thread.gcs_el0_mode & PR_SHADOW_STACK_ENABLE)

? That would ensure that the GCS mode can't be changed.

• return -EBUSY;
  

• target->thread.gcs_el0_mode = user_gcs.features_enabled;
• target->thread.gcs_el0_locked = user_gcs.features_locked;
• target->thread.gcspr_el0 = user_gcs.gcspr_el0;
• return 0;

+} +#endif