Uprobes need more advanced read, push, and pop userspace GCS
functionality. Implement those features using the existing gcsstr()
and copy_from_user().

Its important to note that GCS pages can be read by normal
instructions, but the hardware validates that pages used by GCS
specific operations, have a GCS privilege set. We aren't validating this
in load_user_gcs because it requires stabilizing the VMA over the read
which may fault.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 arch/arm64/include/asm/gcs.h | 52 ++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/arch/arm64/include/asm/gcs.h b/arch/arm64/include/asm/gcs.h
index e3b360c9dba4..f2fc8173fee3 100644
--- a/arch/arm64/include/asm/gcs.h
+++ b/arch/arm64/include/asm/gcs.h
@@ -116,6 +116,45 @@ static inline void put_user_gcs(unsigned long val, unsigned long __user *addr,
 	uaccess_ttbr0_disable();
 }
 
+/*
+ * Unlike put_user_gcs() above, the use of copy_from_user() may provide
+ * an opening for non GCS pages to be used to source data. Therefore this
+ * should only be used in contexts where that is acceptable.
+ */
+static inline u64 load_user_gcs(unsigned long __user *addr, int *err)
+{
+	unsigned long ret;
+	u64 load = 0;
+
+	gcsb_dsync();
+	ret = copy_from_user(&load, addr, sizeof(load));
+	if (ret != 0)
+		*err = ret;
+	return load;
+}
+
+static inline void push_user_gcs(unsigned long val, int *err)
+{
+	u64 gcspr = read_sysreg_s(SYS_GCSPR_EL0);
+
+	gcspr -= sizeof(u64);
+	put_user_gcs(val, (unsigned long __user *)gcspr, err);
+	if (!*err)
+		write_sysreg_s(gcspr, SYS_GCSPR_EL0);
+}
+
+static inline u64 pop_user_gcs(int *err)
+{
+	u64 gcspr = read_sysreg_s(SYS_GCSPR_EL0);
+	u64 read_val;
+
+	read_val = load_user_gcs((unsigned long __user *)gcspr, err);
+	if (!*err)
+		write_sysreg_s(gcspr + sizeof(u64), SYS_GCSPR_EL0);
+
+	return read_val;
+}
+
 #else
 
 static inline bool task_gcs_el0_enabled(struct task_struct *task)
@@ -126,6 +165,10 @@ static inline bool task_gcs_el0_enabled(struct task_struct *task)
 static inline void gcs_set_el0_mode(struct task_struct *task) { }
 static inline void gcs_free(struct task_struct *task) { }
 static inline void gcs_preserve_current_state(void) { }
+static inline void put_user_gcs(unsigned long val, unsigned long __user *addr,
+				int *err) { }
+static inline void push_user_gcs(unsigned long val, int *err) { }
+
 static inline unsigned long gcs_alloc_thread_stack(struct task_struct *tsk,
 						   const struct kernel_clone_args *args)
 {
@@ -136,6 +179,15 @@ static inline int gcs_check_locked(struct task_struct *task,
 {
 	return 0;
 }
+static inline u64 load_user_gcs(unsigned long __user *addr, int *err)
+{
+	*err = -EFAULT;
+	return 0;
+}
+static inline u64 pop_user_gcs(int *err)
+{
+	return 0;
+}
 
 #endif
 
-- 
2.50.1

On Fri, Jul 18, 2025 at 11:37:36PM -0500, Jeremy Linton wrote:
> +/*
> + * Unlike put_user_gcs() above, the use of copy_from_user() may provide
> + * an opening for non GCS pages to be used to source data. Therefore this
> + * should only be used in contexts where that is acceptable.
> + */

Even in user space, the GCS pages can be read with normal loads, so
already usable as a data source if one wants to (not that it's of much
use). So not sure the comment here is needed.

> +static inline u64 load_user_gcs(unsigned long __user *addr, int *err)

Nitpick: name this get_user_gcs() for symmetry with put_user_gcs().

> +{
> +	unsigned long ret;
> +	u64 load = 0;
> +
> +	gcsb_dsync();

Might be worth a comment here, see the one in gcs_restore_signal().

> +	ret = copy_from_user(&load, addr, sizeof(load));
> +	if (ret != 0)
> +		*err = ret;
> +	return load;
> +}

Otherwise the patch looks fine:

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Hi,


Thanks for looking at this.

On 7/23/25 4:50 AM, Catalin Marinas wrote:
> On Fri, Jul 18, 2025 at 11:37:36PM -0500, Jeremy Linton wrote:
>> +/*
>> + * Unlike put_user_gcs() above, the use of copy_from_user() may provide
>> + * an opening for non GCS pages to be used to source data. Therefore this
>> + * should only be used in contexts where that is acceptable.
>> + */
> 
> Even in user space, the GCS pages can be read with normal loads, so
> already usable as a data source if one wants to (not that it's of much
> use). So not sure the comment here is needed.

Right, but userspace isn't using it in a privileged context to emulate 
operations that have a permission check performed as part of the read 
when performed by the HW.

This comment was added in V2 following a number of conversations about 
whether this was an actual risk or something that is only a problem if a 
long set of pre-conditions hold true. Conditions which can be summarized 
as "it is too late anyway".

Hence the comment to remind people that this routine isn't assuring the 
page is correctly marked.

I will reword it a bit if that is ok.


> 
>> +static inline u64 load_user_gcs(unsigned long __user *addr, int *err)
> 
> Nitpick: name this get_user_gcs() for symmetry with put_user_gcs().
> 
>> +{
>> +	unsigned long ret;
>> +	u64 load = 0;
>> +
>> +	gcsb_dsync();
> 
> Might be worth a comment here, see the one in gcs_restore_signal().

Sure,

> 
>> +	ret = copy_from_user(&load, addr, sizeof(load));
>> +	if (ret != 0)
>> +		*err = ret;
>> +	return load;
>> +}
> 
> Otherwise the patch looks fine:
> 
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

On Wed, Jul 23, 2025 at 12:14:17PM -0500, Jeremy Linton wrote:
> On 7/23/25 4:50 AM, Catalin Marinas wrote:
> > On Fri, Jul 18, 2025 at 11:37:36PM -0500, Jeremy Linton wrote:
> > > +/*
> > > + * Unlike put_user_gcs() above, the use of copy_from_user() may provide
> > > + * an opening for non GCS pages to be used to source data. Therefore this
> > > + * should only be used in contexts where that is acceptable.
> > > + */
> > 
> > Even in user space, the GCS pages can be read with normal loads, so
> > already usable as a data source if one wants to (not that it's of much
> > use). So not sure the comment here is needed.
> 
> Right, but userspace isn't using it in a privileged context to emulate
> operations that have a permission check performed as part of the read when
> performed by the HW.
> 
> This comment was added in V2 following a number of conversations about
> whether this was an actual risk or something that is only a problem if a
> long set of pre-conditions hold true. Conditions which can be summarized as
> "it is too late anyway".
> 
> Hence the comment to remind people that this routine isn't assuring the page
> is correctly marked.

I think the comment on the load function doesn't make much difference
since LDR is permitted on an GCS page anyway. It's the pop function that
we actually emulate without proper GCS instructions that's more
problematic and won't be checked against actual GCS permissions.

> I will reword it a bit if that is ok.

Yes, please do.

Thanks.

-- 
Catalin

On Thu, Jul 24, 2025 at 06:14:42AM +0100, Catalin Marinas wrote:
> On Wed, Jul 23, 2025 at 12:14:17PM -0500, Jeremy Linton wrote:

> > Hence the comment to remind people that this routine isn't assuring the page
> > is correctly marked.

> I think the comment on the load function doesn't make much difference
> since LDR is permitted on an GCS page anyway. It's the pop function that
> we actually emulate without proper GCS instructions that's more
> problematic and won't be checked against actual GCS permissions.

This is used in the emulation of RET where it results in a similar lack
of a permission check - that does:

+       if (task_gcs_el0_enabled(current)) {
+               gcspr = read_sysreg_s(SYS_GCSPR_EL0);
+               gcs_ret_vaddr = load_user_gcs((unsigned long __user *)gcspr, &err);

When implemented by the hardware we would generate a fault if the
address we're loading from is not in a page with GCS permissions.  The
issue isn't that userspace wouldn't be permitted to read the value, the
issue is that we are not checking that the value is being read from a
page with GCS permissions.

On Wed, Jul 23, 2025 at 10:50:33AM +0100, Catalin Marinas wrote:
> On Fri, Jul 18, 2025 at 11:37:36PM -0500, Jeremy Linton wrote:
> > +/*
> > + * Unlike put_user_gcs() above, the use of copy_from_user() may provide
> > + * an opening for non GCS pages to be used to source data. Therefore this
> > + * should only be used in contexts where that is acceptable.
> > + */

> Even in user space, the GCS pages can be read with normal loads, so
> already usable as a data source if one wants to (not that it's of much
> use). So not sure the comment here is needed.

The comment should probably be clarified to mention that the specific
issue is the lack of an operation that does a load with GCS permission
check to match what we have for stores, it'd be a bit of a landmine to
have the operation available without anything that flags that it didn't
validate the permissions (even assuming people read the comments in the
header...).