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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201103171537.GC4111@zn.tnic>
Date:   Tue, 3 Nov 2020 18:15:37 +0100
From:   Borislav Petkov <bp@...en8.de>
To:     Oleg Nesterov <oleg@...hat.com>
Cc:     Mark Mossberg <mark.mossberg@...il.com>, tglx@...utronix.de,
        mingo@...hat.com, x86@...nel.org, linux-kernel@...r.kernel.org,
        hpa@...or.com, jannh@...gle.com, kyin@...hat.com
Subject: Re: [PATCH v2] x86/dumpstack: Fix misleading instruction pointer
 error message

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