linux-kernel - Re: [Patch v3 13/13] x86/speculation: Create PRCTL interface to restrict indirect branch speculation

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: <alpine.DEB.2.21.1810181717410.3566@nanos.tec.linutronix.de>
Date:   Thu, 18 Oct 2018 17:31:02 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Tim Chen <tim.c.chen@...ux.intel.com>
cc:     Jiri Kosina <jikos@...nel.org>,
        Tom Lendacky <thomas.lendacky@....com>,
        Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        David Woodhouse <dwmw@...zon.co.uk>,
        Andi Kleen <ak@...ux.intel.com>,
        Dave Hansen <dave.hansen@...el.com>,
        Casey Schaufler <casey.schaufler@...el.com>,
        Asit Mallick <asit.k.mallick@...el.com>,
        Arjan van de Ven <arjan@...ux.intel.com>,
        Jon Masters <jcm@...hat.com>, linux-kernel@...r.kernel.org,
        x86@...nel.org
Subject: Re: [Patch v3 13/13] x86/speculation: Create PRCTL interface to
 restrict indirect branch speculation

On Wed, 17 Oct 2018, Tim Chen wrote:

> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4196,7 +4196,10 @@
>  			lite   - turn on mitigation for non-dumpable
>  				 processes (i.e. protect daemons and other
>  				 privileged processes that tend to be
> -				 non-dumpable).
> +				 non-dumpable), and processes that has indirect
> +				 branch speculation restricted via prctl's
> +				 PR_SET_SPECULATION_CTRL option

				Protect processes which are marked non-dumpable and
				processes which have requested restricted indirect
				branch speculation via the PR_SET_SPECULATION_CTRL
				ptrcl().

> @@ -92,3 +92,13 @@ Speculation misfeature controls
>     * prctl(PR_SET_SPECULATION_CTRL, PR_SPEC_STORE_BYPASS, PR_SPEC_ENABLE, 0, 0);
>     * prctl(PR_SET_SPECULATION_CTRL, PR_SPEC_STORE_BYPASS, PR_SPEC_DISABLE, 0, 0);
>     * prctl(PR_SET_SPECULATION_CTRL, PR_SPEC_STORE_BYPASS, PR_SPEC_FORCE_DISABLE, 0, 0);
> +
> +- PR_INDIR_BRANCH: Indirect Branch Speculation in Applications
> +                   (Mitigate Spectre V2 style user space application
> +                    to application attack)

No. Please do not create a random name space. We have

    PR_SPEC_STORE_BYPASS

so the logical name for this is

    PR_SPEC_INDIR_BRANCH


> +static int indir_branch_prctl_set(struct task_struct *task, unsigned long ctrl)
> +{
> +	bool update;
> +
> +	switch (ctrl) {
> +	case PR_SPEC_ENABLE:
> +		if (spectre_v2_app2app_enabled == SPECTRE_V2_APP2APP_NONE)
> +			return 0;
> +		/*
> +		 * Indirect branch speculation is always disabled in
> +		 * strict mode or if the application is non dumpable
> +		 * in lite mode. 
> +		 */
> +		if (spectre_v2_app2app_enabled == SPECTRE_V2_APP2APP_STRICT)
> +			return -ENXIO;

Please stay consistent with ssb_prctl_set(). EPERM is what you want here.

> +		if (task->mm && get_dumpable(task->mm) != SUID_DUMP_USER)
> +			return -ENXIO;

Ditto

> +		task_clear_spec_indir_branch_disable(task);
> +		update = test_and_clear_tsk_thread_flag(task, TIF_STIBP);
> +		break;
> +	case PR_SPEC_DISABLE:
> +		/*
> +		 * Indirect branch speculation is always enabled when
> +		 * app to app mitigation is off.
> +		 */
> +		if (spectre_v2_app2app_enabled == SPECTRE_V2_APP2APP_NONE)
> +			return -ENXIO;
> +		if (spectre_v2_app2app_enabled == SPECTRE_V2_APP2APP_STRICT)
> +			return 0;
> +		task_set_spec_indir_branch_disable(task);
> +		update = !test_and_set_tsk_thread_flag(task, TIF_STIBP);
> +		break;
> +	case PR_SPEC_FORCE_DISABLE:
> +		if (spectre_v2_app2app_enabled == SPECTRE_V2_APP2APP_NONE)
> +			return -ENXIO;
> +		if (spectre_v2_app2app_enabled == SPECTRE_V2_APP2APP_STRICT)
> +			return 0;
> +		task_set_spec_indir_branch_disable(task);
> +		task_set_spec_indir_branch_force_disable(task);
> +		update = !test_and_set_tsk_thread_flag(task, TIF_STIBP);
> +		break;
> +	default:
> +		return -ERANGE;
> +	}
> +
> +	/*
> +	 * If being set on non-current task, delay setting the CPU
> +	 * mitigation until it is next scheduled.
> +	 * Use speculative_store_bypass_update will update SPEC_CTRL MSR

Stale comment.

> +	 */
> +	if (task == current && update)
> +		speculation_ctrl_update_current();
> +
> +	return 0;
> +}

Aside of that several patches have trailing whitespace. Please be more careful.

Thanks,

	tglx