When Morello support is enabled, there is no guarantee that the view of a task's X and C registers (e.g. regs->regs and regs->cregs) is coherent (see also the note on "write coalescing" in Documentation/arm64/morello.rst). X/C merging always happens when returning to userspace, but the value of a task's X registers may be updated without the task being scheduled. This may happen due to ptrace(PTRACE_SETREGSET), but also, crucially, when multiple pending signals are delivered at the same time (typically when the task unblocks them).
All capability registers are read in preserve_morello_context(), in order to store them in the signal frame, and morello_merge_cap_regs() is called beforehand to update the view of capability registers.
However, there was an oversight in one of the initial Morello support commits ("arm64: morello: Context-switch Restricted registers"): regs->csp is also read in signal_sp(), which is called before preserve_morello_context(). signal_sp() may therefore read a stale SP value.
signal_sp() has changed several times since that commit, especially to add PCuABI support. The last commit ("arm64: signal: Fix signal_sp() in compat64") made the issue resurface, as regs->csp is now used in compat64 too.
The error condition is indeed difficult to hit in PCuABI, as it requires using ptrace with a very precise timing. On the other hand, it can be reliably triggered in compat64 by delivering two signals at the same time: regs->sp is set in setup_return() when the first signal frame is set up, leaving regs->csp unchanged, and signal_sp() will then read a stale SP value through regs->csp to set up the second signal frame.
This situation is precisely what the sigpending02 LTP test creates, allowing the bug to be finally caught.
To avoid any further issue, the call to morello_merge_cap_regs() is therefore moved to the very beginning of setup_rt_frame(), before any register is read.
Note that signal_sp() is also called from the rt_sigreturn handler, but this is not a concern as the register view is up-to-date at the point where a syscall handler is called.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- arch/arm64/kernel/signal.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c index 183293d77f80..81130cc42696 100644 --- a/arch/arm64/kernel/signal.c +++ b/arch/arm64/kernel/signal.c @@ -275,14 +275,6 @@ static int preserve_morello_context(struct morello_context __user *ctx, __put_user_error(sizeof(struct morello_context), &ctx->head.size, err); __put_user_error(0, &ctx->__pad, err);
- /* - * current's 64-bit registers may have been modified (e.g. through - * ptrace) since the last time it was scheduled. - * Perform the standard 64-bit / capability register merging, to ensure - * that both views in the signal frame are consistent. - */ - morello_merge_cap_regs(regs); - for (i = 0; i < ARRAY_SIZE(regs->cregs); i++) __morello_put_user_cap_error(regs->cregs[i], &ctx->cregs[i], err); __morello_put_user_cap_error(regs->csp, &ctx->csp, err); @@ -1108,6 +1100,18 @@ static int setup_rt_frame(int usig, struct ksignal *ksig, sigset_t *set, int err = 0;
fpsimd_signal_preserve_current_state(); +#ifdef CONFIG_ARM64_MORELLO + /* + * current's 64-bit registers may have been modified since the last + * time it was scheduled. This may have happened through ptrace, but + * also if another signal frame just got set up without returning to + * userspace. + * Perform the standard 64-bit / capability register merging to ensure + * that both views are consistent, as we will need to read the current + * value of all (C/X) registers. + */ + morello_merge_cap_regs(regs); +#endif
if (get_sigframe(&user, ksig, regs)) return 1;
Hi Kevin,
On 28/03/2023 14:05, Kevin Brodsky wrote:
When Morello support is enabled, there is no guarantee that the view of a task's X and C registers (e.g. regs->regs and regs->cregs) is coherent (see also the note on "write coalescing" in Documentation/arm64/morello.rst). X/C merging always happens when returning to userspace, but the value of a task's X registers may be updated without the task being scheduled. This may happen due to ptrace(PTRACE_SETREGSET), but also, crucially, when multiple pending signals are delivered at the same time (typically when the task unblocks them).
All capability registers are read in preserve_morello_context(), in order to store them in the signal frame, and morello_merge_cap_regs() is called beforehand to update the view of capability registers.
However, there was an oversight in one of the initial Morello support commits ("arm64: morello: Context-switch Restricted registers"): regs->csp is also read in signal_sp(), which is called before preserve_morello_context(). signal_sp() may therefore read a stale SP value.
signal_sp() has changed several times since that commit, especially to add PCuABI support. The last commit ("arm64: signal: Fix signal_sp() in compat64") made the issue resurface, as regs->csp is now used in compat64 too.
The error condition is indeed difficult to hit in PCuABI, as it requires using ptrace with a very precise timing. On the other hand, it can be reliably triggered in compat64 by delivering two signals at the same time: regs->sp is set in setup_return() when the first signal frame is set up, leaving regs->csp unchanged, and signal_sp() will then read a stale SP value through regs->csp to set up the second signal frame.
This situation is precisely what the sigpending02 LTP test creates, allowing the bug to be finally caught.
To avoid any further issue, the call to morello_merge_cap_regs() is therefore moved to the very beginning of setup_rt_frame(), before any register is read.
Note that signal_sp() is also called from the rt_sigreturn handler, but this is not a concern as the register view is up-to-date at the point where a syscall handler is called.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
I feel that the change makes sense. It doesn't appear that merging registers earlier (in do_signal for example) would be very useful as other C registers don't seem to be accessed until where you moved it, only the X counterparts.
I was able to reproduce the issue with the latest kernel and the fix does work well.
Thanks for the patch and all the details ! Best regards, Téo
arch/arm64/kernel/signal.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c index 183293d77f80..81130cc42696 100644 --- a/arch/arm64/kernel/signal.c +++ b/arch/arm64/kernel/signal.c @@ -275,14 +275,6 @@ static int preserve_morello_context(struct morello_context __user *ctx, __put_user_error(sizeof(struct morello_context), &ctx->head.size, err); __put_user_error(0, &ctx->__pad, err);
- /*
* current's 64-bit registers may have been modified (e.g. through* ptrace) since the last time it was scheduled.* Perform the standard 64-bit / capability register merging, to ensure* that both views in the signal frame are consistent.*/- morello_merge_cap_regs(regs);
- for (i = 0; i < ARRAY_SIZE(regs->cregs); i++) __morello_put_user_cap_error(regs->cregs[i], &ctx->cregs[i], err); __morello_put_user_cap_error(regs->csp, &ctx->csp, err);
@@ -1108,6 +1100,18 @@ static int setup_rt_frame(int usig, struct ksignal *ksig, sigset_t *set, int err = 0; fpsimd_signal_preserve_current_state(); +#ifdef CONFIG_ARM64_MORELLO
- /*
* current's 64-bit registers may have been modified since the last* time it was scheduled. This may have happened through ptrace, but* also if another signal frame just got set up without returning to* userspace.* Perform the standard 64-bit / capability register merging to ensure* that both views are consistent, as we will need to read the current* value of all (C/X) registers.*/- morello_merge_cap_regs(regs);
+#endif if (get_sigframe(&user, ksig, regs)) return 1;
Hi,
On 3/28/23 18:35, Kevin Brodsky wrote:
When Morello support is enabled, there is no guarantee that the view of a task's X and C registers (e.g. regs->regs and regs->cregs) is coherent (see also the note on "write coalescing" in Documentation/arm64/morello.rst). X/C merging always happens when returning to userspace, but the value of a task's X registers may be updated without the task being scheduled. This may happen due to ptrace(PTRACE_SETREGSET), but also, crucially, when multiple pending signals are delivered at the same time (typically when the task unblocks them).
All capability registers are read in preserve_morello_context(), in order to store them in the signal frame, and morello_merge_cap_regs() is called beforehand to update the view of capability registers.
However, there was an oversight in one of the initial Morello support commits ("arm64: morello: Context-switch Restricted registers"): regs->csp is also read in signal_sp(), which is called before preserve_morello_context(). signal_sp() may therefore read a stale SP value.
signal_sp() has changed several times since that commit, especially to add PCuABI support. The last commit ("arm64: signal: Fix signal_sp() in compat64") made the issue resurface, as regs->csp is now used in compat64 too.
The error condition is indeed difficult to hit in PCuABI, as it requires using ptrace with a very precise timing. On the other hand, it can be reliably triggered in compat64 by delivering two signals at the same time: regs->sp is set in setup_return() when the first signal frame is set up, leaving regs->csp unchanged, and signal_sp() will then read a stale SP value through regs->csp to set up the second signal frame.
This situation is precisely what the sigpending02 LTP test creates, allowing the bug to be finally caught.
To avoid any further issue, the call to morello_merge_cap_regs() is therefore moved to the very beginning of setup_rt_frame(), before any register is read.
Note that signal_sp() is also called from the rt_sigreturn handler, but this is not a concern as the register view is up-to-date at the point where a syscall handler is called.
I had a look at and this fix looks good to me and clearly fixes the sigpending issue.
//Amit
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
arch/arm64/kernel/signal.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c index 183293d77f80..81130cc42696 100644 --- a/arch/arm64/kernel/signal.c +++ b/arch/arm64/kernel/signal.c @@ -275,14 +275,6 @@ static int preserve_morello_context(struct morello_context __user *ctx, __put_user_error(sizeof(struct morello_context), &ctx->head.size, err); __put_user_error(0, &ctx->__pad, err);
- /*
* current's 64-bit registers may have been modified (e.g. through* ptrace) since the last time it was scheduled.* Perform the standard 64-bit / capability register merging, to ensure* that both views in the signal frame are consistent.*/- morello_merge_cap_regs(regs);
- for (i = 0; i < ARRAY_SIZE(regs->cregs); i++) __morello_put_user_cap_error(regs->cregs[i], &ctx->cregs[i], err); __morello_put_user_cap_error(regs->csp, &ctx->csp, err);
@@ -1108,6 +1100,18 @@ static int setup_rt_frame(int usig, struct ksignal *ksig, sigset_t *set, int err = 0; fpsimd_signal_preserve_current_state(); +#ifdef CONFIG_ARM64_MORELLO
- /*
* current's 64-bit registers may have been modified since the last* time it was scheduled. This may have happened through ptrace, but* also if another signal frame just got set up without returning to* userspace.* Perform the standard 64-bit / capability register merging to ensure* that both views are consistent, as we will need to read the current* value of all (C/X) registers.*/- morello_merge_cap_regs(regs);
+#endif if (get_sigframe(&user, ksig, regs)) return 1;
On 28/03/2023 15:05, Kevin Brodsky wrote:
When Morello support is enabled, there is no guarantee that the view of a task's X and C registers (e.g. regs->regs and regs->cregs) is coherent (see also the note on "write coalescing" in Documentation/arm64/morello.rst). X/C merging always happens when returning to userspace, but the value of a task's X registers may be updated without the task being scheduled. This may happen due to ptrace(PTRACE_SETREGSET), but also, crucially, when multiple pending signals are delivered at the same time (typically when the task unblocks them).
All capability registers are read in preserve_morello_context(), in order to store them in the signal frame, and morello_merge_cap_regs() is called beforehand to update the view of capability registers.
However, there was an oversight in one of the initial Morello support commits ("arm64: morello: Context-switch Restricted registers"): regs->csp is also read in signal_sp(), which is called before preserve_morello_context(). signal_sp() may therefore read a stale SP value.
signal_sp() has changed several times since that commit, especially to add PCuABI support. The last commit ("arm64: signal: Fix signal_sp() in compat64") made the issue resurface, as regs->csp is now used in compat64 too.
The error condition is indeed difficult to hit in PCuABI, as it requires using ptrace with a very precise timing. On the other hand, it can be reliably triggered in compat64 by delivering two signals at the same time: regs->sp is set in setup_return() when the first signal frame is set up, leaving regs->csp unchanged, and signal_sp() will then read a stale SP value through regs->csp to set up the second signal frame.
This situation is precisely what the sigpending02 LTP test creates, allowing the bug to be finally caught.
To avoid any further issue, the call to morello_merge_cap_regs() is therefore moved to the very beginning of setup_rt_frame(), before any register is read.
Note that signal_sp() is also called from the rt_sigreturn handler, but this is not a concern as the register view is up-to-date at the point where a syscall handler is called.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
arch/arm64/kernel/signal.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)
Applied on next, thanks Téo and Amit for the reviews!
Kevin
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c index 183293d77f80..81130cc42696 100644 --- a/arch/arm64/kernel/signal.c +++ b/arch/arm64/kernel/signal.c @@ -275,14 +275,6 @@ static int preserve_morello_context(struct morello_context __user *ctx, __put_user_error(sizeof(struct morello_context), &ctx->head.size, err); __put_user_error(0, &ctx->__pad, err);
- /*
* current's 64-bit registers may have been modified (e.g. through* ptrace) since the last time it was scheduled.* Perform the standard 64-bit / capability register merging, to ensure* that both views in the signal frame are consistent.*/- morello_merge_cap_regs(regs);
- for (i = 0; i < ARRAY_SIZE(regs->cregs); i++) __morello_put_user_cap_error(regs->cregs[i], &ctx->cregs[i], err); __morello_put_user_cap_error(regs->csp, &ctx->csp, err);
@@ -1108,6 +1100,18 @@ static int setup_rt_frame(int usig, struct ksignal *ksig, sigset_t *set, int err = 0; fpsimd_signal_preserve_current_state(); +#ifdef CONFIG_ARM64_MORELLO
- /*
* current's 64-bit registers may have been modified since the last* time it was scheduled. This may have happened through ptrace, but* also if another signal frame just got set up without returning to* userspace.* Perform the standard 64-bit / capability register merging to ensure* that both views are consistent, as we will need to read the current* value of all (C/X) registers.*/- morello_merge_cap_regs(regs);
+#endif if (get_sigframe(&user, ksig, regs)) return 1;
linux-morello@op-lists.linaro.org
-
Amit Daniel Kachhap -
Kevin Brodsky -
Teo Couprie Diaz