Hi Kevin,
On Tue, Dec 20, 2022 at 02:50:52PM +0000, 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.
That's a nice catch -> should this also bear a "Fixes" tag ?
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.
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);
So overall this solution makes sense and it does the right thing, though I am somewhat not entirely convinced this is the way to go about it. It has this nice advantage of fixing up things upfront and propagating the correct values down the stream, but this could be also achieved by adding small tweaks to the original syscall handler although I do appreciate it would be less 'clean' than the suggested solution. Alternatively we could amend copy_thread (which is already compat/morello aware) and handle the difference there ?
--- BR B.
asmlinkage long __arm64_compatentry_sys_ni_syscall(const struct pt_regs *__unused)
2.38.1
linux-morello mailing list -- linux-morello@op-lists.linaro.org To unsubscribe send an email to linux-morello-leave@op-lists.linaro.org