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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 3 Feb 2022 13:13:26 -0800
From:   Max Filippov <jcmvbkbc@...il.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     Chris Zankel <chris@...kel.net>,
        "open list:TENSILICA XTENSA PORT (xtensa)" 
        <linux-xtensa@...ux-xtensa.org>, linux-hardening@...r.kernel.org
Subject: Re: How large is the xtensa pt_regs::areg array supposed to be?

Hi Kees,

On Wed, Feb 2, 2022 at 3:03 PM Kees Cook <keescook@...omium.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.

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.

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.

-- 
Thanks.
-- Max