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 compat, including 64-bit compat (compat64). This is where the way we convert compat64 syscall arguments to native (PCuABI) 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 arm64/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 compat clone wrapper, but this is rather painful as clone has 4 possible prototypes depending on the architecture. The approach taken here is a middle ground, narrowing down the stack / TLS pointer arguments in the native handler if we got called from compat. This effectively cancels out the automatic creation of capabilities in the compat syscall wrapper, which is not ideal but considered acceptable in this very particular situation.
Fixes: ("clone: Alter clone to accept capabilities") Co-developed-by: Beata Michalska beata.michalska@arm.com Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- kernel/fork.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/kernel/fork.c b/kernel/fork.c index 94777ac4d455..73fe97ad471e 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -2743,14 +2743,19 @@ SYSCALL_DEFINE5(clone, unsigned long, clone_flags, user_uintptr_t, newsp, user_uintptr_t, tls) #endif { + bool compat_mode = in_compat_syscall(); 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, + .stack = (compat_mode ? + (user_uintptr_t)(compat_ulong_t)newsp : + newsp), + .tls = (compat_mode ? + (user_uintptr_t)(compat_ulong_t)tls : + tls), };
return kernel_clone(&args);
LGTM,
Signed-off-by: Beata Michalska beata.michalska@arm.com
--- BR B.
On Tue, Jan 31, 2023 at 10:55:29AM +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 compat, including 64-bit compat (compat64). This is where the way we convert compat64 syscall arguments to native (PCuABI) 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 arm64/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 compat clone wrapper, but this is rather painful as clone has 4 possible prototypes depending on the architecture. The approach taken here is a middle ground, narrowing down the stack / TLS pointer arguments in the native handler if we got called from compat. This effectively cancels out the automatic creation of capabilities in the compat syscall wrapper, which is not ideal but considered acceptable in this very particular situation.
Fixes: ("clone: Alter clone to accept capabilities") Co-developed-by: Beata Michalska beata.michalska@arm.com Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
kernel/fork.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/kernel/fork.c b/kernel/fork.c index 94777ac4d455..73fe97ad471e 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -2743,14 +2743,19 @@ SYSCALL_DEFINE5(clone, unsigned long, clone_flags, user_uintptr_t, newsp, user_uintptr_t, tls) #endif {
- bool compat_mode = in_compat_syscall(); 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,
.stack = (compat_mode ?(user_uintptr_t)(compat_ulong_t)newsp :newsp),.tls = (compat_mode ?(user_uintptr_t)(compat_ulong_t)tls :tls),return kernel_clone(&args); -- 2.38.1
On 31/01/2023 12:31, Beata Michalska wrote:
LGTM,
Signed-off-by: Beata Michalska beata.michalska@arm.com
Thanks! Now applied on next.
Kevin
BR B.
On Tue, Jan 31, 2023 at 10:55:29AM +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 compat, including 64-bit compat (compat64). This is where the way we convert compat64 syscall arguments to native (PCuABI) 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 arm64/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 compat clone wrapper, but this is rather painful as clone has 4 possible prototypes depending on the architecture. The approach taken here is a middle ground, narrowing down the stack / TLS pointer arguments in the native handler if we got called from compat. This effectively cancels out the automatic creation of capabilities in the compat syscall wrapper, which is not ideal but considered acceptable in this very particular situation.
Fixes: ("clone: Alter clone to accept capabilities") Co-developed-by: Beata Michalska beata.michalska@arm.com Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
kernel/fork.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/kernel/fork.c b/kernel/fork.c index 94777ac4d455..73fe97ad471e 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -2743,14 +2743,19 @@ SYSCALL_DEFINE5(clone, unsigned long, clone_flags, user_uintptr_t, newsp, user_uintptr_t, tls) #endif {
- bool compat_mode = in_compat_syscall(); 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,
.stack = (compat_mode ?(user_uintptr_t)(compat_ulong_t)newsp :newsp),.tls = (compat_mode ?(user_uintptr_t)(compat_ulong_t)tls :tls),return kernel_clone(&args); -- 2.38.1
linux-morello@op-lists.linaro.org
-
Beata Michalska -
Kevin Brodsky