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

linux-hardening.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* How large is the xtensa pt_regs::areg array supposed to be?
@ 2022-02-02 23:03 Kees Cook
  2022-02-03 21:13 ` Max Filippov
  2022-03-05 20:43 ` Max Filippov
  0 siblings, 2 replies; 6+ messages in thread
From: Kees Cook @ 2022-02-02 23:03 UTC (permalink / raw)
  To: Chris Zankel, Max Filippov; +Cc: linux-xtensa, linux-hardening

Hi,

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?

Thanks!

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: How large is the xtensa pt_regs::areg array supposed to be?
  2022-02-02 23:03 How large is the xtensa pt_regs::areg array supposed to be? Kees Cook
@ 2022-02-03 21:13 ` Max Filippov
  2022-02-03 21:56   ` Kees Cook
  2022-03-05 20:43 ` Max Filippov
  1 sibling, 1 reply; 6+ messages in thread
From: Max Filippov @ 2022-02-03 21:13 UTC (permalink / raw)
  To: Kees Cook
  Cc: Chris Zankel, open list:TENSILICA XTENSA PORT (xtensa), linux-hardening

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.

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: How large is the xtensa pt_regs::areg array supposed to be?
  2022-02-03 21:13 ` Max Filippov
@ 2022-02-03 21:56   ` Kees Cook
  2022-02-04  0:05     ` Max Filippov
  0 siblings, 1 reply; 6+ messages in thread
From: Kees Cook @ 2022-02-03 21:56 UTC (permalink / raw)
  To: Max Filippov
  Cc: Chris Zankel, open list:TENSILICA XTENSA PORT (xtensa), linux-hardening

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: How large is the xtensa pt_regs::areg array supposed to be?
  2022-02-03 21:56   ` Kees Cook
@ 2022-02-04  0:05     ` Max Filippov
  0 siblings, 0 replies; 6+ messages in thread
From: Max Filippov @ 2022-02-04  0:05 UTC (permalink / raw)
  To: Kees Cook
  Cc: Chris Zankel, open list:TENSILICA XTENSA PORT (xtensa), linux-hardening

On Thu, Feb 3, 2022 at 1:56 PM Kees Cook <keescook@chromium.org> wrote:
> > > 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?

Correct. That's because the register window may move 4, 8 or 12
registers at a time, so it's always a multiple of 4 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.

I'd say that's the different kinds of stack frames on the kernel stack.
I don't think we expose the structure itself to the userspace anymore.

> > 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?

I agree. Let me try to do this change.

-- 
Thanks.
-- Max

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: How large is the xtensa pt_regs::areg array supposed to be?
  2022-02-02 23:03 How large is the xtensa pt_regs::areg array supposed to be? Kees Cook
  2022-02-03 21:13 ` Max Filippov
@ 2022-03-05 20:43 ` Max Filippov
  2022-03-06  1:50   ` Max Filippov
  1 sibling, 1 reply; 6+ messages in thread
From: Max Filippov @ 2022-03-05 20:43 UTC (permalink / raw)
  To: Kees Cook
  Cc: Chris Zankel, open list:TENSILICA XTENSA PORT (xtensa), linux-hardening

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]

I tried to reproduce this by building with -Warray-bounds added to
KBUILD_CFLAGS but couldn't get neither gcc-11 nor the current
gcc trunk to issue this warning. How did you get it?

-- 
Thanks.
-- Max

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: How large is the xtensa pt_regs::areg array supposed to be?
  2022-03-05 20:43 ` Max Filippov
@ 2022-03-06  1:50   ` Max Filippov
  0 siblings, 0 replies; 6+ messages in thread
From: Max Filippov @ 2022-03-06  1:50 UTC (permalink / raw)
  To: Kees Cook
  Cc: Chris Zankel, open list:TENSILICA XTENSA PORT (xtensa), linux-hardening

On Sat, Mar 5, 2022 at 12:43 PM Max Filippov <jcmvbkbc@gmail.com> wrote:
> 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]
>
> I tried to reproduce this by building with -Warray-bounds added to
> KBUILD_CFLAGS but couldn't get neither gcc-11 nor the current
> gcc trunk to issue this warning. How did you get it?

Never mind, it reproduces with the current -next tree.

-- 
Thanks.
-- Max

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-03-06  1:50 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-02 23:03 How large is the xtensa pt_regs::areg array supposed to be? Kees Cook
2022-02-03 21:13 ` Max Filippov
2022-02-03 21:56   ` Kees Cook
2022-02-04  0:05     ` Max Filippov
2022-03-05 20:43 ` Max Filippov
2022-03-06  1:50   ` Max Filippov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).