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

From: Borislav Petkov <bp@alien8.de>
To: Oleg Nesterov <oleg@redhat.com>
Cc: Mark Mossberg <mark.mossberg@gmail.com>,
	tglx@linutronix.de, mingo@redhat.com, x86@kernel.org,
	linux-kernel@vger.kernel.org, hpa@zytor.com, jannh@google.com,
	kyin@redhat.com
Subject: Re: [PATCH v2] x86/dumpstack: Fix misleading instruction pointer error message
Date: Tue, 3 Nov 2020 18:15:37 +0100	[thread overview]
Message-ID: <20201103171537.GC4111@zn.tnic> (raw)
In-Reply-To: <20201103125034.GA30391@redhat.com>

On Tue, Nov 03, 2020 at 01:50:34PM +0100, Oleg Nesterov wrote:
> Another problem is that show_opcodes() makes no sense if user_mode(regs)
> and tsk is not current.

Because if not current, we would access *some* user address space but
not the one to which regs belong to?

> Try "echo t > /proc/sysrq-trigger".

What am I looking for?

I see a bunch of:

[   37.622896] Code: Unable to access opcode bytes at RIP <user address>

and three Code: lines with opcode bytes, as expected:

[   37.148693] Code: 11 0d 00 48 89 c6 4c 89 ef e8 98 07 00 00 48 83 f8 ff 0f 84 3e 02 00 00 48 3b 05 b7 28 0d 00 48 89 c3 0f 83 b5 00 00 00 48 8b <0d> e7 10 0d 00 48 83 f8 0d 76 13 48 b8 28 75 6e 72 65 61 63 68 48

So all those other but the three cases, copy_code() failed.

> In this case copy_from_user_nmi() will either fail, or (worse) it will
> read the "random" memory from current->mm.
> 
> Perhaps we can add something like
> 
> 	if (user_mode(regs) && regs != task_pt_regs(current))
> 		return;
> 
> at the start of show_opcodes() ?

tglx made it use copy_from_user_nmi() in:

d181d2da0141 ("x86/dumpstack: Dump user space code correctly again")

I'm thinking this should not use the atomic variant if it can get called
in !atomic context too.

Thomas?

Leaving in the rest for reference.

> > --- a/arch/x86/kernel/dumpstack.c
> > +++ b/arch/x86/kernel/dumpstack.c
> > @@ -115,7 +115,8 @@ void show_opcodes(struct pt_regs *regs, const char *loglvl)
> >  	unsigned long prologue = regs->ip - PROLOGUE_SIZE;
> >  
> >  	if (copy_code(regs, opcodes, prologue, sizeof(opcodes))) {
> > -		printk("%sCode: Bad RIP value.\n", loglvl);
> > +		printk("%sCode: Unable to access opcode bytes at RIP 0x%lx.\n",
> > +		       loglvl, prologue);
> >  	} else {
> >  		printk("%sCode: %" __stringify(PROLOGUE_SIZE) "ph <%02x> %"
> >  		       __stringify(EPILOGUE_SIZE) "ph\n", loglvl, opcodes,
> > -- 
> > 2.25.1

-- 
Regards/Gruss,
    Boris.

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