linux-kernel - Re: [PATCH v4 06/10] x86/alternative: use temporary mm for text poking

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]

Date:   Mon, 12 Nov 2018 00:52:20 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Nadav Amit <namit@...are.com>
Cc:     Ingo Molnar <mingo@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>, X86 ML <x86@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Andy Lutomirski <luto@...nel.org>,
        Kees Cook <keescook@...omium.org>,
        Dave Hansen <dave.hansen@...el.com>,
        Masami Hiramatsu <mhiramat@...nel.org>
Subject: Re: [PATCH v4 06/10] x86/alternative: use temporary mm for text
 poking

On Sun, Nov 11, 2018 at 08:53:07PM +0000, Nadav Amit wrote:

> >> +	/*
> >> +	 * The lock is not really needed, but this allows to avoid open-coding.
> >> +	 */
> >> +	ptep = get_locked_pte(poking_mm, poking_addr, &ptl);
> >> +
> >> +	/*
> >> +	 * If we failed to allocate a PTE, fail. This should *never* happen,
> >> +	 * since we preallocate the PTE.
> >> +	 */
> >> +	if (WARN_ON_ONCE(!ptep))
> >> +		goto out;
> > 
> > Since we hard rely on init getting that right; can't we simply get rid
> > of this?
> 
> This is a repeated complaint of yours, which I do not feel comfortable with.
> One day someone will run some static analysis tool and start finding that
> all these checks are missing.
> 
> The question is why do you care about them.

Mostly because they should not be happening, ever. And if they happen,
there really isn't anything sensible we can do about it.

> If it is because they affect the
> generated code and make it less efficient, I can fully understand and perhaps
> we should have something like PARANOID_WARN_ON_ONCE() which compiles into nothing
> unless a certain debug option is set.
> 
> If it is about the way the source code looks - I guess it doesn’t sore my
> eyes as hard as some other stuff, and I cannot do much about it (other than
> removing it as you asked).

And yes on the above two points. It adds both runtime overhead (albeit
trivially small) and code complexity.

> >> +out:
> >> +	if (memcmp(addr, opcode, len))
> >> +		r = -EFAULT;
> > 
> > How could this ever fail? And how can we reliably recover from that?
> 
> This code has been there before (with slightly uglier code). Before this
> patch, a BUG_ON() was used here. However, I noticed that kgdb actually
> checks that text_poke() succeeded after calling it and gracefully fail.
> However, this was useless, since text_poke() would panic before kgdb gets
> the chance to do anything (see patch 7).

Yes, I know it was there before, and I did see kgdb do it too. But aside
from that out-label case, which we also should never hit, how can we
realistically ever fail that memcmp()?

If we fail here, something is _seriously_ buggered.