Mark Brown <broo...@kernel.org> writes:
> When a new thread is created by a thread with GCS enabled the GCS needs
> to be specified along with the regular stack. clone3() has been
> extended to support this case, allowing userspace to explicitly specify
> the size and location of the GCS. The specified GCS must have a valid
> GCS token at the top of the stack, as though userspace were pivoting to
> the new GCS. This will be consumed on use. At present we do not
> atomically consume the token, this will be addressed in a future
> revision.
>
> Unfortunately plain clone() is not extensible and existing clone3()
> users will not specify a stack so all existing code would be broken if
> we mandated specifying the stack explicitly. For compatibility with
> these cases and also x86 (which did not initially implement clone3()
> support for shadow stacks) if no GCS is specified we will allocate one
> thread so when a thread is created which has GCS enabled allocate one
~~~~~~
This "thread" seems extraneous in the sentence. Remove it?
> for it. We follow the extensively discussed x86 implementation and
> allocate min(RLIMIT_STACK, 4G). Since the GCS only stores the call
Isn't it min(RLIMIT_STACK/2, 2G), as seen in gcs_size()? If true, this
size should also be fixed in Documentation/arch/arm64/gcs.rst.
> stack and not any variables this should be more than sufficient for most
> applications.
>
> GCSs allocated via this mechanism then it will be freed when the thread
> exits, those explicitly configured by the user will not.
I'm not sure I parsed this sentence correctly. Is it missing an "If" at
the beginning?
> +unsigned long gcs_alloc_thread_stack(struct task_struct *tsk,
> + const struct kernel_clone_args *args)
> +{
> + unsigned long addr, size, gcspr_el0;
> +
> + /* If the user specified a GCS use it. */
> + if (args->shadow_stack_size) {
> + if (!system_supports_gcs())
> + return (unsigned long)ERR_PTR(-EINVAL);
> +
> + addr = args->shadow_stack;
> + size = args->shadow_stack_size;
> +
> + /*
> + * There should be a token, there might be an end of
> + * stack marker.
> + */
> + gcspr_el0 = addr + size - (2 * sizeof(u64));
> + if (!gcs_consume_token(tsk, gcspr_el0)) {
Should this code validate the end of stack marker? Or doesn't it matter
whether the marker is correct or not?
> + gcspr_el0 += sizeof(u64);
> + if (!gcs_consume_token(tsk, gcspr_el0))
> + return (unsigned long)ERR_PTR(-EINVAL);
> + }
> +
> + /* Userspace is responsible for unmapping */
> + tsk->thread.gcspr_el0 = gcspr_el0 + sizeof(u64);
> + } else {
--
Thiago
