ARM: mm: Disregard user space addresses in BUG() address check

Commit Message

Ard Biesheuvel Jan. 17, 2024, 3:07 p.m. UTC

From: Ard Biesheuvel <[e-mail]>

is_valid_bugaddr() dereferences the faulting PC to fetch the instruction
that triggered the fault, to decide whether it is a BRK instruction used
to force an exception. This is used by the BUG() infrastructure to keep
the handling logic (which should never execute) separate from the code
that normally runs.

This dereference may attempt to access user memory if the faulting PC
happens to contain a user address. One way this might happen is when
the kernel is tricked into executing from user space while PAN
protections (Privileged Access Never) are in effect: the instruction
fetch will trigger a prefetch abort, the handling of which involves a
check whether the instruction that caused it is a BRK, requiring a
load from the same address. This load is privileged too, and so it will
trigger another exception, which we fail to recover from.

Given that BRK instructions tied to BUG() handling can only appear in
kernel code, let's check first that the PC actually points into kernel
memory.

Cc: Kees Cook <[e-mail]>
Cc: Russell King <[e-mail]>
Cc: Mark Brown <[e-mail]>
Cc: Zhen Lei <[e-mail]>
Cc: Linus Walleij <[e-mail]>
Link: https://lkml.kernel.org/r/202401111544.18EBB6AA%40keescook
Signed-off-by: Ard Biesheuvel <[e-mail]>
---
 arch/arm/kernel/traps.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Mark Brown Jan. 17, 2024, 6:25 p.m. UTC | #1

On Wed, Jan 17, 2024 at 04:07:34PM +0100, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <[e-mail]>
> 
> is_valid_bugaddr() dereferences the faulting PC to fetch the instruction
> that triggered the fault, to decide whether it is a BRK instruction used
> to force an exception. This is used by the BUG() infrastructure to keep
> the handling logic (which should never execute) separate from the code
> that normally runs.

...

> Given that BRK instructions tied to BUG() handling can only appear in
> kernel code, let's check first that the PC actually points into kernel
> memory.

This makes sense to me and resolves the LKDTM issues I originally
reported to Kees, the LKDTM selftests now finish rather than causing the
board to reset mid test.

Reviewed-by: Mark Brown <[e-mail]>
Tested-by: Mark Brown <[e-mail]>

Kees, you can see a test run at:

  https://lava.sirena.org.uk/scheduler/job/466606

Ard Biesheuvel Jan. 18, 2024, 1:16 p.m. UTC | #2

On Wed, 17 Jan 2024 at 19:25, Mark Brown <[e-mail]> wrote:
>
> On Wed, Jan 17, 2024 at 04:07:34PM +0100, Ard Biesheuvel wrote:
> > From: Ard Biesheuvel <[e-mail]>
> >
> > is_valid_bugaddr() dereferences the faulting PC to fetch the instruction
> > that triggered the fault, to decide whether it is a BRK instruction used
> > to force an exception. This is used by the BUG() infrastructure to keep
> > the handling logic (which should never execute) separate from the code
> > that normally runs.
>
> ...
>
> > Given that BRK instructions tied to BUG() handling can only appear in
> > kernel code, let's check first that the PC actually points into kernel
> > memory.
>
> This makes sense to me and resolves the LKDTM issues I originally
> reported to Kees, the LKDTM selftests now finish rather than causing the
> board to reset mid test.
>
> Reviewed-by: Mark Brown <[e-mail]>
> Tested-by: Mark Brown <[e-mail]>
>
> Kees, you can see a test run at:
>
>   https://lava.sirena.org.uk/scheduler/job/466606

Thanks

I've dropped this into the patch tracker as #9348

Kees Cook Jan. 18, 2024, 8:15 p.m. UTC | #3

On Wed, Jan 17, 2024 at 04:07:34PM +0100, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <[e-mail]>
>
> is_valid_bugaddr() dereferences the faulting PC to fetch the instruction
> that triggered the fault, to decide whether it is a BRK instruction used
> to force an exception. This is used by the BUG() infrastructure to keep
> the handling logic (which should never execute) separate from the code
> that normally runs.
>
> This dereference may attempt to access user memory if the faulting PC
> happens to contain a user address. One way this might happen is when
> the kernel is tricked into executing from user space while PAN
> protections (Privileged Access Never) are in effect: the instruction
> fetch will trigger a prefetch abort, the handling of which involves a
> check whether the instruction that caused it is a BRK, requiring a
> load from the same address. This load is privileged too, and so it will
> trigger another exception, which we fail to recover from.
>
> Given that BRK instructions tied to BUG() handling can only appear in
> kernel code, let's check first that the PC actually points into kernel
> memory.
>
> Cc: Kees Cook <[e-mail]>
> Cc: Russell King <[e-mail]>
> Cc: Mark Brown <[e-mail]>
> Cc: Zhen Lei <[e-mail]>
> Cc: Linus Walleij <[e-mail]>
> Link: https://lkml.kernel.org/r/202401111544.18EBB6AA%40keescook
> Signed-off-by: Ard Biesheuvel <[e-mail]>
> ---
>  arch/arm/kernel/traps.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> index 3bad79db5d6e..f342bd6b2a5d 100644
> --- a/arch/arm/kernel/traps.c
> +++ b/arch/arm/kernel/traps.c
> @@ -402,6 +402,9 @@ int is_valid_bugaddr(unsigned long pc)
>  	u32 insn = __opcode_to_mem_arm(BUG_INSTR_VALUE);
>  #endif
>
> +	if (pc < TASK_SIZE)
> +		return 0;
> +
>  	if (get_kernel_nofault(bkpt, (void *)pc))
>  		return 0;
>

Okay, I've finally had a chance to test this and it doesn't fix it
for me. I still see the double domain fault, and I still lose a CPU.
I patched in a pr_info() to show that the "pc < TASK_SIZE" was hitting
(which it is) so I see:


kdtm: Performing direct entry EXEC_USERSPACE
lkdtm: attempting ok execution at 80761b4c
lkdtm: attempting bad execution at 76ff4000
Unhandled prefetch abort: page domain fault (0x01b) at 0x76ff4000
Skipping is_valid_bugaddr(76ff4000)
Internal error: : 1b [#12] SMP ARM
Modules linked in:
...
Process cat (pid: 1301, stack limit = 0x3c8ec418)
Stack: (0xf0975e58 to 0xf0976000)
...
Backtrace:
 lkdtm_EXEC_USERSPACE from lkdtm_do_action+0x2c/0x4c
...
Exception stack(0xf0975fa8 to 0xf0975ff0)
...
8<--- cut here ---
Unhandled fault: page domain fault (0x01b) at 0x76ff3ff0
[76ff3ff0] *pgd=44baa835, *pte=00000000, *ppte=00000000
Internal error: : 1b [#13] SMP ARM
Modules linked in:
...
Process cat (pid: 1301, stack limit = 0x3c8ec418)
Stack: (0xf0975ce0 to 0xf0976000)
...
Backtrace:
 copy_from_kernel_nofault from dump_instr+0x1cc/0x208
 r7:f0975d27 r6:80ddf3d8 r5:f0975e08 r4:fffffffc
 dump_instr from die+0x294/0x2f0
 r10:8110a3dc r9:8100eb60 r8:60070193 r7:82760000 r6:0000001b r5:80df6c8c
 r4:f0975e08
 die from arm_notify_die+0x54/0x58
 r10:82760000 r9:82760000 r8:810ab75c r7:81010058 r6:f0975e08 r5:76ff4000
 r4:0000001b
 arm_notify_die from do_PrefetchAbort+0x90/0x98
 do_PrefetchAbort from __pabt_svc+0x5c/0xa0
Exception stack(0xf0975e08 to 0xf0975e50)
...
Exception stack(0xf0975fa8 to 0xf0975ff0)
...
---[ end trace 0000000000000000 ]---
note: cat[1301] exited with irqs disabled

But then that CPU is gone.

For example, on a 2 CPU arm32 instance, if I run EXEC_USERSPACE twice
the whole system dies. If I run it once, it will eventually die. Along
the way I'll see:

rcu: INFO: rcu_sched detected stalls on CPUs/tasks:
rcu:    1-...0: (1 ticks this GP) idle=199c/1/0x40000000 softirq=3056/3056 fqs=4702
rcu:    (detected by 0, t=10407 jiffies, g=4113, q=51 ncpus=2)
Sending NMI from CPU 0 to CPUs 1:

But I never get the system back.

Compared to ACCESS_USERSPACE, I see:

lkdtm: Performing direct entry ACCESS_USERSPACE
lkdtm: attempting bad read at 76f2b000
8<--- cut here ---
Unhandled fault: page domain fault (0x01b) at 0x76f2b000
[76f2b000] *pgd=42591835, *pte=4691055f, *ppte=46910c7e
Internal error: : 1b [#1] SMP ARM
Modules linked in:
...
Process cat (pid: 1286, stack limit = 0x9ff640f6)
Stack: (0xf0965e50 to 0xf0966000)
...
Backtrace:
 lkdtm_ACCESS_USERSPACE from lkdtm_do_action+0x2c/0x4c
...
Exception stack(0xf0965fa8 to 0xf0965ff0)
...
---[ end trace 0000000000000000 ]---

So it looks like everything between the "lkdtm: " lines and the
"8<--- cut here ---" line is unexpected in the EXEC_USERSPACE case?

I tried combinations of:

CONFIG_UNWINDER_FRAME_POINTER
CONFIG_UNWINDER_ARM
CONFIG_DEBUG_USER

but nothing changed the behavior. Further pr_info() debugging shows me:


lkdtm: attempting bad execution at 76fc2000
Unhandled prefetch abort: page domain fault (0x01b) at 0x76fc2000
user_mode(regs) is false
Skipping is_valid_bugaddr(76fc2000)
Internal error: : 1b [#1] SMP ARM
...
Exception stack(0xf0a99fa8 to 0xf0a99ff0)
9fa0:                   00000074 7ff00000 00000001 76a08000 0000000f 00000000
9fc0: 00000074 7ff00000 00000000 00000004 00000001 00000000 00020000 00000000
9fe0: 00000004 7ea81a88 76f315b3 76eba746
8<--- cut here (do_DataAbort) ---
Unhandled fault: page domain fault (0x01b) at 0x76fc1ff0
[76fc1ff0] *pgd=45da2835, *pte=00000000, *ppte=00000000
user_mode(regs) is false
Already called die!

If I skip the second die, I end up in a loop:

8<--- cut here (do_DataAbort) ---
Unhandled fault: page domain fault (0x01b) at 0x76fcdff0
[76fcdff0] *pgd=45dd6835, *pte=00000000, *ppte=00000000
user_mode(regs) is false
Already called die -- skipping this one.
8<--- cut here (do_DataAbort) ---
...


But if I comment out dump_instr(), I don't get the double fault, and the
CPU survives. So, borrowing from Ard's patch, this fixes it for me:

diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
index f342bd6b2a5d..3874afccb574 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -300,7 +300,8 @@ static int __die(const char *str, int err, struct pt_regs *regs)
 			 ALIGN(regs->ARM_sp - THREAD_SIZE, THREAD_ALIGN)
 			 + THREAD_SIZE);
 		dump_backtrace(regs, tsk, KERN_EMERG);
-		dump_instr(KERN_EMERG, regs);
+		if (instruction_pointer(regs) >= TASK_SIZE)
+			dump_instr(KERN_EMERG, regs);
 	}
 
 	return 0;


Is this the right approach?

Also, sticking with arm32 tradition, it seems a "cut here" is missing
for the prefetch case:

diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
index e96fb40b9cc3..df44f83c9c41 100644
--- a/arch/arm/mm/fault.c
+++ b/arch/arm/mm/fault.c
@@ -586,6 +586,7 @@ do_PrefetchAbort(unsigned long addr, unsigned int ifsr, struct pt_regs *regs)
 	if (!inf->fn(addr, ifsr | FSR_LNX_PF, regs))
 		return;
 
+	pr_alert("8<--- cut here ---\n");
 	pr_alert("Unhandled prefetch abort: %s (0x%03x) at 0x%08lx\n",
 		inf->name, ifsr, addr);
 

--
Kees Cook

Linus Walleij Jan. 18, 2024, 8:35 p.m. UTC | #4

Unsolicited by-the-way:

I will post an updated version of Catalin's PAN-on-LPAE patches
after v6.8-rc1, but you can already look at them if you need to know
what's coming:
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-integrator.git/log/?h=b4/arm32-lpae-pan

I also have CFI with CLANG cooking for ARM32, it boots to userspace
on some systems even.

Yours,
Linus Walleij

Ard Biesheuvel Jan. 19, 2024, 11:52 a.m. UTC | #5

On Thu, 18 Jan 2024 at 21:15, Kees Cook <[e-mail]> wrote:
>
> On Wed, Jan 17, 2024 at 04:07:34PM +0100, Ard Biesheuvel wrote:
> > From: Ard Biesheuvel <[e-mail]>
> >
> > is_valid_bugaddr() dereferences the faulting PC to fetch the instruction
> > that triggered the fault, to decide whether it is a BRK instruction used
> > to force an exception. This is used by the BUG() infrastructure to keep
> > the handling logic (which should never execute) separate from the code
> > that normally runs.
> >
> > This dereference may attempt to access user memory if the faulting PC
> > happens to contain a user address. One way this might happen is when
> > the kernel is tricked into executing from user space while PAN
> > protections (Privileged Access Never) are in effect: the instruction
> > fetch will trigger a prefetch abort, the handling of which involves a
> > check whether the instruction that caused it is a BRK, requiring a
> > load from the same address. This load is privileged too, and so it will
> > trigger another exception, which we fail to recover from.
> >
> > Given that BRK instructions tied to BUG() handling can only appear in
> > kernel code, let's check first that the PC actually points into kernel
> > memory.
> >
> > Cc: Kees Cook <[e-mail]>
> > Cc: Russell King <[e-mail]>
> > Cc: Mark Brown <[e-mail]>
> > Cc: Zhen Lei <[e-mail]>
> > Cc: Linus Walleij <[e-mail]>
> > Link: https://lkml.kernel.org/r/202401111544.18EBB6AA%40keescook
> > Signed-off-by: Ard Biesheuvel <[e-mail]>
> > ---
> >  arch/arm/kernel/traps.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> > index 3bad79db5d6e..f342bd6b2a5d 100644
> > --- a/arch/arm/kernel/traps.c
> > +++ b/arch/arm/kernel/traps.c
> > @@ -402,6 +402,9 @@ int is_valid_bugaddr(unsigned long pc)
> >       u32 insn = __opcode_to_mem_arm(BUG_INSTR_VALUE);
> >  #endif
> >
> > +     if (pc < TASK_SIZE)
> > +             return 0;
> > +
> >       if (get_kernel_nofault(bkpt, (void *)pc))
> >               return 0;
> >
>
> Okay, I've finally had a chance to test this and it doesn't fix it
> for me. I still see the double domain fault, and I still lose a CPU.
> I patched in a pr_info() to show that the "pc < TASK_SIZE" was hitting
> (which it is) so I see:
>
>
> kdtm: Performing direct entry EXEC_USERSPACE
> lkdtm: attempting ok execution at 80761b4c
> lkdtm: attempting bad execution at 76ff4000
> Unhandled prefetch abort: page domain fault (0x01b) at 0x76ff4000
> Skipping is_valid_bugaddr(76ff4000)
> Internal error: : 1b [#12] SMP ARM
> Modules linked in:
> ...
> Process cat (pid: 1301, stack limit = 0x3c8ec418)
> Stack: (0xf0975e58 to 0xf0976000)
> ...
> Backtrace:
>  lkdtm_EXEC_USERSPACE from lkdtm_do_action+0x2c/0x4c
> ...
> Exception stack(0xf0975fa8 to 0xf0975ff0)
> ...
> 8<--- cut here ---
> Unhandled fault: page domain fault (0x01b) at 0x76ff3ff0
> [76ff3ff0] *pgd=44baa835, *pte=00000000, *ppte=00000000
> Internal error: : 1b [#13] SMP ARM
> Modules linked in:
> ...
> Process cat (pid: 1301, stack limit = 0x3c8ec418)
> Stack: (0xf0975ce0 to 0xf0976000)
> ...
> Backtrace:
>  copy_from_kernel_nofault from dump_instr+0x1cc/0x208
>  r7:f0975d27 r6:80ddf3d8 r5:f0975e08 r4:fffffffc
>  dump_instr from die+0x294/0x2f0

This is indeed a similar but different issue (as you conclude below).
dump_instr() attempts to dereference PC to generate the 'Code: xxxx
xxxx' line with the opcodes leading up to the fault, and this triggers
a PAN fault for the same reason.
...
>
> But if I comment out dump_instr(), I don't get the double fault, and the
> CPU survives. So, borrowing from Ard's patch, this fixes it for me:
>
> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> index f342bd6b2a5d..3874afccb574 100644
> --- a/arch/arm/kernel/traps.c
> +++ b/arch/arm/kernel/traps.c
> @@ -300,7 +300,8 @@ static int __die(const char *str, int err, struct pt_regs *regs)
>                          ALIGN(regs->ARM_sp - THREAD_SIZE, THREAD_ALIGN)
>                          + THREAD_SIZE);
>                 dump_backtrace(regs, tsk, KERN_EMERG);
> -               dump_instr(KERN_EMERG, regs);
> +               if (instruction_pointer(regs) >= TASK_SIZE)
> +                       dump_instr(KERN_EMERG, regs);
>         }
>
>         return 0;
>
>
> Is this the right approach?
>

Yes.

Not sure why Mark nor me were seeing this in testing, but obviously,
reading from user memory is not appropriate here either.

Note that there is another occurrence of this for CONFIG_DEBUG_USER=y,
but I suppose that feature could be made dependent on !PAN entirely.


> Also, sticking with arm32 tradition, it seems a "cut here" is missing
> for the prefetch case:
>
> diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
> index e96fb40b9cc3..df44f83c9c41 100644
> --- a/arch/arm/mm/fault.c
> +++ b/arch/arm/mm/fault.c
> @@ -586,6 +586,7 @@ do_PrefetchAbort(unsigned long addr, unsigned int ifsr, struct pt_regs *regs)
>         if (!inf->fn(addr, ifsr | FSR_LNX_PF, regs))
>                 return;
>
> +       pr_alert("8<--- cut here ---\n");
>         pr_alert("Unhandled prefetch abort: %s (0x%03x) at 0x%08lx\n",
>                 inf->name, ifsr, addr);
>

Yes, let's add that as well.

Russell King (Oracle) Jan. 19, 2024, 12:14 p.m. UTC | #6

On Fri, Jan 19, 2024 at 12:52:21PM +0100, Ard Biesheuvel wrote:
> On Thu, 18 Jan 2024 at 21:15, Kees Cook <[e-mail]> wrote:
> >
> > On Wed, Jan 17, 2024 at 04:07:34PM +0100, Ard Biesheuvel wrote:
> > > From: Ard Biesheuvel <[e-mail]>
> > >
> > > is_valid_bugaddr() dereferences the faulting PC to fetch the instruction
> > > that triggered the fault, to decide whether it is a BRK instruction used
> > > to force an exception. This is used by the BUG() infrastructure to keep
> > > the handling logic (which should never execute) separate from the code
> > > that normally runs.
> > >
> > > This dereference may attempt to access user memory if the faulting PC
> > > happens to contain a user address. One way this might happen is when
> > > the kernel is tricked into executing from user space while PAN
> > > protections (Privileged Access Never) are in effect: the instruction
> > > fetch will trigger a prefetch abort, the handling of which involves a
> > > check whether the instruction that caused it is a BRK, requiring a
> > > load from the same address. This load is privileged too, and so it will
> > > trigger another exception, which we fail to recover from.
> > >
> > > Given that BRK instructions tied to BUG() handling can only appear in
> > > kernel code, let's check first that the PC actually points into kernel
> > > memory.
> > >
> > > Cc: Kees Cook <[e-mail]>
> > > Cc: Russell King <[e-mail]>
> > > Cc: Mark Brown <[e-mail]>
> > > Cc: Zhen Lei <[e-mail]>
> > > Cc: Linus Walleij <[e-mail]>
> > > Link: https://lkml.kernel.org/r/202401111544.18EBB6AA%40keescook
> > > Signed-off-by: Ard Biesheuvel <[e-mail]>
> > > ---
> > >  arch/arm/kernel/traps.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> > > index 3bad79db5d6e..f342bd6b2a5d 100644
> > > --- a/arch/arm/kernel/traps.c
> > > +++ b/arch/arm/kernel/traps.c
> > > @@ -402,6 +402,9 @@ int is_valid_bugaddr(unsigned long pc)
> > >       u32 insn = __opcode_to_mem_arm(BUG_INSTR_VALUE);
> > >  #endif
> > >
> > > +     if (pc < TASK_SIZE)
> > > +             return 0;
> > > +
> > >       if (get_kernel_nofault(bkpt, (void *)pc))
> > >               return 0;
> > >
> >
> > Okay, I've finally had a chance to test this and it doesn't fix it
> > for me. I still see the double domain fault, and I still lose a CPU.
> > I patched in a pr_info() to show that the "pc < TASK_SIZE" was hitting
> > (which it is) so I see:
> >
> >
> > kdtm: Performing direct entry EXEC_USERSPACE
> > lkdtm: attempting ok execution at 80761b4c
> > lkdtm: attempting bad execution at 76ff4000
> > Unhandled prefetch abort: page domain fault (0x01b) at 0x76ff4000
> > Skipping is_valid_bugaddr(76ff4000)
> > Internal error: : 1b [#12] SMP ARM
> > Modules linked in:
> > ...
> > Process cat (pid: 1301, stack limit = 0x3c8ec418)
> > Stack: (0xf0975e58 to 0xf0976000)
> > ...
> > Backtrace:
> >  lkdtm_EXEC_USERSPACE from lkdtm_do_action+0x2c/0x4c
> > ...
> > Exception stack(0xf0975fa8 to 0xf0975ff0)
> > ...
> > 8<--- cut here ---
> > Unhandled fault: page domain fault (0x01b) at 0x76ff3ff0
> > [76ff3ff0] *pgd=44baa835, *pte=00000000, *ppte=00000000
> > Internal error: : 1b [#13] SMP ARM
> > Modules linked in:
> > ...
> > Process cat (pid: 1301, stack limit = 0x3c8ec418)
> > Stack: (0xf0975ce0 to 0xf0976000)
> > ...
> > Backtrace:
> >  copy_from_kernel_nofault from dump_instr+0x1cc/0x208
> >  r7:f0975d27 r6:80ddf3d8 r5:f0975e08 r4:fffffffc
> >  dump_instr from die+0x294/0x2f0
> 
> This is indeed a similar but different issue (as you conclude below).
> dump_instr() attempts to dereference PC to generate the 'Code: xxxx
> xxxx' line with the opcodes leading up to the fault, and this triggers
> a PAN fault for the same reason.

Both of these have faulted in copy_from_kernel_nofault(), so rather than
adding these tests all over the place, wouldn't it be better to
implement copy_from_kernel_nofault_allowed() so that we verify that the
source is actually something we can copy from?

E.g.

bool copy_from_kernel_nofault_allowed(const void *src, size_t size)
{
	unsigned long srcv = (unsigned long)src;

	return src >= TASK_SIZE && src + size - 1 >= TASK_SIZE;
}

Ard Biesheuvel Jan. 19, 2024, 12:24 p.m. UTC | #7

On Fri, 19 Jan 2024 at 13:14, Russell King (Oracle)
<[e-mail]> wrote:
>
> On Fri, Jan 19, 2024 at 12:52:21PM +0100, Ard Biesheuvel wrote:
> > On Thu, 18 Jan 2024 at 21:15, Kees Cook <[e-mail]> wrote:
> > >
> > > On Wed, Jan 17, 2024 at 04:07:34PM +0100, Ard Biesheuvel wrote:
> > > > From: Ard Biesheuvel <[e-mail]>
> > > >
> > > > is_valid_bugaddr() dereferences the faulting PC to fetch the instruction
> > > > that triggered the fault, to decide whether it is a BRK instruction used
> > > > to force an exception. This is used by the BUG() infrastructure to keep
> > > > the handling logic (which should never execute) separate from the code
> > > > that normally runs.
> > > >
> > > > This dereference may attempt to access user memory if the faulting PC
> > > > happens to contain a user address. One way this might happen is when
> > > > the kernel is tricked into executing from user space while PAN
> > > > protections (Privileged Access Never) are in effect: the instruction
> > > > fetch will trigger a prefetch abort, the handling of which involves a
> > > > check whether the instruction that caused it is a BRK, requiring a
> > > > load from the same address. This load is privileged too, and so it will
> > > > trigger another exception, which we fail to recover from.
> > > >
> > > > Given that BRK instructions tied to BUG() handling can only appear in
> > > > kernel code, let's check first that the PC actually points into kernel
> > > > memory.
> > > >
> > > > Cc: Kees Cook <[e-mail]>
> > > > Cc: Russell King <[e-mail]>
> > > > Cc: Mark Brown <[e-mail]>
> > > > Cc: Zhen Lei <[e-mail]>
> > > > Cc: Linus Walleij <[e-mail]>
> > > > Link: https://lkml.kernel.org/r/202401111544.18EBB6AA%40keescook
> > > > Signed-off-by: Ard Biesheuvel <[e-mail]>
> > > > ---
> > > >  arch/arm/kernel/traps.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> > > > index 3bad79db5d6e..f342bd6b2a5d 100644
> > > > --- a/arch/arm/kernel/traps.c
> > > > +++ b/arch/arm/kernel/traps.c
> > > > @@ -402,6 +402,9 @@ int is_valid_bugaddr(unsigned long pc)
> > > >       u32 insn = __opcode_to_mem_arm(BUG_INSTR_VALUE);
> > > >  #endif
> > > >
> > > > +     if (pc < TASK_SIZE)
> > > > +             return 0;
> > > > +
> > > >       if (get_kernel_nofault(bkpt, (void *)pc))
> > > >               return 0;
> > > >
> > >
> > > Okay, I've finally had a chance to test this and it doesn't fix it
> > > for me. I still see the double domain fault, and I still lose a CPU.
> > > I patched in a pr_info() to show that the "pc < TASK_SIZE" was hitting
> > > (which it is) so I see:
> > >
> > >
> > > kdtm: Performing direct entry EXEC_USERSPACE
> > > lkdtm: attempting ok execution at 80761b4c
> > > lkdtm: attempting bad execution at 76ff4000
> > > Unhandled prefetch abort: page domain fault (0x01b) at 0x76ff4000
> > > Skipping is_valid_bugaddr(76ff4000)
> > > Internal error: : 1b [#12] SMP ARM
> > > Modules linked in:
> > > ...
> > > Process cat (pid: 1301, stack limit = 0x3c8ec418)
> > > Stack: (0xf0975e58 to 0xf0976000)
> > > ...
> > > Backtrace:
> > >  lkdtm_EXEC_USERSPACE from lkdtm_do_action+0x2c/0x4c
> > > ...
> > > Exception stack(0xf0975fa8 to 0xf0975ff0)
> > > ...
> > > 8<--- cut here ---
> > > Unhandled fault: page domain fault (0x01b) at 0x76ff3ff0
> > > [76ff3ff0] *pgd=44baa835, *pte=00000000, *ppte=00000000
> > > Internal error: : 1b [#13] SMP ARM
> > > Modules linked in:
> > > ...
> > > Process cat (pid: 1301, stack limit = 0x3c8ec418)
> > > Stack: (0xf0975ce0 to 0xf0976000)
> > > ...
> > > Backtrace:
> > >  copy_from_kernel_nofault from dump_instr+0x1cc/0x208
> > >  r7:f0975d27 r6:80ddf3d8 r5:f0975e08 r4:fffffffc
> > >  dump_instr from die+0x294/0x2f0
> >
> > This is indeed a similar but different issue (as you conclude below).
> > dump_instr() attempts to dereference PC to generate the 'Code: xxxx
> > xxxx' line with the opcodes leading up to the fault, and this triggers
> > a PAN fault for the same reason.
>
> Both of these have faulted in copy_from_kernel_nofault(), so rather than
> adding these tests all over the place, wouldn't it be better to
> implement copy_from_kernel_nofault_allowed() so that we verify that the
> source is actually something we can copy from?
>
> E.g.
>
> bool copy_from_kernel_nofault_allowed(const void *src, size_t size)
> {
>         unsigned long srcv = (unsigned long)src;
>
>         return src >= TASK_SIZE && src + size - 1 >= TASK_SIZE;
> }
>

Indeed. I had no idea this existed, but obviously,
copy_from_kernel_nofault() should never access user memory to begin
with.

Patch

diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
index 3bad79db5d6e..f342bd6b2a5d 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -402,6 +402,9 @@  int is_valid_bugaddr(unsigned long pc)
 	u32 insn = __opcode_to_mem_arm(BUG_INSTR_VALUE);
 #endif
 
+	if (pc < TASK_SIZE)
+		return 0;
+
 	if (get_kernel_nofault(bkpt, (void *)pc))
 		return 0;