There are two issues with signal_sp() in compat64: 1. It uses regs->sp, which is not always the Executive SP. 2. It uses uaddr_to_user_ptr_safe(), which shouldn't be used to create user pointers in compat.
The comment in signal_sp() also became meaningless, as it should refer to regs->sp and not regs->csp.
Fix both issuses and expand the comment, as the situation is clearly complicated enough to warrant more explanations.
Fixes: ("arm64: signal: Add support for 64-bit compat signal handling") Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- arch/arm64/kernel/signal.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c index 80289e90fc66..183293d77f80 100644 --- a/arch/arm64/kernel/signal.c +++ b/arch/arm64/kernel/signal.c @@ -184,15 +184,25 @@ static user_uintptr_t signal_sp(struct pt_regs *regs) { #ifdef CONFIG_ARM64_MORELLO /* - * If the interrupted context was in Restricted, regs->csp is actually - * RCSP_EL0, which is usually what we want but not here, because signal - * handlers are always executed in Executive and therefore on the - * Executive stack. Read the actual (Executive) CSP_EL0 instead. + * Callers of this function want a valid user pointer to the stack. + * There are a number of situations to consider: + * - In PCuABI, CSP is expected to be a valid capability to access the + * stack, so we just return that. + * - In compat64, userspace uses only SP; we create a valid user pointer + * from SP using compat_ptr(). + * - In the standard ABI (PCuABI not selected), user pointers are still + * 64-bit integers so we just need to return SP. + * + * Signal handlers are always run in Executive. We therefore need to + * return a pointer to the Executive stack. As a result we cannot use + * regs->sp, as it is actually RSP_EL0 if the interrupted context was in + * Restricted. For this reason we use regs->csp even when we just want + * the (64-bit) SP. */ #ifndef SIGNAL_COMPAT64 return (user_uintptr_t)regs->csp; #else /* SIGNAL_COMPAT64 */ - return (user_uintptr_t)uaddr_to_user_ptr_safe(regs->sp); + return (user_uintptr_t)compat_ptr((ptraddr_t)regs->csp); #endif /* SIGNAL_COMPAT64 */ #else /* !CONFIG_ARM64_MORELLO */ return regs->sp;
Hi,
On 3/16/23 13:34, Kevin Brodsky wrote:
There are two issues with signal_sp() in compat64:
- It uses regs->sp, which is not always the Executive SP.
- It uses uaddr_to_user_ptr_safe(), which shouldn't be used to create user pointers in compat.
The comment in signal_sp() also became meaningless, as it should refer to regs->sp and not regs->csp.
Fix both issuses and expand the comment, as the situation is clearly complicated enough to warrant more explanations.
+1 from my side. Currently the kernel selftests are limited in the area of restricted/executive switches. It would be good to have more of them.
//Amit
Fixes: ("arm64: signal: Add support for 64-bit compat signal handling") Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
arch/arm64/kernel/signal.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c index 80289e90fc66..183293d77f80 100644 --- a/arch/arm64/kernel/signal.c +++ b/arch/arm64/kernel/signal.c @@ -184,15 +184,25 @@ static user_uintptr_t signal_sp(struct pt_regs *regs) { #ifdef CONFIG_ARM64_MORELLO /*
* If the interrupted context was in Restricted, regs->csp is actually* RCSP_EL0, which is usually what we want but not here, because signal* handlers are always executed in Executive and therefore on the* Executive stack. Read the actual (Executive) CSP_EL0 instead.
* Callers of this function want a valid user pointer to the stack.* There are a number of situations to consider:* - In PCuABI, CSP is expected to be a valid capability to access the* stack, so we just return that.* - In compat64, userspace uses only SP; we create a valid user pointer* from SP using compat_ptr().* - In the standard ABI (PCuABI not selected), user pointers are still* 64-bit integers so we just need to return SP.** Signal handlers are always run in Executive. We therefore need to* return a pointer to the Executive stack. As a result we cannot use* regs->sp, as it is actually RSP_EL0 if the interrupted context was in* Restricted. For this reason we use regs->csp even when we just want* the (64-bit) SP.
- return (user_uintptr_t)uaddr_to_user_ptr_safe(regs->sp);
- return (user_uintptr_t)compat_ptr((ptraddr_t)regs->csp); #endif /* SIGNAL_COMPAT64 */ #else /* !CONFIG_ARM64_MORELLO */ return regs->sp;
On 23/03/2023 10:37, Amit Daniel Kachhap wrote:
Hi,
On 3/16/23 13:34, Kevin Brodsky wrote:
There are two issues with signal_sp() in compat64:
- It uses regs->sp, which is not always the Executive SP.
- It uses uaddr_to_user_ptr_safe(), which shouldn't be used to
create user pointers in compat.
The comment in signal_sp() also became meaningless, as it should refer to regs->sp and not regs->csp.
Fix both issuses and expand the comment, as the situation is clearly complicated enough to warrant more explanations.
+1 from my side. Currently the kernel selftests are limited in the area of restricted/executive switches. It would be good to have more of them.
//Amit
Thanks for the review! Agreed this area needs more testing, for now though we're focusing on purecap testing and we don't really have the infrastructure for hybrid Morello kselftests. I think that's not too bad as I doubt anyone is running truly hybrid code with the PCuABI kernel at the moment, but for sure we should improve the testing in that area eventually.
Now applied that patch on next with a small typo fixed (s/issuses/issues/).
Kevin
Fixes: ("arm64: signal: Add support for 64-bit compat signal handling") Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
arch/arm64/kernel/signal.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c index 80289e90fc66..183293d77f80 100644 --- a/arch/arm64/kernel/signal.c +++ b/arch/arm64/kernel/signal.c @@ -184,15 +184,25 @@ static user_uintptr_t signal_sp(struct pt_regs *regs) { #ifdef CONFIG_ARM64_MORELLO /* - * If the interrupted context was in Restricted, regs->csp is actually - * RCSP_EL0, which is usually what we want but not here, because signal - * handlers are always executed in Executive and therefore on the - * Executive stack. Read the actual (Executive) CSP_EL0 instead. + * Callers of this function want a valid user pointer to the stack. + * There are a number of situations to consider: + * - In PCuABI, CSP is expected to be a valid capability to access the + * stack, so we just return that. + * - In compat64, userspace uses only SP; we create a valid user pointer + * from SP using compat_ptr(). + * - In the standard ABI (PCuABI not selected), user pointers are still + * 64-bit integers so we just need to return SP. + * + * Signal handlers are always run in Executive. We therefore need to + * return a pointer to the Executive stack. As a result we cannot use + * regs->sp, as it is actually RSP_EL0 if the interrupted context was in + * Restricted. For this reason we use regs->csp even when we just want + * the (64-bit) SP. */ #ifndef SIGNAL_COMPAT64 return (user_uintptr_t)regs->csp; #else /* SIGNAL_COMPAT64 */ - return (user_uintptr_t)uaddr_to_user_ptr_safe(regs->sp); + return (user_uintptr_t)compat_ptr((ptraddr_t)regs->csp); #endif /* SIGNAL_COMPAT64 */ #else /* !CONFIG_ARM64_MORELLO */ return regs->sp;
linux-morello@op-lists.linaro.org
-
Amit Daniel Kachhap -
Kevin Brodsky