Re: [PATCH v2] x86/dumpstack: Fix misleading instruction pointer error message

[Borislav Petkov]

On Mon, Nov 16, 2020 at 11:01:03PM +0100, Thomas Gleixner wrote:
> Subject: x86/dumpstack: Don't try to access user space code of other tasks
> From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Date: Mon, 16 Nov 2020 22:26:52 +0100
> 
> sysrq-t ends up invoking show_opcodes() for each task which tries to access
> the user space code of other processes which is obviously bogus.
> 
> It either manages to dump where the foreign tasks regs->ip points to in

I guess you mean here "points to valid mapping of current" or so.

> currents mapping or triggers a pagefault and prints "Code: Bad RIP
> value.". Both is just wrong.
> 
> Add a safeguard in copy_code() and check whether the @regs pointer matches
> currents pt_regs. If not, do not even try to access it.
> 
> While at it, add commentry why using copy_from_user_nmi() is safe in

s/commentry/commentary/

> copy_code() even if the function name suggests otherwise.
> 
> Reported-by: Mark Mossberg <mark.mossberg@xxxxxxxxx>

This is Reported-by: Oleg

> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> ---
>  arch/x86/kernel/dumpstack.c |   23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> --- a/arch/x86/kernel/dumpstack.c
> +++ b/arch/x86/kernel/dumpstack.c
> @@ -78,6 +78,9 @@ static int copy_code(struct pt_regs *reg
>  	if (!user_mode(regs))
>  		return copy_from_kernel_nofault(buf, (u8 *)src, nbytes);
>  
> +	/* The user space code from other tasks cannot be accessed. */
> +	if (regs != task_pt_regs(current))
> +		return -EPERM;
>  	/*
>  	 * Make sure userspace isn't trying to trick us into dumping kernel
>  	 * memory by pointing the userspace instruction pointer at it.
> @@ -85,6 +88,12 @@ static int copy_code(struct pt_regs *reg
>  	if (__chk_range_not_ok(src, nbytes, TASK_SIZE_MAX))
>  		return -EINVAL;
>  
> +	/*
> +	 * Even if named copy_from_user_nmi() this can be invoked from
> +	 * other contexts and will not try to resolve a pagefault, which is
> +	 * the correct thing to do here as this code can be called from any
> +	 * context.
> +	 */

Can we stick the first part of this comment about "this can be invoked
from other contexts" over the function definition?

>  	return copy_from_user_nmi(buf, (void __user *)src, nbytes);
>  }

...

With this, I see Code: only once with Sysrq-T:

[   25.491878] task:bash            state:R  running task     stack:    0 pid: 4267 ppid:  4187 flags:0x00004000

...

[   25.497740] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b3 0f 1f 80 00 00 00 00 48 8d 05 f9 61 0d 00 8b 00 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 c3 0f 1f 00 41 54 49 89 d4 55 48 89 f5 53

which is the shell doing the

$ echo t > /proc/sysrq-trigger

So

Reviewed-by: Borislav Petkov <bp@xxxxxxx>
Tested-by: Borislav Petkov <bp@xxxxxxx>

Thanks!

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette