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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 3 Feb 2022 13:56:29 -0800 From: Kees Cook <keescook@...omium.org> To: Max Filippov <jcmvbkbc@...il.com> 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? 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@...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. 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
Powered by blists - more mailing lists