Re: [PATCH v2] retpoline/module: Warn for missing retpoline in module

[Thomas Gleixner]

On Fri, 12 Jan 2018, Andi Kleen wrote:
> From: Andi Kleen <ak@xxxxxxxxxxxxxxx>
>  void stop_this_cpu(void *dummy);
>  void df_debug(struct pt_regs *regs, long error_code);
> +
> +void disable_retpoline(void);
> +bool retpoline_enabled(void);

Can you please use a consistent name space? retpoline_ ... or such?

> +/* A module has been loaded. Disable reporting that we're good. */
> +void disable_retpoline(void)
> +{
> +	spectre_v2_enabled = SPECTRE_V2_NONE;

I really don't like fiddling with that variable. That's just hackery. The
variable reflects the actual enabled mitigation state of the kernel proper.

> +	pr_err("system may be vunerable to spectre\n");
> +}
> +
> +bool retpoline_enabled(void)
> +{
> +	return spectre_v2_enabled != SPECTRE_V2_NONE;
> +}

That'll break once we get other mitigation variants.

> @@ -3020,7 +3020,13 @@ static int check_modinfo(struct module *mod, struct load_info *info, int flags)
>  				mod->name);
>  		add_taint_module(mod, TAINT_OOT_MODULE, LOCKDEP_STILL_OK);
>  	}
> -

These newlines are there to separate stuff for readability sake.

> +#ifdef RETPOLINE
> +	if (retpoline_enabled() && !get_modinfo(info, "retpoline")) {
> +		pr_warn("%s: loading module not compiled with retpoline compiler.\n",
> +				mod->name);
> +		disable_retpoline();
> +	}
> +#endif

This really can be done in a cleaner way.

in linux/module.h

#ifdef RETPOLINE
extern bool retpoline_module_ok(bool has_retpoline);
#else
static inline bool retpoline_module_ok(bool has_retpoline)
{
	return true;
}
#endif

static void check_modinfo_retpoline(mod, info)
{
	if (retpoline_module_ok(get_modinfo(info, "retpoline")))
		return;
		
	pr_warn("%s: loading module not compiled with retpoline compiler.\n",
		mod->name);
}

That only needs one function and that one can take care of setting a
variable in the spectre code which then influences the sysfs output.

And that output should not be "Vulnerable" like you force with the hack
above. It actually should tell WHY it is vulnerable despite having had
protection in place before the module was loaded.

Thanks,

	tglx