Re: [RFC PATCH] x86/debug: Dump more detailed segfault info

[Ingo Molnar]

* Borislav Petkov <bp@xxxxxxxxx> wrote:

> From: Borislav Petkov <bp@xxxxxxx>
> 
> I found out recently that this is very helpful when trying to look at
> opcodes around the rIP when the segfault happens, and also poke at
> architectural registers. When enabled, it looks something like this:
> 
>   strsep[3702]: segfault at 40066b ip 00007ffff7abe22b sp 00007fffffffea70 error 7 in libc-2.19.so[7ffff7a33000+19f000]
>   RIP: 0033:[<00007ffff7abe22b>]  [<00007ffff7abe22b>] 0x7ffff7abe22b
>   RSP: 002b:00007fffffffea70  EFLAGS: 00010202
>   RAX: 000000000040066b RBX: 0000000000400664 RCX: 0000000000000000
>   RDX: 0000000000000000 RSI: 000000000000003d RDI: 0000000000400665
>   RBP: 00007fffffffea90 R08: 00007ffff7dd7c60 R09: 00007ffff7deae20
>   R10: 00007fffffffe850 R11: 00007ffff7abe200 R12: 0000000000400460
>   R13: 00007fffffffeb80 R14: 0000000000000000 R15: 0000000000000000
>   FS:  00007ffff7fdc700(0000) GS:ffff88007ed40000(0000) knlGS:0000000000000000
>   CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>   CR2: 000000000040066b CR3: 0000000079a39000 CR4: 00000000000406e0
>   Code: 74 33 80 7e 01 00 74 22 48 89 df e8 5a 8a ff ff 48 85 c0 74 20 <c6> 00 00 48 83 c0
>   01 48 89 45 00 48 89 d8 48 83 c4 08 5b 5d c3 0f b6 13 38 d0 74 29 84 d2 75 15 48 c7 45 00 00 00 00 00 48 83 c4

Note that on recent kernels, with printk log timestamping enabled, this looks 
like:

[  206.721243] CR2: 0000000000000000 CR3: 000000042ab75000 CR4: 00000000001406e0
[  206.729217] Code: 
[  206.731271] 55 
[  206.733046] 48 
[  206.733348] 89 
[  206.733665] e5 
[  206.733982] ff 
[  206.734292] d0 5d e9 
[  206.735209] 7a 
[  206.735519] ff ff 
[  206.736133] ff 
[  206.736444] 55 48 
[  206.737057] 89 
[  206.737367] e5 b8 
[  206.737992] 00 
[  206.738303] 00 00 00 
[  206.739216] <c6> 
[  206.739728] 00 
[  206.740031] 00 


> I know, this info can be collected with core dumps but in constrained
> environments or when writing out core dumps is impossible (too early in
> the boot, fs is fscked), getting some more detailed info might be really
> helpful.
> 
> Oh, and the size of the change is small enough.
> 
> Signed-off-by: Borislav Petkov <bp@xxxxxxx>
> Cc: Andy Lutomirski <luto@xxxxxxxxxx>
> Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> ---
>  arch/x86/Kconfig.debug |  9 +++++++++
>  arch/x86/mm/fault.c    | 24 +++++++++++++++++++++++-
>  2 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
> index 67eec55093a5..514bbae2f4c6 100644
> --- a/arch/x86/Kconfig.debug
> +++ b/arch/x86/Kconfig.debug
> @@ -361,4 +361,13 @@ config PUNIT_ATOM_DEBUG
>  	  The current power state can be read from
>  	  /sys/kernel/debug/punit_atom/dev_power_state
>  
> +config SEGFAULT_DETAILED_DEBUG
> +	bool "Dump detailed information for fatal segfaults"
> +	---help---
> +	  Enable this option if you want to see more debug info in
> +	  the kernel log when fatal segfaults get reported. This
> +	  option might be useful in constrained environments when
> +	  core dumps might not be possible and/or filesystems are
> +	  not ready for a core dump writeout.
> +
>  endmenu
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index 9f72ca3b2669..120f126f5b54 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -847,8 +847,30 @@ show_signal_msg(struct pt_regs *regs, unsigned long error_code,
>  		(void *)regs->ip, (void *)regs->sp, error_code);
>  
>  	print_vma_addr(KERN_CONT " in ", regs->ip);
> -
>  	printk(KERN_CONT "\n");
> +
> +	if (IS_ENABLED(CONFIG_SEGFAULT_DETAILED_DEBUG)) {
> +#define PREAMBLE_LEN	21
> +#define OPC_BUF_LEN	64
> +		u8 code[OPC_BUF_LEN];
> +		int len, i;
> +
> +		len = __copy_from_user_inatomic(code,
> +						(void *)regs->ip - PREAMBLE_LEN,
> +						OPC_BUF_LEN);
> +
> +		__show_regs(regs, 1);
> +
> +		printk(KERN_DEFAULT "Code: ");
> +		for (i = 0; i < OPC_BUF_LEN - len; i++) {
> +			if (i == PREAMBLE_LEN)
> +				pr_cont("<%02x> ", code[i]);
> +			else
> +				pr_cont("%02x ", code[i]);
> +		}
> +
> +		printk(KERN_CONT "\n");
> +	}

The main problem I have with this is that it's a big information leak: if an admin 
leaves this enabled then any user can use segfaults to read any kernel address.

Something like this will set RIP to arbitrary value:

void main(void)
{
        void (*fn)(void) = (void *)0xffff8800000fcda0;

        fn();
}

and the segfault info dumper then prints the user-inaccessible contents of the RIP 
address.

So I don't mind the feature, but this should only dump code that is user-readable.

Thanks,

	Ingo