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

[Thomas Gleixner <tglx@linutronix.de>]

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