Re: How large is the xtensa pt_regs::areg array supposed to be?

[Kees Cook <keescook@chromium.org>]

On Thu, Feb 03, 2022 at 01:13:26PM -0800, Max Filippov wrote:
> Hi Kees,
> 
> On Wed, Feb 2, 2022 at 3:03 PM Kees Cook <keescook@chromium.org> wrote:
> > When building with -Warray-bounds, I see this:
> >
> > In file included from ./include/linux/uaccess.h:11,
> >                  from ./include/linux/sched/task.h:11,
> >                  from arch/xtensa/kernel/process.c:21:
> > arch/xtensa/kernel/process.c: In function 'copy_thread':
> > arch/xtensa/kernel/process.c:262:52: warning: array subscript 53 is above array bounds of 'long unsigned int[16]' [-Warray-bounds]
> >   262 |                                 put_user(regs->areg[caller_ars+1],
> > ./arch/xtensa/include/asm/uaccess.h:171:18: note: in definition of macro '__put_user_asm'
> >   171 |         :[x] "r"(x_), [efault] "i"(-EFAULT))
> >       |                  ^~
> > ./arch/xtensa/include/asm/uaccess.h:89:17: note: in expansion of macro '__put_user_size'
> >    89 |                 __put_user_size((x), __pu_addr, (size), __pu_err);      \
> >       |                 ^~~~~~~~~~~~~~~
> > ./arch/xtensa/include/asm/uaccess.h:62:33: note: in expansion of macro '__put_user_check'
> >    62 | #define put_user(x, ptr)        __put_user_check((x), (ptr), sizeof(*(ptr)))
> >       |                                 ^~~~~~~~~~~~~~~~
> > arch/xtensa/kernel/process.c:262:33: note: in expansion of macro 'put_user'
> >   262 |                                 put_user(regs->areg[caller_ars+1],
> >       |                                 ^~~~~~~~
> > In file included from ./arch/xtensa/include/asm/processor.h:17,
> >                  from ./arch/xtensa/include/asm/thread_info.h:20,
> >                  from ./arch/xtensa/include/asm/current.h:14,
> >                  from ./include/linux/sched.h:12,
> >                  from arch/xtensa/kernel/process.c:19:
> > ./arch/xtensa/include/asm/ptrace.h:80:23: note: while referencing 'areg'
> >    80 |         unsigned long areg[16];
> >       |                       ^~~~
> >
> >
> > The code is:
> >                                 int callinc = (regs->areg[0] >> 30) & 3;
> >                                 int caller_ars = XCHAL_NUM_AREGS - callinc * 4;
> >                                 put_user(regs->areg[caller_ars+1],
> >                                          (unsigned __user*)(usp - 12));
> >
> > It looks like XCHAL_NUM_AREGS is larger than "16", though?
> >
> > struct pt_regs {
> > ...
> >         unsigned long areg[16];
> >
> > What should be happening here?
> 
> pt_regs::areg is the current register window, but when we enter
> the kernel from the userspace additional valid registers up the call
> stack are saved here:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/xtensa/kernel/entry.S?h=v5.16#n204
> This is done because when the kernel is built with windowed ABI
> we cannot have a mix of user and kernel registers in the physical
> register file.

This is a bit opaque for me to read, but it looks like it's a loop of
4-at-a-time of the "extra" registers?

> The space for these registers is reserved here:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/xtensa/kernel/entry.S?h=v5.16#n2102
> by adding PT_REGS_OFFSET, which accounts for the
> XCHAL_NUM_AREGS value, to the task stack address.

Okay, so it seems like there are two "views" of the registers from an
ABI perspective, the userspace view (struct pt_regs), and the kernel
view which is struct pt_regs + more.

> On the other hand, when the kernel is re-entered we don't need
> to save registers up the kernel stack, so the pt_regs structure
> exactly represents the kernel stack frame.
> 
> The xtensa architecture has configurable windowed registers option.
> When it's enabled the machine has more than 16 general purpose
> registers: typically 32 or 64, but only 16 of them are in the current
> window and may be used by instructions.
> The window rotates forward on entry to functions and backwards
> on return. Additional special registers track current window
> position and valid registers.
> 
> For some reason during xtensa port development it was chosen
> to have pt_regs::areg only cover the current register window. I guess
> this was done as a common denominator for the user and kernel
> stack frames and to avoid an exposure of XCHAL_NUM_AREGS
> constant in the user-visible headers. Chris may have additional details.

Right, that makes sense: pt_regs should be the shared user/kernel view.

The compiler is mad about trying to access the extra registers from the
pt_reg struct, so maybe a kernel-only struct could be used here?

-- 
Kees Cook