diff mbox series

[kernel,v2] powerpc/kuap: Restore AMR after replaying soft interrupts

Message ID 20201203054724.44838-1-aik@ozlabs.ru (mailing list archive)
State Superseded
Headers show
Series [kernel,v2] powerpc/kuap: Restore AMR after replaying soft interrupts | expand

Checks

Context Check Description
snowpatch_ozlabs/apply_patch success Successfully applied on branch powerpc/merge (a1aeabd25a36d9e019381278e543e2d538dd44a7)
snowpatch_ozlabs/build-ppc64le success Build succeeded
snowpatch_ozlabs/build-ppc64be success Build succeeded
snowpatch_ozlabs/build-ppc64e success Build succeeded
snowpatch_ozlabs/build-pmac32 fail Build failed!
snowpatch_ozlabs/checkpatch success total: 0 errors, 0 warnings, 0 checks, 52 lines checked
snowpatch_ozlabs/needsstable success Patch has no Fixes tags

Commit Message

Alexey Kardashevskiy Dec. 3, 2020, 5:47 a.m. UTC 
When interrupted in raw_copy_from_user()/... after user memory access
is enabled, a nested handler may also access user memory (perf is
one example) and when it does so, it calls prevent_read_from_user()
which prevents the upper handler from accessing user memory.

This saves/restores AMR when replaying interrupts.

get_kuap/set_kuap have stubs for disabled KUAP on RADIX but there are
none for hash-only configs (BOOK3E) so this adds stubs and moves
AMR_KUAP_BLOCK_xxx.

Found by syzkaller. More likely to break with enabled
CONFIG_DEBUG_ATOMIC_SLEEP, the call chain is
timer_interrupt -> ktime_get -> read_seqcount_begin -> local_irq_restore.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v2:
* fixed compile on hash
* moved get/set to arch_local_irq_restore
* block KUAP before replaying


---

This is an example:

------------[ cut here ]------------
Bug: Read fault blocked by AMR!
WARNING: CPU: 0 PID: 1603 at /home/aik/p/kernel/arch/powerpc/include/asm/book3s/64/kup-radix.h:145 __do_page_fau

Modules linked in:
CPU: 0 PID: 1603 Comm: amr Not tainted 5.10.0-rc6_v5.10-rc6_a+fstn1 #24
NIP:  c00000000009ece8 LR: c00000000009ece4 CTR: 0000000000000000
REGS: c00000000dc63560 TRAP: 0700   Not tainted  (5.10.0-rc6_v5.10-rc6_a+fstn1)
MSR:  8000000000021033 <SF,ME,IR,DR,RI,LE>  CR: 28002888  XER: 20040000
CFAR: c0000000001fa928 IRQMASK: 1
GPR00: c00000000009ece4 c00000000dc637f0 c000000002397600 000000000000001f
GPR04: c0000000020eb318 0000000000000000 c00000000dc63494 0000000000000027
GPR08: c00000007fe4de68 c00000000dfe9180 0000000000000000 0000000000000001
GPR12: 0000000000002000 c0000000030a0000 0000000000000000 0000000000000000
GPR16: 0000000000000000 0000000000000000 0000000000000000 bfffffffffffffff
GPR20: 0000000000000000 c0000000134a4020 c0000000019c2218 0000000000000fe0
GPR24: 0000000000000000 0000000000000000 c00000000d106200 0000000040000000
GPR28: 0000000000000000 0000000000000300 c00000000dc63910 c000000001946730
NIP [c00000000009ece8] __do_page_fault+0xb38/0xde0
LR [c00000000009ece4] __do_page_fault+0xb34/0xde0
Call Trace:
[c00000000dc637f0] [c00000000009ece4] __do_page_fault+0xb34/0xde0 (unreliable)
[c00000000dc638a0] [c00000000000c968] handle_page_fault+0x10/0x2c
--- interrupt: 300 at strncpy_from_user+0x290/0x440
    LR = strncpy_from_user+0x284/0x440
[c00000000dc63ba0] [c000000000c3dcb0] strncpy_from_user+0x2f0/0x440 (unreliable)
[c00000000dc63c30] [c00000000068b888] getname_flags+0x88/0x2c0
[c00000000dc63c90] [c000000000662a44] do_sys_openat2+0x2d4/0x5f0
[c00000000dc63d30] [c00000000066560c] do_sys_open+0xcc/0x140
[c00000000dc63dc0] [c000000000045e10] system_call_exception+0x160/0x240
[c00000000dc63e20] [c00000000000da60] system_call_common+0xf0/0x27c
Instruction dump:
409c0048 3fe2ff5b 3bfff128 fac10060 fae10068 482f7a85 60000000 3c62ff5b
7fe4fb78 3863f250 4815bbd9 60000000 <0fe00000> 3c62ff5b 3863f2b8 4815c8b5
irq event stamp: 254
hardirqs last  enabled at (253): [<c000000000019550>] arch_local_irq_restore+0xa0/0x150
hardirqs last disabled at (254): [<c000000000008a10>] data_access_common_virt+0x1b0/0x1d0
softirqs last  enabled at (0): [<c0000000001f6d5c>] copy_process+0x78c/0x2120
softirqs last disabled at (0): [<0000000000000000>] 0x0
---[ end trace ba98aec5151f3aeb ]---
---
 arch/powerpc/include/asm/book3s/64/kup-radix.h |  3 ---
 arch/powerpc/include/asm/kup.h                 | 10 ++++++++++
 arch/powerpc/kernel/irq.c                      |  6 ++++++
 3 files changed, 16 insertions(+), 3 deletions(-)

Comments

Aneesh Kumar K V Dec. 3, 2020, 6:38 a.m. UTC | #1 
Alexey Kardashevskiy <aik@ozlabs.ru> writes:

> When interrupted in raw_copy_from_user()/... after user memory access
> is enabled, a nested handler may also access user memory (perf is
> one example) and when it does so, it calls prevent_read_from_user()
> which prevents the upper handler from accessing user memory.
>
> This saves/restores AMR when replaying interrupts.
>
> get_kuap/set_kuap have stubs for disabled KUAP on RADIX but there are
> none for hash-only configs (BOOK3E) so this adds stubs and moves
> AMR_KUAP_BLOCK_xxx.
>
> Found by syzkaller. More likely to break with enabled
> CONFIG_DEBUG_ATOMIC_SLEEP, the call chain is
> timer_interrupt -> ktime_get -> read_seqcount_begin -> local_irq_restore.

Can you test this with https://github.com/kvaneesh/linux/commits/hash-kuap-reworked-2

We do save restore AMR on interrupt entry and exit.

-aneesh
kernel test robot Dec. 3, 2020, 7:41 a.m. UTC | #2 
Hi Alexey,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.10-rc6 next-20201201]
[cannot apply to powerpc/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Alexey-Kardashevskiy/powerpc-kuap-Restore-AMR-after-replaying-soft-interrupts/20201203-135010
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 34816d20f173a90389c8a7e641166d8ea9dce70a
config: powerpc-wii_defconfig (attached as .config)
compiler: powerpc-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/d2a99260ef6d241abe6fb961ee3ed84bbc5db7f5
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Alexey-Kardashevskiy/powerpc-kuap-Restore-AMR-after-replaying-soft-interrupts/20201203-135010
        git checkout d2a99260ef6d241abe6fb961ee3ed84bbc5db7f5
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from arch/powerpc/include/asm/uaccess.h:9,
                    from include/linux/uaccess.h:11,
                    from include/linux/sched/task.h:11,
                    from include/linux/sched/signal.h:9,
                    from include/linux/rcuwait.h:6,
                    from include/linux/percpu-rwsem.h:7,
                    from include/linux/fs.h:33,
                    from include/linux/compat.h:17,
                    from arch/powerpc/kernel/asm-offsets.c:14:
   arch/powerpc/include/asm/kup.h: In function 'get_kuap':
>> arch/powerpc/include/asm/kup.h:19:26: warning: conversion from 'long long unsigned int' to 'long unsigned int' changes value from '13835058055282163712' to '0' [-Woverflow]
      19 | #define AMR_KUAP_BLOCKED (AMR_KUAP_BLOCK_READ | AMR_KUAP_BLOCK_WRITE)
         |                          ^
   arch/powerpc/include/asm/kup.h:73:9: note: in expansion of macro 'AMR_KUAP_BLOCKED'
      73 |  return AMR_KUAP_BLOCKED;
         |         ^~~~~~~~~~~~~~~~
--
   In file included from arch/powerpc/include/asm/uaccess.h:9,
                    from include/linux/uaccess.h:11,
                    from include/linux/sched/task.h:11,
                    from include/linux/sched/signal.h:9,
                    from include/linux/rcuwait.h:6,
                    from include/linux/percpu-rwsem.h:7,
                    from include/linux/fs.h:33,
                    from include/linux/huge_mm.h:8,
                    from include/linux/mm.h:687,
                    from include/linux/ring_buffer.h:5,
                    from include/linux/trace_events.h:6,
                    from include/trace/trace_events.h:21,
                    from include/trace/define_trace.h:102,
                    from include/trace/events/sched.h:656,
                    from kernel/sched/core.c:10:
   arch/powerpc/include/asm/kup.h: In function 'get_kuap':
>> arch/powerpc/include/asm/kup.h:19:26: warning: conversion from 'long long unsigned int' to 'long unsigned int' changes value from '13835058055282163712' to '0' [-Woverflow]
      19 | #define AMR_KUAP_BLOCKED (AMR_KUAP_BLOCK_READ | AMR_KUAP_BLOCK_WRITE)
         |                          ^
   arch/powerpc/include/asm/kup.h:73:9: note: in expansion of macro 'AMR_KUAP_BLOCKED'
      73 |  return AMR_KUAP_BLOCKED;
         |         ^~~~~~~~~~~~~~~~
   kernel/sched/core.c: In function 'ttwu_stat':
   kernel/sched/core.c:2419:13: warning: variable 'rq' set but not used [-Wunused-but-set-variable]
    2419 |  struct rq *rq;
         |             ^~
--
   In file included from arch/powerpc/include/asm/uaccess.h:9,
                    from include/linux/uaccess.h:11,
                    from include/linux/sched/task.h:11,
                    from include/linux/sched/signal.h:9,
                    from include/linux/sched/cputime.h:5,
                    from kernel/sched/sched.h:11,
                    from kernel/sched/fair.c:23:
   arch/powerpc/include/asm/kup.h: In function 'get_kuap':
>> arch/powerpc/include/asm/kup.h:19:26: warning: conversion from 'long long unsigned int' to 'long unsigned int' changes value from '13835058055282163712' to '0' [-Woverflow]
      19 | #define AMR_KUAP_BLOCKED (AMR_KUAP_BLOCK_READ | AMR_KUAP_BLOCK_WRITE)
         |                          ^
   arch/powerpc/include/asm/kup.h:73:9: note: in expansion of macro 'AMR_KUAP_BLOCKED'
      73 |  return AMR_KUAP_BLOCKED;
         |         ^~~~~~~~~~~~~~~~
   kernel/sched/fair.c: At top level:
   kernel/sched/fair.c:5368:6: warning: no previous prototype for 'init_cfs_bandwidth' [-Wmissing-prototypes]
    5368 | void init_cfs_bandwidth(struct cfs_bandwidth *cfs_b) {}
         |      ^~~~~~~~~~~~~~~~~~
   kernel/sched/fair.c:11150:6: warning: no previous prototype for 'free_fair_sched_group' [-Wmissing-prototypes]
   11150 | void free_fair_sched_group(struct task_group *tg) { }
         |      ^~~~~~~~~~~~~~~~~~~~~
   kernel/sched/fair.c:11152:5: warning: no previous prototype for 'alloc_fair_sched_group' [-Wmissing-prototypes]
   11152 | int alloc_fair_sched_group(struct task_group *tg, struct task_group *parent)
         |     ^~~~~~~~~~~~~~~~~~~~~~
   kernel/sched/fair.c:11157:6: warning: no previous prototype for 'online_fair_sched_group' [-Wmissing-prototypes]
   11157 | void online_fair_sched_group(struct task_group *tg) { }
         |      ^~~~~~~~~~~~~~~~~~~~~~~
   kernel/sched/fair.c:11159:6: warning: no previous prototype for 'unregister_fair_sched_group' [-Wmissing-prototypes]
   11159 | void unregister_fair_sched_group(struct task_group *tg) { }
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~
--
   In file included from arch/powerpc/include/asm/uaccess.h:9,
                    from include/linux/uaccess.h:11,
                    from include/linux/sched/task.h:11,
                    from include/linux/sched/signal.h:9,
                    from include/linux/sched/cputime.h:5,
                    from kernel/sched/sched.h:11,
                    from kernel/sched/rt.c:6:
   arch/powerpc/include/asm/kup.h: In function 'get_kuap':
>> arch/powerpc/include/asm/kup.h:19:26: warning: conversion from 'long long unsigned int' to 'long unsigned int' changes value from '13835058055282163712' to '0' [-Woverflow]
      19 | #define AMR_KUAP_BLOCKED (AMR_KUAP_BLOCK_READ | AMR_KUAP_BLOCK_WRITE)
         |                          ^
   arch/powerpc/include/asm/kup.h:73:9: note: in expansion of macro 'AMR_KUAP_BLOCKED'
      73 |  return AMR_KUAP_BLOCKED;
         |         ^~~~~~~~~~~~~~~~
   kernel/sched/rt.c: At top level:
   kernel/sched/rt.c:253:6: warning: no previous prototype for 'free_rt_sched_group' [-Wmissing-prototypes]
     253 | void free_rt_sched_group(struct task_group *tg) { }
         |      ^~~~~~~~~~~~~~~~~~~
   kernel/sched/rt.c:255:5: warning: no previous prototype for 'alloc_rt_sched_group' [-Wmissing-prototypes]
     255 | int alloc_rt_sched_group(struct task_group *tg, struct task_group *parent)
         |     ^~~~~~~~~~~~~~~~~~~~
   kernel/sched/rt.c:668:6: warning: no previous prototype for 'sched_rt_bandwidth_account' [-Wmissing-prototypes]
     668 | bool sched_rt_bandwidth_account(struct rt_rq *rt_rq)
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~~
--
   In file included from arch/powerpc/include/asm/uaccess.h:9,
                    from include/linux/uaccess.h:11,
                    from include/linux/sched/task.h:11,
                    from include/linux/sched/signal.h:9,
                    from include/linux/rcuwait.h:6,
                    from include/linux/percpu-rwsem.h:7,
                    from include/linux/fs.h:33,
                    from include/linux/compat.h:17,
                    from arch/powerpc/kernel/asm-offsets.c:14:
   arch/powerpc/include/asm/kup.h: In function 'get_kuap':
>> arch/powerpc/include/asm/kup.h:19:26: warning: conversion from 'long long unsigned int' to 'long unsigned int' changes value from '13835058055282163712' to '0' [-Woverflow]
      19 | #define AMR_KUAP_BLOCKED (AMR_KUAP_BLOCK_READ | AMR_KUAP_BLOCK_WRITE)
         |                          ^
   arch/powerpc/include/asm/kup.h:73:9: note: in expansion of macro 'AMR_KUAP_BLOCKED'
      73 |  return AMR_KUAP_BLOCKED;
         |         ^~~~~~~~~~~~~~~~

vim +19 arch/powerpc/include/asm/kup.h

    16	
    17	#define AMR_KUAP_BLOCK_READ	UL(0x4000000000000000)
    18	#define AMR_KUAP_BLOCK_WRITE	UL(0x8000000000000000)
  > 19	#define AMR_KUAP_BLOCKED	(AMR_KUAP_BLOCK_READ | AMR_KUAP_BLOCK_WRITE)
    20	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Christophe Leroy Dec. 3, 2020, 8:03 a.m. UTC | #3 
Le 03/12/2020 à 06:47, Alexey Kardashevskiy a écrit :
> When interrupted in raw_copy_from_user()/... after user memory access
> is enabled, a nested handler may also access user memory (perf is
> one example) and when it does so, it calls prevent_read_from_user()
> which prevents the upper handler from accessing user memory.
> 
> This saves/restores AMR when replaying interrupts.
> 
> get_kuap/set_kuap have stubs for disabled KUAP on RADIX but there are
> none for hash-only configs (BOOK3E) so this adds stubs and moves
> AMR_KUAP_BLOCK_xxx.
> 
> Found by syzkaller. More likely to break with enabled
> CONFIG_DEBUG_ATOMIC_SLEEP, the call chain is
> timer_interrupt -> ktime_get -> read_seqcount_begin -> local_irq_restore.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> Changes:
> v2:
> * fixed compile on hash
> * moved get/set to arch_local_irq_restore
> * block KUAP before replaying
> 
> 
> ---
> 
> This is an example:
> 
> ------------[ cut here ]------------
> Bug: Read fault blocked by AMR!
> WARNING: CPU: 0 PID: 1603 at /home/aik/p/kernel/arch/powerpc/include/asm/book3s/64/kup-radix.h:145 __do_page_fau
> 
> Modules linked in:
> CPU: 0 PID: 1603 Comm: amr Not tainted 5.10.0-rc6_v5.10-rc6_a+fstn1 #24
> NIP:  c00000000009ece8 LR: c00000000009ece4 CTR: 0000000000000000
> REGS: c00000000dc63560 TRAP: 0700   Not tainted  (5.10.0-rc6_v5.10-rc6_a+fstn1)
> MSR:  8000000000021033 <SF,ME,IR,DR,RI,LE>  CR: 28002888  XER: 20040000
> CFAR: c0000000001fa928 IRQMASK: 1
> GPR00: c00000000009ece4 c00000000dc637f0 c000000002397600 000000000000001f
> GPR04: c0000000020eb318 0000000000000000 c00000000dc63494 0000000000000027
> GPR08: c00000007fe4de68 c00000000dfe9180 0000000000000000 0000000000000001
> GPR12: 0000000000002000 c0000000030a0000 0000000000000000 0000000000000000
> GPR16: 0000000000000000 0000000000000000 0000000000000000 bfffffffffffffff
> GPR20: 0000000000000000 c0000000134a4020 c0000000019c2218 0000000000000fe0
> GPR24: 0000000000000000 0000000000000000 c00000000d106200 0000000040000000
> GPR28: 0000000000000000 0000000000000300 c00000000dc63910 c000000001946730
> NIP [c00000000009ece8] __do_page_fault+0xb38/0xde0
> LR [c00000000009ece4] __do_page_fault+0xb34/0xde0
> Call Trace:
> [c00000000dc637f0] [c00000000009ece4] __do_page_fault+0xb34/0xde0 (unreliable)
> [c00000000dc638a0] [c00000000000c968] handle_page_fault+0x10/0x2c
> --- interrupt: 300 at strncpy_from_user+0x290/0x440
>      LR = strncpy_from_user+0x284/0x440
> [c00000000dc63ba0] [c000000000c3dcb0] strncpy_from_user+0x2f0/0x440 (unreliable)
> [c00000000dc63c30] [c00000000068b888] getname_flags+0x88/0x2c0
> [c00000000dc63c90] [c000000000662a44] do_sys_openat2+0x2d4/0x5f0
> [c00000000dc63d30] [c00000000066560c] do_sys_open+0xcc/0x140
> [c00000000dc63dc0] [c000000000045e10] system_call_exception+0x160/0x240
> [c00000000dc63e20] [c00000000000da60] system_call_common+0xf0/0x27c
> Instruction dump:
> 409c0048 3fe2ff5b 3bfff128 fac10060 fae10068 482f7a85 60000000 3c62ff5b
> 7fe4fb78 3863f250 4815bbd9 60000000 <0fe00000> 3c62ff5b 3863f2b8 4815c8b5
> irq event stamp: 254
> hardirqs last  enabled at (253): [<c000000000019550>] arch_local_irq_restore+0xa0/0x150
> hardirqs last disabled at (254): [<c000000000008a10>] data_access_common_virt+0x1b0/0x1d0
> softirqs last  enabled at (0): [<c0000000001f6d5c>] copy_process+0x78c/0x2120
> softirqs last disabled at (0): [<0000000000000000>] 0x0
> ---[ end trace ba98aec5151f3aeb ]---
> ---
>   arch/powerpc/include/asm/book3s/64/kup-radix.h |  3 ---
>   arch/powerpc/include/asm/kup.h                 | 10 ++++++++++
>   arch/powerpc/kernel/irq.c                      |  6 ++++++
>   3 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h b/arch/powerpc/include/asm/book3s/64/kup-radix.h
> index a39e2d193fdc..4ad607461b75 100644
> --- a/arch/powerpc/include/asm/book3s/64/kup-radix.h
> +++ b/arch/powerpc/include/asm/book3s/64/kup-radix.h
> @@ -5,9 +5,6 @@
>   #include <linux/const.h>
>   #include <asm/reg.h>
>   
> -#define AMR_KUAP_BLOCK_READ	UL(0x4000000000000000)
> -#define AMR_KUAP_BLOCK_WRITE	UL(0x8000000000000000)
> -#define AMR_KUAP_BLOCKED	(AMR_KUAP_BLOCK_READ | AMR_KUAP_BLOCK_WRITE)
>   #define AMR_KUAP_SHIFT		62
>   
>   #ifdef __ASSEMBLY__
> diff --git a/arch/powerpc/include/asm/kup.h b/arch/powerpc/include/asm/kup.h
> index 0d93331d0fab..e63930767a96 100644
> --- a/arch/powerpc/include/asm/kup.h
> +++ b/arch/powerpc/include/asm/kup.h
> @@ -14,6 +14,10 @@
>   #define KUAP_CURRENT_WRITE	8
>   #define KUAP_CURRENT		(KUAP_CURRENT_READ | KUAP_CURRENT_WRITE)
>   
> +#define AMR_KUAP_BLOCK_READ	UL(0x4000000000000000)
> +#define AMR_KUAP_BLOCK_WRITE	UL(0x8000000000000000)
> +#define AMR_KUAP_BLOCKED	(AMR_KUAP_BLOCK_READ | AMR_KUAP_BLOCK_WRITE)
> +

Those macro are specific to BOOK3S_64, they have nothing to do in asm/kup.h, must stay in the file 
included just below

>   #ifdef CONFIG_PPC_BOOK3S_64
>   #include <asm/book3s/64/kup-radix.h>
>   #endif
> @@ -64,6 +68,12 @@ bad_kuap_fault(struct pt_regs *regs, unsigned long address, bool is_write)
>   }
>   
>   static inline void kuap_check_amr(void) { }
> +static inline unsigned long get_kuap(void)
> +{
> +	return AMR_KUAP_BLOCKED;
> +}
> +

The above is not generic, it is specific to book3s 64, AMR doesn't exist on book3s/32 or on 8xx.

> +static inline void set_kuap(unsigned long value) { }
>   
>   /*
>    * book3s/64/kup-radix.h defines these functions for the !KUAP case to flush
> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
> index 7d0f7682d01d..d9fd46da04d6 100644
> --- a/arch/powerpc/kernel/irq.c
> +++ b/arch/powerpc/kernel/irq.c
> @@ -314,6 +314,7 @@ void replay_soft_interrupts(void)
>   notrace void arch_local_irq_restore(unsigned long mask)
>   {
>   	unsigned char irq_happened;
> +	unsigned long kuap_state;
>   
>   	/* Write the new soft-enabled value */
>   	irq_soft_mask_set(mask);
> @@ -373,9 +374,14 @@ notrace void arch_local_irq_restore(unsigned long mask)
>   	irq_soft_mask_set(IRQS_ALL_DISABLED);
>   	trace_hardirqs_off();
>   
> +	kuap_state = get_kuap();
> +	set_kuap(AMR_KUAP_BLOCKED);
> +
>   	replay_soft_interrupts();
>   	local_paca->irq_happened = 0;
>   
> +	set_kuap(kuap_state);
> +
>   	trace_hardirqs_on();
>   	irq_soft_mask_set(IRQS_ENABLED);
>   	__hard_irq_enable();
>
Alexey Kardashevskiy Dec. 3, 2020, 8:10 a.m. UTC | #4 
On 03/12/2020 17:38, Aneesh Kumar K.V wrote:
> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
> 
>> When interrupted in raw_copy_from_user()/... after user memory access
>> is enabled, a nested handler may also access user memory (perf is
>> one example) and when it does so, it calls prevent_read_from_user()
>> which prevents the upper handler from accessing user memory.
>>
>> This saves/restores AMR when replaying interrupts.
>>
>> get_kuap/set_kuap have stubs for disabled KUAP on RADIX but there are
>> none for hash-only configs (BOOK3E) so this adds stubs and moves
>> AMR_KUAP_BLOCK_xxx.
>>
>> Found by syzkaller. More likely to break with enabled
>> CONFIG_DEBUG_ATOMIC_SLEEP, the call chain is
>> timer_interrupt -> ktime_get -> read_seqcount_begin -> local_irq_restore.
> 
> Can you test this with https://github.com/kvaneesh/linux/commits/hash-kuap-reworked-2

Yup, broken:

[    8.813115] ------------[ cut here ]------------ 

[    8.813499] Bug: Read fault blocked by AMR! 

[    8.813532] WARNING: CPU: 0 PID: 1113 at 
/home/aik/p/kernel/arch/powerpc/include/asm/book3s/64/kup.h:369 
__do_page_fault+0xd
34/0xdf0 

[    8.814248] Modules linked in: 

[    8.814459] CPU: 0 PID: 1113 Comm: amr Not tainted 
5.10.0-rc4_aneesh--hash-kuap-reworked-2_a+fstn1 #61
[    8.815075] NIP:  c0000000000a4674 LR: c0000000000a4670 CTR: 
0000000000000000
[    8.815632] REGS: c00000000d44f530 TRAP: 0700   Not tainted 
(5.10.0-rc4_aneesh--hash-kuap-reworked-2_a+fstn1)



> We do save restore AMR on interrupt entry and exit.

This is nested interrupt. we get interrupted while copying to/from user. 
The first handler has AMR off, then it goes to strncpy_from_user, 
enables AMR, page fault happens and another interrupt arrives - and if 
the new interrupt also wants strncpy_from_user/co, it will enable AMR 
again (which is ok), do copy and then disable AMR; and then we go back 
and resume the first strncpy_from_user which thinks AMR is enabling 
while it is not. I just see how strncpy_from_user is called while 
strncpy_from_user is running. Nick understands the mechanics better.

I can give you an initramdisk which crashes in a millisecond, ping me in 
slack.
Alexey Kardashevskiy Dec. 3, 2020, 8:13 a.m. UTC | #5 
On 03/12/2020 19:03, Christophe Leroy wrote:
> 
> 
> Le 03/12/2020 à 06:47, Alexey Kardashevskiy a écrit :
>> When interrupted in raw_copy_from_user()/... after user memory access
>> is enabled, a nested handler may also access user memory (perf is
>> one example) and when it does so, it calls prevent_read_from_user()
>> which prevents the upper handler from accessing user memory.
>>
>> This saves/restores AMR when replaying interrupts.
>>
>> get_kuap/set_kuap have stubs for disabled KUAP on RADIX but there are
>> none for hash-only configs (BOOK3E) so this adds stubs and moves
>> AMR_KUAP_BLOCK_xxx.
>>
>> Found by syzkaller. More likely to break with enabled
>> CONFIG_DEBUG_ATOMIC_SLEEP, the call chain is
>> timer_interrupt -> ktime_get -> read_seqcount_begin -> local_irq_restore.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> Changes:
>> v2:
>> * fixed compile on hash
>> * moved get/set to arch_local_irq_restore
>> * block KUAP before replaying
>>
>>
>> ---
>>
>> This is an example:
>>
>> ------------[ cut here ]------------
>> Bug: Read fault blocked by AMR!
>> WARNING: CPU: 0 PID: 1603 at 
>> /home/aik/p/kernel/arch/powerpc/include/asm/book3s/64/kup-radix.h:145 
>> __do_page_fau
>>
>> Modules linked in:
>> CPU: 0 PID: 1603 Comm: amr Not tainted 5.10.0-rc6_v5.10-rc6_a+fstn1 #24
>> NIP:  c00000000009ece8 LR: c00000000009ece4 CTR: 0000000000000000
>> REGS: c00000000dc63560 TRAP: 0700   Not tainted  
>> (5.10.0-rc6_v5.10-rc6_a+fstn1)
>> MSR:  8000000000021033 <SF,ME,IR,DR,RI,LE>  CR: 28002888  XER: 20040000
>> CFAR: c0000000001fa928 IRQMASK: 1
>> GPR00: c00000000009ece4 c00000000dc637f0 c000000002397600 
>> 000000000000001f
>> GPR04: c0000000020eb318 0000000000000000 c00000000dc63494 
>> 0000000000000027
>> GPR08: c00000007fe4de68 c00000000dfe9180 0000000000000000 
>> 0000000000000001
>> GPR12: 0000000000002000 c0000000030a0000 0000000000000000 
>> 0000000000000000
>> GPR16: 0000000000000000 0000000000000000 0000000000000000 
>> bfffffffffffffff
>> GPR20: 0000000000000000 c0000000134a4020 c0000000019c2218 
>> 0000000000000fe0
>> GPR24: 0000000000000000 0000000000000000 c00000000d106200 
>> 0000000040000000
>> GPR28: 0000000000000000 0000000000000300 c00000000dc63910 
>> c000000001946730
>> NIP [c00000000009ece8] __do_page_fault+0xb38/0xde0
>> LR [c00000000009ece4] __do_page_fault+0xb34/0xde0
>> Call Trace:
>> [c00000000dc637f0] [c00000000009ece4] __do_page_fault+0xb34/0xde0 
>> (unreliable)
>> [c00000000dc638a0] [c00000000000c968] handle_page_fault+0x10/0x2c
>> --- interrupt: 300 at strncpy_from_user+0x290/0x440
>>      LR = strncpy_from_user+0x284/0x440
>> [c00000000dc63ba0] [c000000000c3dcb0] strncpy_from_user+0x2f0/0x440 
>> (unreliable)
>> [c00000000dc63c30] [c00000000068b888] getname_flags+0x88/0x2c0
>> [c00000000dc63c90] [c000000000662a44] do_sys_openat2+0x2d4/0x5f0
>> [c00000000dc63d30] [c00000000066560c] do_sys_open+0xcc/0x140
>> [c00000000dc63dc0] [c000000000045e10] system_call_exception+0x160/0x240
>> [c00000000dc63e20] [c00000000000da60] system_call_common+0xf0/0x27c
>> Instruction dump:
>> 409c0048 3fe2ff5b 3bfff128 fac10060 fae10068 482f7a85 60000000 3c62ff5b
>> 7fe4fb78 3863f250 4815bbd9 60000000 <0fe00000> 3c62ff5b 3863f2b8 4815c8b5
>> irq event stamp: 254
>> hardirqs last  enabled at (253): [<c000000000019550>] 
>> arch_local_irq_restore+0xa0/0x150
>> hardirqs last disabled at (254): [<c000000000008a10>] 
>> data_access_common_virt+0x1b0/0x1d0
>> softirqs last  enabled at (0): [<c0000000001f6d5c>] 
>> copy_process+0x78c/0x2120
>> softirqs last disabled at (0): [<0000000000000000>] 0x0
>> ---[ end trace ba98aec5151f3aeb ]---
>> ---
>>   arch/powerpc/include/asm/book3s/64/kup-radix.h |  3 ---
>>   arch/powerpc/include/asm/kup.h                 | 10 ++++++++++
>>   arch/powerpc/kernel/irq.c                      |  6 ++++++
>>   3 files changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h 
>> b/arch/powerpc/include/asm/book3s/64/kup-radix.h
>> index a39e2d193fdc..4ad607461b75 100644
>> --- a/arch/powerpc/include/asm/book3s/64/kup-radix.h
>> +++ b/arch/powerpc/include/asm/book3s/64/kup-radix.h
>> @@ -5,9 +5,6 @@
>>   #include <linux/const.h>
>>   #include <asm/reg.h>
>> -#define AMR_KUAP_BLOCK_READ    UL(0x4000000000000000)
>> -#define AMR_KUAP_BLOCK_WRITE    UL(0x8000000000000000)
>> -#define AMR_KUAP_BLOCKED    (AMR_KUAP_BLOCK_READ | AMR_KUAP_BLOCK_WRITE)
>>   #define AMR_KUAP_SHIFT        62
>>   #ifdef __ASSEMBLY__
>> diff --git a/arch/powerpc/include/asm/kup.h 
>> b/arch/powerpc/include/asm/kup.h
>> index 0d93331d0fab..e63930767a96 100644
>> --- a/arch/powerpc/include/asm/kup.h
>> +++ b/arch/powerpc/include/asm/kup.h
>> @@ -14,6 +14,10 @@
>>   #define KUAP_CURRENT_WRITE    8
>>   #define KUAP_CURRENT        (KUAP_CURRENT_READ | KUAP_CURRENT_WRITE)
>> +#define AMR_KUAP_BLOCK_READ    UL(0x4000000000000000)
>> +#define AMR_KUAP_BLOCK_WRITE    UL(0x8000000000000000)
>> +#define AMR_KUAP_BLOCKED    (AMR_KUAP_BLOCK_READ | AMR_KUAP_BLOCK_WRITE)
>> +
> 
> Those macro are specific to BOOK3S_64, they have nothing to do in 
> asm/kup.h, must stay in the file included just below
> 
>>   #ifdef CONFIG_PPC_BOOK3S_64
>>   #include <asm/book3s/64/kup-radix.h>
>>   #endif
>> @@ -64,6 +68,12 @@ bad_kuap_fault(struct pt_regs *regs, unsigned long 
>> address, bool is_write)
>>   }
>>   static inline void kuap_check_amr(void) { }
>> +static inline unsigned long get_kuap(void)
>> +{
>> +    return AMR_KUAP_BLOCKED;
>> +}
>> +
> 
> The above is not generic, it is specific to book3s 64, AMR doesn't exist 
> on book3s/32 or on 8xx.


ah ok, I did not want more ifdefs there but looks like it is 
unavoidable, I'll repost if there are no other comments. Thanks,
kernel test robot Dec. 4, 2020, 9:02 p.m. UTC | #6 
Hi Alexey,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.10-rc6 next-20201204]
[cannot apply to powerpc/next scottwood/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Alexey-Kardashevskiy/powerpc-kuap-Restore-AMR-after-replaying-soft-interrupts/20201203-135010
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 34816d20f173a90389c8a7e641166d8ea9dce70a
config: powerpc64-randconfig-r023-20201204 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 32c501dd88b62787d3a5ffda7aabcf4650dbe3cd)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install powerpc64 cross compiling tool for clang build
        # apt-get install binutils-powerpc64-linux-gnu
        # https://github.com/0day-ci/linux/commit/d2a99260ef6d241abe6fb961ee3ed84bbc5db7f5
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Alexey-Kardashevskiy/powerpc-kuap-Restore-AMR-after-replaying-soft-interrupts/20201203-135010
        git checkout d2a99260ef6d241abe6fb961ee3ed84bbc5db7f5
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from arch/powerpc/kernel/asm-offsets.c:14:
   In file included from include/linux/compat.h:17:
   In file included from include/linux/fs.h:33:
   In file included from include/linux/percpu-rwsem.h:7:
   In file included from include/linux/rcuwait.h:6:
   In file included from include/linux/sched/signal.h:9:
   In file included from include/linux/sched/task.h:11:
   In file included from include/linux/uaccess.h:11:
   In file included from arch/powerpc/include/asm/uaccess.h:9:
>> arch/powerpc/include/asm/kup.h:71:29: error: redefinition of 'get_kuap'
   static inline unsigned long get_kuap(void)
                               ^
   arch/powerpc/include/asm/book3s/64/kup-radix.h:152:29: note: previous definition is here
   static inline unsigned long get_kuap(void)
                               ^
   In file included from arch/powerpc/kernel/asm-offsets.c:14:
   In file included from include/linux/compat.h:17:
   In file included from include/linux/fs.h:33:
   In file included from include/linux/percpu-rwsem.h:7:
   In file included from include/linux/rcuwait.h:6:
   In file included from include/linux/sched/signal.h:9:
   In file included from include/linux/sched/task.h:11:
   In file included from include/linux/uaccess.h:11:
   In file included from arch/powerpc/include/asm/uaccess.h:9:
>> arch/powerpc/include/asm/kup.h:76:20: error: redefinition of 'set_kuap'
   static inline void set_kuap(unsigned long value) { }
                      ^
   arch/powerpc/include/asm/book3s/64/kup-radix.h:157:20: note: previous definition is here
   static inline void set_kuap(unsigned long value) { }
                      ^
   In file included from arch/powerpc/kernel/asm-offsets.c:21:
   include/linux/mman.h:155:9: warning: division by zero is undefined [-Wdivision-by-zero]
                  _calc_vm_trans(flags, MAP_LOCKED,     VM_LOCKED    ) |
                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/mman.h:133:21: note: expanded from macro '_calc_vm_trans'
      : ((x) & (bit1)) / ((bit1) / (bit2))))
                       ^ ~~~~~~~~~~~~~~~~~
   include/linux/mman.h:156:9: warning: division by zero is undefined [-Wdivision-by-zero]
                  _calc_vm_trans(flags, MAP_SYNC,       VM_SYNC      ) |
                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/mman.h:133:21: note: expanded from macro '_calc_vm_trans'
      : ((x) & (bit1)) / ((bit1) / (bit2))))
                       ^ ~~~~~~~~~~~~~~~~~
   2 warnings and 2 errors generated.
--
   In file included from arch/powerpc/kernel/asm-offsets.c:14:
   In file included from include/linux/compat.h:17:
   In file included from include/linux/fs.h:33:
   In file included from include/linux/percpu-rwsem.h:7:
   In file included from include/linux/rcuwait.h:6:
   In file included from include/linux/sched/signal.h:9:
   In file included from include/linux/sched/task.h:11:
   In file included from include/linux/uaccess.h:11:
   In file included from arch/powerpc/include/asm/uaccess.h:9:
>> arch/powerpc/include/asm/kup.h:71:29: error: redefinition of 'get_kuap'
   static inline unsigned long get_kuap(void)
                               ^
   arch/powerpc/include/asm/book3s/64/kup-radix.h:152:29: note: previous definition is here
   static inline unsigned long get_kuap(void)
                               ^
   In file included from arch/powerpc/kernel/asm-offsets.c:14:
   In file included from include/linux/compat.h:17:
   In file included from include/linux/fs.h:33:
   In file included from include/linux/percpu-rwsem.h:7:
   In file included from include/linux/rcuwait.h:6:
   In file included from include/linux/sched/signal.h:9:
   In file included from include/linux/sched/task.h:11:
   In file included from include/linux/uaccess.h:11:
   In file included from arch/powerpc/include/asm/uaccess.h:9:
>> arch/powerpc/include/asm/kup.h:76:20: error: redefinition of 'set_kuap'
   static inline void set_kuap(unsigned long value) { }
                      ^
   arch/powerpc/include/asm/book3s/64/kup-radix.h:157:20: note: previous definition is here
   static inline void set_kuap(unsigned long value) { }
                      ^
   In file included from arch/powerpc/kernel/asm-offsets.c:21:
   include/linux/mman.h:155:9: warning: division by zero is undefined [-Wdivision-by-zero]
                  _calc_vm_trans(flags, MAP_LOCKED,     VM_LOCKED    ) |
                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/mman.h:133:21: note: expanded from macro '_calc_vm_trans'
      : ((x) & (bit1)) / ((bit1) / (bit2))))
                       ^ ~~~~~~~~~~~~~~~~~
   include/linux/mman.h:156:9: warning: division by zero is undefined [-Wdivision-by-zero]
                  _calc_vm_trans(flags, MAP_SYNC,       VM_SYNC      ) |
                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/mman.h:133:21: note: expanded from macro '_calc_vm_trans'
      : ((x) & (bit1)) / ((bit1) / (bit2))))
                       ^ ~~~~~~~~~~~~~~~~~
   2 warnings and 2 errors generated.
   make[2]: *** [scripts/Makefile.build:117: arch/powerpc/kernel/asm-offsets.s] Error 1
   make[2]: Target '__build' not remade because of errors.
   make[1]: *** [Makefile:1198: prepare0] Error 2
   make[1]: Target 'prepare' not remade because of errors.
   make: *** [Makefile:185: __sub-make] Error 2
   make: Target 'prepare' not remade because of errors.

vim +/get_kuap +71 arch/powerpc/include/asm/kup.h

    69	
    70	static inline void kuap_check_amr(void) { }
  > 71	static inline unsigned long get_kuap(void)
    72	{
    73		return AMR_KUAP_BLOCKED;
    74	}
    75	
  > 76	static inline void set_kuap(unsigned long value) { }
    77	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/arch/powerpc/include/asm/book3s/64/kup-radix.h b/arch/powerpc/include/asm/book3s/64/kup-radix.h
index a39e2d193fdc..4ad607461b75 100644
--- a/arch/powerpc/include/asm/book3s/64/kup-radix.h
+++ b/arch/powerpc/include/asm/book3s/64/kup-radix.h
@@ -5,9 +5,6 @@ 
 #include <linux/const.h>
 #include <asm/reg.h>
 
-#define AMR_KUAP_BLOCK_READ	UL(0x4000000000000000)
-#define AMR_KUAP_BLOCK_WRITE	UL(0x8000000000000000)
-#define AMR_KUAP_BLOCKED	(AMR_KUAP_BLOCK_READ | AMR_KUAP_BLOCK_WRITE)
 #define AMR_KUAP_SHIFT		62
 
 #ifdef __ASSEMBLY__
diff --git a/arch/powerpc/include/asm/kup.h b/arch/powerpc/include/asm/kup.h
index 0d93331d0fab..e63930767a96 100644
--- a/arch/powerpc/include/asm/kup.h
+++ b/arch/powerpc/include/asm/kup.h
@@ -14,6 +14,10 @@ 
 #define KUAP_CURRENT_WRITE	8
 #define KUAP_CURRENT		(KUAP_CURRENT_READ | KUAP_CURRENT_WRITE)
 
+#define AMR_KUAP_BLOCK_READ	UL(0x4000000000000000)
+#define AMR_KUAP_BLOCK_WRITE	UL(0x8000000000000000)
+#define AMR_KUAP_BLOCKED	(AMR_KUAP_BLOCK_READ | AMR_KUAP_BLOCK_WRITE)
+
 #ifdef CONFIG_PPC_BOOK3S_64
 #include <asm/book3s/64/kup-radix.h>
 #endif
@@ -64,6 +68,12 @@  bad_kuap_fault(struct pt_regs *regs, unsigned long address, bool is_write)
 }
 
 static inline void kuap_check_amr(void) { }
+static inline unsigned long get_kuap(void)
+{
+	return AMR_KUAP_BLOCKED;
+}
+
+static inline void set_kuap(unsigned long value) { }
 
 /*
  * book3s/64/kup-radix.h defines these functions for the !KUAP case to flush
diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index 7d0f7682d01d..d9fd46da04d6 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c
@@ -314,6 +314,7 @@  void replay_soft_interrupts(void)
 notrace void arch_local_irq_restore(unsigned long mask)
 {
 	unsigned char irq_happened;
+	unsigned long kuap_state;
 
 	/* Write the new soft-enabled value */
 	irq_soft_mask_set(mask);
@@ -373,9 +374,14 @@  notrace void arch_local_irq_restore(unsigned long mask)
 	irq_soft_mask_set(IRQS_ALL_DISABLED);
 	trace_hardirqs_off();
 
+	kuap_state = get_kuap();
+	set_kuap(AMR_KUAP_BLOCKED);
+
 	replay_soft_interrupts();
 	local_paca->irq_happened = 0;
 
+	set_kuap(kuap_state);
+
 	trace_hardirqs_on();
 	irq_soft_mask_set(IRQS_ENABLED);
 	__hard_irq_enable();