| Message ID | 20250318204841.373116-4-jeremy.linton@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: Enable UPROBES with GCS | expand |
On Tue, Mar 18, 2025 at 03:48:37PM -0500, Jeremy Linton wrote: > +static inline u64 load_user_gcs(unsigned long __user *addr, int *err) > +{ > + unsigned long ret; > + u64 load; > + > + if (!access_ok((char __user *)addr, sizeof(load))) { > + *err = -EFAULT; > + return 0; > + } > + > + gcsb_dsync(); > + ret = copy_from_user(&load, addr, sizeof(load)); > + if (ret != 0) > + *err = ret; > + return load; > +} A GCS load done by the hardware will verify that we are loading from GCS memory (the accesses are marked as AccessType_GCS in the pseudocode which is then validated in for example S1CheckPermissions()). Sadly there's no equivalent of GCSSTR so we'd need to do the permission check ourselves to match this behaviour.
Hi, On 3/19/25 8:24 AM, Mark Brown wrote: > On Tue, Mar 18, 2025 at 03:48:37PM -0500, Jeremy Linton wrote: > >> +static inline u64 load_user_gcs(unsigned long __user *addr, int *err) >> +{ >> + unsigned long ret; >> + u64 load; >> + >> + if (!access_ok((char __user *)addr, sizeof(load))) { >> + *err = -EFAULT; >> + return 0; >> + } >> + >> + gcsb_dsync(); >> + ret = copy_from_user(&load, addr, sizeof(load)); >> + if (ret != 0) >> + *err = ret; >> + return load; >> +} > > A GCS load done by the hardware will verify that we are loading from GCS > memory (the accesses are marked as AccessType_GCS in the pseudocode > which is then validated in for example S1CheckPermissions()). Sadly > there's no equivalent of GCSSTR so we'd need to do the permission check > ourselves to match this behaviour. Right, except that if I grab the VMA as a placeholder for the page, check to see if its a VM_SHADOW_STACK under any of map_read_lock()/lock_vma_under_rcu()/etc and then perform the access, the resulting possible fault will have problems with vma locking. Otherwise there ends up being a few different races that at the moment I've not yet figured out how to fix without making a big mess. For example, we can reduce that possible window, by reading the value/locking and checking shadow stack state/dropping the lock/rereading the value, or some other construct but it seems pointless because the suggested problem is that we might be creating a way to bypass some of the shadow stack security. In which case, leaving a little race is likely the same as leaving it wide open. Otherwise, maybe we can ignore the problem, or just refuse to allow probes on 'RET' instructions which seems to be the main problematic case. Although, given we don't really know if GCS is enabled until the probe is hit, SIGSEG'ing the target process is a big hammer. Ignoring it might be a valid option. I guess it could to be one of those "if the user puts a uprobe on a RET some of the shadow stack security is reduced" footguns. If an attacker can also manipulate the address space in a way to exploit it then its probably game over anyway. Ideally, the kernel would warn on this, but per the conversation around patch 6/7 that seems to be off the table.
On Fri, Mar 21, 2025 at 06:43:23PM -0500, Jeremy Linton wrote: > On 3/19/25 8:24 AM, Mark Brown wrote: > > A GCS load done by the hardware will verify that we are loading from GCS > > memory (the accesses are marked as AccessType_GCS in the pseudocode > > which is then validated in for example S1CheckPermissions()). Sadly > > there's no equivalent of GCSSTR so we'd need to do the permission check > > ourselves to match this behaviour. > Right, except that if I grab the VMA as a placeholder for the page, check to > see if its a VM_SHADOW_STACK under any of > map_read_lock()/lock_vma_under_rcu()/etc and then perform the access, the > resulting possible fault will have problems with vma locking. Otherwise > there ends up being a few different races that at the moment I've not yet > figured out how to fix without making a big mess. For example, we can reduce > that possible window, by reading the value/locking and checking shadow stack > state/dropping the lock/rereading the value, or some other construct but it > seems pointless because the suggested problem is that we might be creating a > way to bypass some of the shadow stack security. In which case, leaving a > little race is likely the same as leaving it wide open. Yeah, it's messy. The "nicest" thing I could think of was doing a GCS store of the value we just read to validate the GCS permission but that has very obvious ick and is in it's own way incorrect. Since the GCS permission is always read/write I'm not sure what would notice without an incredibly dodgy race but it's wrong. > Otherwise, maybe we can ignore the problem, or just refuse to allow probes > on 'RET' instructions which seems to be the main problematic case. Although, > given we don't really know if GCS is enabled until the probe is hit, > SIGSEG'ing the target process is a big hammer. Yeah, that doesn't feel like the solution. > Ignoring it might be a valid option. I guess it could to be one of those "if > the user puts a uprobe on a RET some of the shadow stack security is > reduced" footguns. If an attacker can also manipulate the address space in a > way to exploit it then its probably game over anyway. Ideally, the kernel > would warn on this, but per the conversation around patch 6/7 that seems to > be off the table. I'm not completely opposed to just not doing the validation given the pain with implementing it, it's hard to be enthusiastic about any of the options really. If we are going to do something other than fully and accurately emulate then we should acknowledge what's missing and why, at least in the changelog and probably also in the code.
diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h index 5b91803201ef..c77ab09a01c2 100644 --- a/arch/arm64/include/asm/uaccess.h +++ b/arch/arm64/include/asm/uaccess.h @@ -20,6 +20,7 @@ #include <asm/asm-extable.h> #include <asm/cpufeature.h> +#include <asm/gcs.h> #include <asm/mmu.h> #include <asm/mte.h> #include <asm/ptrace.h> @@ -539,6 +540,47 @@ static inline void put_user_gcs(unsigned long val, unsigned long __user *addr, uaccess_ttbr0_disable(); } +static __always_inline unsigned long __must_check +copy_from_user(void *to, const void __user *from, unsigned long n); + +static inline u64 load_user_gcs(unsigned long __user *addr, int *err) +{ + unsigned long ret; + u64 load; + + if (!access_ok((char __user *)addr, sizeof(load))) { + *err = -EFAULT; + return 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; +} #endif /* CONFIG_ARM64_GCS */
Uprobes need more advanced read, push, and pop userspace GCS functionality. Implement those features using the existing gcsstr() and copy_from_user(). Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> --- arch/arm64/include/asm/uaccess.h | 42 ++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+)