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

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

[Max Filippov]

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

[PATCH 1/2] xtensa: rename PT_SIZE to PT_KERNEL_SIZE

[Max Filippov]

PT_SIZE is used by the xtensa port to designate kernel exception frame
size. In preparation for struct pt_regs size change rename PT_SIZE to
PT_KERNEL_SIZE for clarity and change its definition to always cover
only the kernel exception frame.

Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
 arch/xtensa/kernel/asm-offsets.c |  2 +-
 arch/xtensa/kernel/entry.S       | 14 +++++++-------
 arch/xtensa/kernel/vectors.S     |  4 ++--
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/arch/xtensa/kernel/asm-offsets.c b/arch/xtensa/kernel/asm-offsets.c
index f1fd1390d069..37278e2785fb 100644
--- a/arch/xtensa/kernel/asm-offsets.c
+++ b/arch/xtensa/kernel/asm-offsets.c
@@ -63,7 +63,7 @@ int main(void)
 	DEFINE(PT_AREG15, offsetof (struct pt_regs, areg[15]));
 	DEFINE(PT_WINDOWBASE, offsetof (struct pt_regs, windowbase));
 	DEFINE(PT_WINDOWSTART, offsetof(struct pt_regs, windowstart));
-	DEFINE(PT_SIZE, sizeof(struct pt_regs));
+	DEFINE(PT_KERNEL_SIZE, offsetof(struct pt_regs, areg[16]));
 	DEFINE(PT_AREG_END, offsetof (struct pt_regs, areg[XCHAL_NUM_AREGS]));
 	DEFINE(PT_USER_SIZE, offsetof(struct pt_regs, areg[XCHAL_NUM_AREGS]));
 	DEFINE(PT_XTREGS_OPT, offsetof(struct pt_regs, xtregs_opt));
diff --git a/arch/xtensa/kernel/entry.S b/arch/xtensa/kernel/entry.S
index a1029a5b6a1d..77a7c8da3ff5 100644
--- a/arch/xtensa/kernel/entry.S
+++ b/arch/xtensa/kernel/entry.S
@@ -341,8 +341,8 @@ KABI_W	_bbsi.l	a2, 3, 1f
 	/* Copy spill slots of a0 and a1 to imitate movsp
 	 * in order to keep exception stack continuous
 	 */
-	l32i	a3, a1, PT_SIZE
-	l32i	a0, a1, PT_SIZE + 4
+	l32i	a3, a1, PT_KERNEL_SIZE
+	l32i	a0, a1, PT_KERNEL_SIZE + 4
 	s32e	a3, a1, -16
 	s32e	a0, a1, -12
 #endif
@@ -706,12 +706,12 @@ kernel_exception_exit:
 	addi    a0, a1, -16
 	l32i    a3, a0, 0
 	l32i    a4, a0, 4
-	s32i    a3, a1, PT_SIZE+0
-	s32i    a4, a1, PT_SIZE+4
+	s32i    a3, a1, PT_KERNEL_SIZE + 0
+	s32i    a4, a1, PT_KERNEL_SIZE + 4
 	l32i    a3, a0, 8
 	l32i    a4, a0, 12
-	s32i    a3, a1, PT_SIZE+8
-	s32i    a4, a1, PT_SIZE+12
+	s32i    a3, a1, PT_KERNEL_SIZE + 8
+	s32i    a4, a1, PT_KERNEL_SIZE + 12
 
 	/* Common exception exit.
 	 * We restore the special register and the current window frame, and
@@ -821,7 +821,7 @@ ENTRY(debug_exception)
 
 	bbsi.l	a2, PS_UM_BIT, 2f	# jump if user mode
 
-	addi	a2, a1, -16-PT_SIZE	# assume kernel stack
+	addi	a2, a1, -16 - PT_KERNEL_SIZE	# assume kernel stack
 3:
 	l32i	a0, a3, DT_DEBUG_SAVE
 	s32i	a1, a2, PT_AREG1
diff --git a/arch/xtensa/kernel/vectors.S b/arch/xtensa/kernel/vectors.S
index 407ece204e7c..1073fe4a584d 100644
--- a/arch/xtensa/kernel/vectors.S
+++ b/arch/xtensa/kernel/vectors.S
@@ -88,7 +88,7 @@ ENDPROC(_UserExceptionVector)
  * Kernel exception vector. (Exceptions with PS.UM == 0, PS.EXCM == 0)
  *
  * We get this exception when we were already in kernel space.
- * We decrement the current stack pointer (kernel) by PT_SIZE and
+ * We decrement the current stack pointer (kernel) by PT_KERNEL_SIZE and
  * jump to the first-level handler associated with the exception cause.
  *
  * Note: we need to preserve space for the spill region.
@@ -100,7 +100,7 @@ ENTRY(_KernelExceptionVector)
 
 	xsr	a3, excsave1		# save a3, and get dispatch table
 	wsr	a2, depc		# save a2
-	addi	a2, a1, -16-PT_SIZE	# adjust stack pointer
+	addi	a2, a1, -16 - PT_KERNEL_SIZE	# adjust stack pointer
 	s32i	a0, a2, PT_AREG0	# save a0 to ESF
 	rsr	a0, exccause		# retrieve exception cause
 	s32i	a0, a2, PT_DEPC		# mark it as a regular exception
-- 
2.30.2

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

[Max Filippov]

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 <keescook@chromium.org>
Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
 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

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

[Kees Cook]

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 <keescook@chromium.org>
> Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
> ---
>  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 <keescook@chromium.org>


-- 
Kees Cook

Re: [PATCH 1/2] xtensa: rename PT_SIZE to PT_KERNEL_SIZE

[Kees Cook]

On Sat, Mar 05, 2022 at 10:44:34PM -0800, Max Filippov wrote:
> PT_SIZE is used by the xtensa port to designate kernel exception frame
> size. In preparation for struct pt_regs size change rename PT_SIZE to
> PT_KERNEL_SIZE for clarity and change its definition to always cover
> only the kernel exception frame.
> 
> Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook