2022-03-06 09:37:38

by Max Filippov

Subject: [PATCH 0/2] xtensa: fix pt_regs to cover user exception frame

Hello,

this series updates xtensa struct pt_regs so that it covers the whole
user exception frame fixing kernel build issue visible in the current
linux-next.

Max Filippov (2):
xtensa: rename PT_SIZE to PT_KERNEL_SIZE
xtensa: use XCHAL_NUM_AREGS as pt_regs::areg size

arch/xtensa/include/asm/ptrace.h | 7 +++----
arch/xtensa/kernel/asm-offsets.c | 2 +-
arch/xtensa/kernel/entry.S | 14 +++++++-------
arch/xtensa/kernel/process.c | 10 ----------
arch/xtensa/kernel/vectors.S | 4 ++--
5 files changed, 13 insertions(+), 24 deletions(-)

--
2.30.2

2022-03-06 13:30:30

by Max Filippov

Subject: [PATCH 2/2] xtensa: use XCHAL_NUM_AREGS as pt_regs::areg size

struct pt_regs is used to access both kernel and user exception frames.
User exception frames may contain up to XCHAL_NUM_AREG registers that
task creation and signal delivery code may access, but pt_regs::areg
array has only 16 entries that cover only the kernel exception frame.
This results in the following build error:

arch/xtensa/kernel/process.c: In function 'copy_thread':
arch/xtensa/kernel/process.c:262:52: error: array subscript 53 is above
array bounds of 'long unsigned int[16]' [-Werror=array-bounds]
262 | put_user(regs->areg[caller_ars+1],

Change struct pt_regs::areg size to XCHAL_NUM_AREGS so that it covers
the whole user exception frame. Adjust task_pt_regs and drop additional
register copying code from copy_thread now that the whole user exception
stack frame is copied.

Reported-by: Kees Cook <[email protected]>
Signed-off-by: Max Filippov <[email protected]>
---
arch/xtensa/include/asm/ptrace.h | 7 +++----
arch/xtensa/kernel/process.c | 10 ----------
2 files changed, 3 insertions(+), 14 deletions(-)

diff --git a/arch/xtensa/include/asm/ptrace.h b/arch/xtensa/include/asm/ptrace.h
index b109416dc07e..308f209a4740 100644
--- a/arch/xtensa/include/asm/ptrace.h
+++ b/arch/xtensa/include/asm/ptrace.h
@@ -44,6 +44,7 @@
#ifndef __ASSEMBLY__

#include <asm/coprocessor.h>
+#include <asm/core.h>

/*
* This struct defines the way the registers are stored on the
@@ -77,14 +78,12 @@ struct pt_regs {
/* current register frame.
* Note: The ESF for kernel exceptions ends after 16 registers!
*/
- unsigned long areg[16];
+ unsigned long areg[XCHAL_NUM_AREGS];
};

-#include <asm/core.h>
-
# define arch_has_single_step() (1)
# define task_pt_regs(tsk) ((struct pt_regs*) \
- (task_stack_page(tsk) + KERNEL_STACK_SIZE - (XCHAL_NUM_AREGS-16)*4) - 1)
+ (task_stack_page(tsk) + KERNEL_STACK_SIZE) - 1)
# define user_mode(regs) (((regs)->ps & 0x00000020)!=0)
# define instruction_pointer(regs) ((regs)->pc)
# define return_pointer(regs) (MAKE_PC_FROM_RA((regs)->areg[0], \
diff --git a/arch/xtensa/kernel/process.c b/arch/xtensa/kernel/process.c
index bd80df890b1e..e8bfbca5f001 100644
--- a/arch/xtensa/kernel/process.c
+++ b/arch/xtensa/kernel/process.c
@@ -232,10 +232,6 @@ int copy_thread(unsigned long clone_flags, unsigned long usp_thread_fn,
p->thread.ra = MAKE_RA_FOR_CALL(
(unsigned long)ret_from_fork, 0x1);

- /* This does not copy all the regs.
- * In a bout of brilliance or madness,
- * ARs beyond a0-a15 exist past the end of the struct.
- */
*childregs = *regs;
childregs->areg[1] = usp;
childregs->areg[2] = 0;
@@ -265,14 +261,8 @@ int copy_thread(unsigned long clone_flags, unsigned long usp_thread_fn,
childregs->wmask = 1;
childregs->windowstart = 1;
childregs->windowbase = 0;
- } else {
- int len = childregs->wmask & ~0xf;
- memcpy(&childregs->areg[XCHAL_NUM_AREGS - len/4],
- &regs->areg[XCHAL_NUM_AREGS - len/4], len);
}

- childregs->syscall = regs->syscall;
-
if (clone_flags & CLONE_SETTLS)
childregs->threadptr = tls;
} else {
--
2.30.2

2022-03-08 01:24:09

by Kees Cook

Subject: Re: [PATCH 2/2] xtensa: use XCHAL_NUM_AREGS as pt_regs::areg size

On Sat, Mar 05, 2022 at 10:44:35PM -0800, Max Filippov wrote:
> struct pt_regs is used to access both kernel and user exception frames.
> User exception frames may contain up to XCHAL_NUM_AREG registers that
> task creation and signal delivery code may access, but pt_regs::areg
> array has only 16 entries that cover only the kernel exception frame.
> This results in the following build error:
>
> arch/xtensa/kernel/process.c: In function 'copy_thread':
> arch/xtensa/kernel/process.c:262:52: error: array subscript 53 is above
> array bounds of 'long unsigned int[16]' [-Werror=array-bounds]
> 262 | put_user(regs->areg[caller_ars+1],
>
> Change struct pt_regs::areg size to XCHAL_NUM_AREGS so that it covers
> the whole user exception frame. Adjust task_pt_regs and drop additional
> register copying code from copy_thread now that the whole user exception
> stack frame is copied.
>
> Reported-by: Kees Cook <[email protected]>
> Signed-off-by: Max Filippov <[email protected]>
> ---
> arch/xtensa/include/asm/ptrace.h | 7 +++----
> arch/xtensa/kernel/process.c | 10 ----------
> 2 files changed, 3 insertions(+), 14 deletions(-)
^^^^^^^^^^^^

Well that's always nice to see in a fix. :) Thanks for digging into
this!

Reviewed-by: Kees Cook <[email protected]>


--
Kees Cook