Re: [PATCH v10 25/40] arm64/ptrace: Expose GCS via ptrace and core files

Mark Brown
2024-08-21T18:28:08Z

[-- Attachment #1: Type: text/plain, Size: 2120 bytes --]

On Wed, Aug 21, 2024 at 06:57:16PM +0100, Catalin Marinas wrote:
> On Thu, Aug 01, 2024 at 01:06:52PM +0100, Mark Brown wrote:

> > + 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))
> > + 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;

> As in the previous thread, I thought we need to restore GCSPR_EL0
> unconditionally.

My thinking here is that if we return an error code then ideally we
shouldn't implement any changes, especially in cases like this one
where we're rejecting inputs that are just invalid. That seems like the
least surprising outcome and what we should strive for, even if it's too
complicated for some cases. It is possible to write GCSPR_EL0
regardless of if GCS is enabled, it's just not possible to write it as
part of an otherwise invalid write. The validation is checking for
unknown features and enables. With clone3() we could relax the enable
check, but I've just pulled that out of the series for the time being.

> I don't particularly like that this register becomes some scrap one that
> threads can use regardless of GCS. Not sure we have a simple solution.
> We could track three states: GCS never enabled, GCS enabled and GCS
> disabled after being enabled. It's probably not worth it.

Yeah, I did give consideration to that but as you say I think it's
probably not worth it - you end up with a state machine which for the
most part only gets two of the states exercised.

> On ptrace() access to the shadow stack, we rely on the barrier in the
> context switch code if stopping a thread. If other threads are running
> on other CPUs, it's racy anyway even for normal accesses, so I don't
> think we need to do anything more for ptrace.

Yes, I don't see any reason to try to do anything special there.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]