On 20-12-2022 14:50, Kevin Brodsky wrote:
A subtle ABI change was introduced in compat64 by "clone: Alter clone to accept capabilities". Indeed, since there is no compat handler for clone, the native one is also used for compat64. This is where the way we convert compat64 syscall arguments to native is showing its limits: if a syscall wrapper expects a capability-sized argument, compat_ptr() is used to convert the user-provided 64-bit value to a capability.
In general, this is what we want, and in fact it is the case for the parent_tidptr and child_tidptr arguments of clone, which are ordinary pointers to user data. However, the newsp and tls arguments are special: they specify the value to set registers to. We should not alter these values in any way: in PCuABI, they are capabilities and we set CSP/CTPIDR accordingly, but in hybrid, they are still 64-bit values and we should only set the lower 64 bits of CSP/CTPIDR. This is not the case in compat64 as compat_ptr() is called to turn these 64-bit values into capabilities.
Hi Kevin,
The approach looks good, but I have only one question. Where specifically newsp and tls arguments would become valid capabilities from compat_ptr?
As far as I understand it, compat_ptr is used only when "clone_args_get_user_ptr" is used, but newsp and tls are obtained using "clone_args_get", so the wrapper would never convert it to a capability. Unless I am missing something...
Thanks, Tudor
The most correct solution would be to introduce a generic compat clone wrapper, but this is rather painful as clone has 4 possible prototypes depending on the architecture. Given that the issue is completely specific to the hybrid ABI, overriding the compat64 syscall wrapper in sys_compat64.c feels like a reasonable compromise.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
I've realised this inconsistency while thinking about the initialisation of capability registers ("New CHERI API and separation root capabilities" series), as well as reviewing the clone3 series. We are already doing the right thing for clone3, time to align clone.
arch/arm64/kernel/sys_compat64.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)
diff --git a/arch/arm64/kernel/sys_compat64.c b/arch/arm64/kernel/sys_compat64.c index 819b895ec21d..b3c4cf9f3af8 100644 --- a/arch/arm64/kernel/sys_compat64.c +++ b/arch/arm64/kernel/sys_compat64.c @@ -9,6 +9,7 @@ #include <linux/compat.h> #include <linux/compiler.h> +#include <linux/sched/task.h> #include <linux/syscalls.h> #include <asm/syscall.h> @@ -83,6 +84,33 @@ #define __arm64_compatentry_compat_sys_setitimer __arm64_compatentry_sys_setitimer #define __arm64_compatentry_compat_sys_getrusage __arm64_compatentry_sys_getrusage +/*
- This is exactly the same definition as the native clone, except that newsp
- and tls are defined as unsigned long, not user_uintptr_t. When the native ABI
- is PCuABI, this prevents capabilities from being implicitly created for the
- stack/TLS in compat64 by the syscall wrapper. This ensures alignment with the
- hybrid ABI (i.e. CSP/CTPIDR are set to the 64-bit values passed to clone()).
- */
+COMPAT_SYSCALL_DEFINE5(arm64_clone, unsigned long, clone_flags, unsigned long, newsp,
int __user *, parent_tidptr,unsigned long, tls,int __user *, child_tidptr)+{
- struct kernel_clone_args args = {
.flags = (lower_32_bits(clone_flags) & ~CSIGNAL),.pidfd = parent_tidptr,.child_tid = child_tidptr,.parent_tid = parent_tidptr,.exit_signal = (lower_32_bits(clone_flags) & CSIGNAL),.stack = newsp,.tls = tls,- };
- return kernel_clone(&args);
+}
+#define __arm64_compatentry_sys_clone __arm64_compatentry_compat_sys_arm64_clone
- asmlinkage long sys_ni_syscall(void);
asmlinkage long __arm64_compatentry_sys_ni_syscall(const struct pt_regs *__unused)