Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 12 Aug 2021 20:00:46 +1000
From:   Nicholas Piggin <npiggin@gmail.com>
Subject: Re: [PATCH] powerpc/interrupt: Fix OOPS by not calling do_IRQ() from
 timer_interrupt()
To:     Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Christophe Leroy <christophe.leroy@csgroup.eu>,
        fthain@linux-m68k.org, Michael Ellerman <mpe@ellerman.id.au>,
        Paul Mackerras <paulus@samba.org>, userm57@yahoo.com
Cc:     linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org
References: <c17d234f4927d39a1d7100864a8e1145323d33a0.1628611927.git.christophe.leroy@csgroup.eu>
In-Reply-To: <c17d234f4927d39a1d7100864a8e1145323d33a0.1628611927.git.christophe.leroy@csgroup.eu>
MIME-Version: 1.0
Message-Id: <1628762421.8aij8ncawp.astroid@bobo.none>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Excerpts from Christophe Leroy's message of August 11, 2021 2:13 am:
> An interrupt handler shall not be called from another interrupt
> handler otherwise this leads to problems like the following:
>=20
> 	Kernel attempted to write user page (afd4fa84) - exploit attempt? (uid: =
1000)
> 	------------[ cut here ]------------
> 	Bug: Write fault blocked by KUAP!
> 	WARNING: CPU: 0 PID: 1617 at arch/powerpc/mm/fault.c:230 do_page_fault+0=
x484/0x720
> 	Modules linked in:
> 	CPU: 0 PID: 1617 Comm: sshd Tainted: G        W         5.13.0-pmac-0001=
0-g8393422eb77 #7
> 	NIP:  c001b77c LR: c001b77c CTR: 00000000
> 	REGS: cb9e5bc0 TRAP: 0700   Tainted: G        W          (5.13.0-pmac-00=
010-g8393422eb77)
> 	MSR:  00021032 <ME,IR,DR,RI>  CR: 24942424  XER: 00000000
>=20
> 	GPR00: c001b77c cb9e5c80 c1582c00 00000021 3ffffbff 085b0000 00000027 c8=
eb644c
> 	GPR08: 00000023 00000000 00000000 00000000 24942424 0063f8c8 00000000 00=
0186a0
> 	GPR16: afd52dd4 afd52dd0 afd52dcc afd52dc8 0065a990 c07640c4 cb9e5e98 cb=
9e5e90
> 	GPR24: 00000040 afd4fa96 00000040 02000000 c1fda6c0 afd4fa84 00000300 cb=
9e5cc0
> 	NIP [c001b77c] do_page_fault+0x484/0x720
> 	LR [c001b77c] do_page_fault+0x484/0x720
> 	Call Trace:
> 	[cb9e5c80] [c001b77c] do_page_fault+0x484/0x720 (unreliable)
> 	[cb9e5cb0] [c000424c] DataAccess_virt+0xd4/0xe4
> 	--- interrupt: 300 at __copy_tofrom_user+0x110/0x20c
> 	NIP:  c001f9b4 LR: c03250a0 CTR: 00000004
> 	REGS: cb9e5cc0 TRAP: 0300   Tainted: G        W          (5.13.0-pmac-00=
010-g8393422eb77)
> 	MSR:  00009032 <EE,ME,IR,DR,RI>  CR: 48028468  XER: 20000000
> 	DAR: afd4fa84 DSISR: 0a000000
> 	GPR00: 20726f6f cb9e5d80 c1582c00 00000004 cb9e5e3a 00000016 afd4fa80 00=
000000
> 	GPR08: 3835202d 72777872 2d78722d 00000004 28028464 0063f8c8 00000000 00=
0186a0
> 	GPR16: afd52dd4 afd52dd0 afd52dcc afd52dc8 0065a990 c07640c4 cb9e5e98 cb=
9e5e90
> 	GPR24: 00000040 afd4fa96 00000040 cb9e5e0c 00000daa a0000000 cb9e5e98 af=
d4fa56
> 	NIP [c001f9b4] __copy_tofrom_user+0x110/0x20c
> 	LR [c03250a0] _copy_to_iter+0x144/0x990
> 	--- interrupt: 300
> 	[cb9e5d80] [c03e89c0] n_tty_read+0xa4/0x598 (unreliable)
> 	[cb9e5df0] [c03e2a0c] tty_read+0xdc/0x2b4
> 	[cb9e5e80] [c0156bf8] vfs_read+0x274/0x340
> 	[cb9e5f00] [c01571ac] ksys_read+0x70/0x118
> 	[cb9e5f30] [c0016048] ret_from_syscall+0x0/0x28
> 	--- interrupt: c00 at 0xa7855c88
> 	NIP:  a7855c88 LR: a7855c5c CTR: 00000000
> 	REGS: cb9e5f40 TRAP: 0c00   Tainted: G        W          (5.13.0-pmac-00=
010-g8393422eb77)
> 	MSR:  0000d032 <EE,PR,ME,IR,DR,RI>  CR: 2402446c  XER: 00000000
>=20
> 	GPR00: 00000003 afd4ec70 a72137d0 0000000b afd4ecac 00004000 0065a990 00=
000800
> 	GPR08: 00000000 a7947930 00000000 00000004 c15831b0 0063f8c8 00000000 00=
0186a0
> 	GPR16: afd52dd4 afd52dd0 afd52dcc afd52dc8 0065a990 0065a9e0 00000001 00=
65fac0
> 	GPR24: 00000000 00000089 00664050 00000000 00668e30 a720c8dc a7943ff4 00=
65f9b0
> 	NIP [a7855c88] 0xa7855c88
> 	LR [a7855c5c] 0xa7855c5c
> 	--- interrupt: c00
> 	Instruction dump:
> 	3884aa88 38630178 48076861 807f0080 48042e45 2f830000 419e0148 3c80c079
> 	3c60c076 38841be4 386301c0 4801f705 <0fe00000> 3860000b 4bfffe30 3c80c06=
b
> 	---[ end trace fd69b91a8046c2e5 ]---
>=20
> Here the problem is that by re-enterring an exception handler,
> kuap_save_and_lock() is called a second time with this time KUAP
> access locked, leading to regs->kuap being overwritten hence
> KUAP not being unlocked at exception exit as expected.
>=20
> Do not call do_IRQ() from timer_interrupt() directly. Instead,
> redefine do_IRQ() as a standard function named __do_IRQ(), and
> call it from both do_IRQ() and time_interrupt() handlers.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

>=20
> Reported-by: Stan Johnson <userm57@yahoo.com>
> Fixes: 3a96570ffceb ("powerpc: convert interrupt handlers to use wrappers=
")
> Cc: stable@vger.kernel.org
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Cc: Finn Thain <fthain@linux-m68k.org>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  arch/powerpc/include/asm/interrupt.h | 3 +++
>  arch/powerpc/include/asm/irq.h       | 2 +-
>  arch/powerpc/kernel/irq.c            | 7 ++++++-
>  arch/powerpc/kernel/time.c           | 2 +-
>  4 files changed, 11 insertions(+), 3 deletions(-)
>=20
> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/=
asm/interrupt.h
> index fc4702bdd119..1e984a35a39f 100644
> --- a/arch/powerpc/include/asm/interrupt.h
> +++ b/arch/powerpc/include/asm/interrupt.h
> @@ -590,6 +590,9 @@ DECLARE_INTERRUPT_HANDLER_NMI(hmi_exception_realmode)=
;
> =20
>  DECLARE_INTERRUPT_HANDLER_ASYNC(TAUException);
> =20
> +/* irq.c */
> +DECLARE_INTERRUPT_HANDLER_ASYNC(do_IRQ);
> +
>  void __noreturn unrecoverable_exception(struct pt_regs *regs);
> =20
>  void replay_system_reset(void);
> diff --git a/arch/powerpc/include/asm/irq.h b/arch/powerpc/include/asm/ir=
q.h
> index 4982f3711fc3..2b3278534bc1 100644
> --- a/arch/powerpc/include/asm/irq.h
> +++ b/arch/powerpc/include/asm/irq.h
> @@ -52,7 +52,7 @@ extern void *mcheckirq_ctx[NR_CPUS];
>  extern void *hardirq_ctx[NR_CPUS];
>  extern void *softirq_ctx[NR_CPUS];
> =20
> -extern void do_IRQ(struct pt_regs *regs);
> +void __do_IRQ(struct pt_regs *regs);
>  extern void __init init_IRQ(void);
>  extern void __do_irq(struct pt_regs *regs);
> =20
> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
> index 91e63eac4e8f..551b653228c4 100644
> --- a/arch/powerpc/kernel/irq.c
> +++ b/arch/powerpc/kernel/irq.c
> @@ -750,7 +750,7 @@ void __do_irq(struct pt_regs *regs)
>  	trace_irq_exit(regs);
>  }
> =20
> -DEFINE_INTERRUPT_HANDLER_ASYNC(do_IRQ)
> +void __do_IRQ(struct pt_regs *regs)
>  {
>  	struct pt_regs *old_regs =3D set_irq_regs(regs);
>  	void *cursp, *irqsp, *sirqsp;
> @@ -774,6 +774,11 @@ DEFINE_INTERRUPT_HANDLER_ASYNC(do_IRQ)
>  	set_irq_regs(old_regs);
>  }
> =20
> +DEFINE_INTERRUPT_HANDLER_ASYNC(do_IRQ)
> +{
> +	__do_IRQ(regs);
> +}
> +
>  static void *__init alloc_vm_stack(void)
>  {
>  	return __vmalloc_node(THREAD_SIZE, THREAD_ALIGN, THREADINFO_GFP,
> diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
> index e45ce427bffb..c487ba5a6e11 100644
> --- a/arch/powerpc/kernel/time.c
> +++ b/arch/powerpc/kernel/time.c
> @@ -586,7 +586,7 @@ DEFINE_INTERRUPT_HANDLER_ASYNC(timer_interrupt)
> =20
>  #if defined(CONFIG_PPC32) && defined(CONFIG_PPC_PMAC)
>  	if (atomic_read(&ppc_n_lost_interrupts) !=3D 0)
> -		do_IRQ(regs);
> +		__do_IRQ(regs);
>  #endif
> =20
>  	old_regs =3D set_irq_regs(regs);
> --=20
> 2.25.0
>=20
>=20