On Fri, Jun 27, 2025 at 06:19:33PM +0100, Catalin Marinas wrote:
On Mon, Jun 09, 2025 at 01:54:05PM +0100, Mark Brown wrote:
- /* Ensure that a token written as a result of a pivot is visible */
- gcsb_dsync();
- gcspr_el0 = args->shadow_stack_token;
- if (!gcs_consume_token(vma, page, gcspr_el0))
return -EINVAL;- tsk->thread.gcspr_el0 = gcspr_el0 + sizeof(u64);
- /* Ensure that our token consumption visible */
- gcsb_dsync();
What are the scenarios where we need the barriers? We have one via map_shadow_stack() that would cover the first one. IIUC, GCSSS2 also generates a GCSB event (or maybe I got it all wrong; I need to read the spec).
I think now that gcs_consume_token() does a cmpxchg they're redundant, your analysis covers the first one (anything that puts a valid token in memory should have a barrier) and now gcs_consume_token() does a cmpxchg the second one should also be redundant thanks to R_FZRGP. It would be good if someone double checked though.
Originally gcs_consume_token() was using regular accesses as for the example in DDI0487 L.a K3.3 and was tried on two addresses, I missed dropping the barriers when changing to a cmpxchg.
+static int shstk_validate_clone(struct task_struct *p,
struct kernel_clone_args *args)+{
- mmap_read_lock(mm);
- addr = untagged_addr_remote(mm, args->shadow_stack_token);
I think down the line, get_user_page_vma_remote() already does an untagged_addr_remote(). But it does it after the vma look-up, so we still need the untagging early.
That said, would we ever allowed a tagged pointer for the shadow stack?
For arm64 you can architecturally use tags as per G_HMJHM. I_WBHHX says that GCS accesses are tag unchecked, but tags are used on GCSSS1 as per I_MGLTC and I_MBHFS. We'll need new ABI to allow userspace to get a PROT_MTE GCS though, I'd planned on extending map_shadow_stack() for that, and adding handling in the token validation here.
There's also the fact that the untagging should be very cheap in the context of what we're doing so it seems sensible to just have it, especially generic code which applies to all arches.
+static inline bool clone3_shadow_stack_valid(struct kernel_clone_args *kargs) +{
- if (!kargs->shadow_stack_token)
return true;- if (!IS_ALIGNED(kargs->shadow_stack_token, sizeof(void *)))
return false;- /*
* The architecture must check support on the specific* machine.*/- return IS_ENABLED(CONFIG_ARCH_HAS_USER_SHADOW_STACK);
I don't understand the comment here. It implies some kind of fallback for further arch checks but it's just a return.
Here we're just doing initial triage that a shadow stack could possibly be valid, the check here is there to fail if there's one specified but there is no support in the kernel (eg, for architectures that don't have the feature at all like arm32). The comment is trying to say that we're not attempting to validate that we can actually use shadow stacks on the current system, just that the support exists in the kernel. I'll reword the comment, it's not clear.
BTW, clone3_stack_valid() has an access_ok() check as well. Shall we add it here? That's where the size would have come in handy but IIUC the decision was to drop it (fine by me, just validate that the token is accessible).
AIUI the main reason for doing that for the normal stack is to report an error before we actually start the thread and have it fault trying to access an invalid stack since we don't otherwise look at the memory, like you say with shadow stacks we'll consume the token before we start the new thread so we get the equivalent error reporting as part of that. I don't think the extra check would buy us much.