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.