linux-hardening - Re: [RFC PATCH 01/11] x86: kernel FineIBT

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: <20220429013704.4n4lmadpstdioe7a@treble>
Date:   Thu, 28 Apr 2022 18:37:04 -0700
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     joao@...rdrivepizza.com
Cc:     linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org,
        peterz@...radead.org, andrew.cooper3@...rix.com,
        keescook@...omium.org, samitolvanen@...gle.com,
        mark.rutland@....com, hjl.tools@...il.com,
        alyssa.milburn@...ux.intel.com, ndesaulniers@...gle.com,
        gabriel.gomes@...ux.intel.com, rick.p.edgecombe@...el.com
Subject: Re: [RFC PATCH 01/11] x86: kernel FineIBT

On Tue, Apr 19, 2022 at 05:42:31PM -0700, joao@...rdrivepizza.com wrote:
> +void __noendbr __fineibt_handler(void){
> +	unsigned i;
> +	unsigned long flags;
> +	bool skip;
> +	void * ret;
> +	void * caller;
> +
> +	DO_ALL_PUSHS;

So this function isn't C ABI compliant, right? e.g. the compiler just
calls the handler without regard for preserving registers?

If this function is going to be implemented in C, it should probably
have an asm thunk wrapper which can properly save/restore the registers
before calling into the C version.

Even better, if the compiler did an invalid op (UD2?), which I think you
mentioned elsewhere, instead of calling the handler directly, and there
were a way for the trap code to properly detect it as a FineIBT
violation, we could get rid of the pushes/pops, plus the uaccess objtool
warning from patch 7, plus I'm guessing a bunch of noinstr validation
warnings.

> +
> +	spin_lock_irqsave(&fineibt_lock, flags);
> +	skip = false;
> +
> +	asm("\t movq 0x90(%%rsp),%0" : "=r"(ret));
> +	asm("\t movq 0x98(%%rsp),%0" : "=r"(caller));

This is making some questionable assumptions about the stack layout.

I assume this function is still in the prototype stage ;-)

> +	if(!skip) {
> +		printk("FineIBT violation: %px:%px:%u\n", ret, caller,
> +				vlts_next);
> +	}
> +	DO_ALL_POPS;
> +}

Right now this handler just does a printk if it hasn't already for this
caller/callee combo, and then resumes control.  Which is fine for
debugging, but it really needs to behave similarly to an IBT violation,
by panicking unless "ibt=warn" on the cmdline.

Not sure what would happen for "ibt=off"?  Maybe apply_ibt_endbr() could
NOP out all the FineIBT stuff.

-- 
Josh