linux-kernel - Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC

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:   Sat, 6 Jan 2018 10:10:59 -0800
From:   Tim Chen <tim.c.chen@...ux.intel.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Andy Lutomirski <luto@...nel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Dave Hansen <dave.hansen@...el.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Arjan Van De Ven <arjan.van.de.ven@...el.com>,
        David Woodhouse <dwmw@...zon.co.uk>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable
 SPEC_CTRL feature



On 01/06/2018 12:54 AM, Greg KH wrote:
> On Fri, Jan 05, 2018 at 06:12:19PM -0800, Tim Chen wrote:
>> From: Tim Chen <tim.c.chen@...ux.intel.com>
>> From: Andrea Arcangeli <aarcange@...hat.com>
>>
>> There are 2 ways to control IBRS
>>
>> 1. At boot time
>>     noibrs kernel boot parameter will disable IBRS usage
>>
>> Otherwise if the above parameters are not specified, the system
>> will enable ibrs and ibpb usage if the cpu supports it.
>>
>> 2. At run time
>>     echo 0 > /sys/kernel/debug/x86/ibrs_enabled will turn off IBRS
>>     echo 1 > /sys/kernel/debug/x86/ibrs_enabled will turn on IBRS in kernel
>>     echo 2 > /sys/kernel/debug/x86/ibrs_enabled will turn on IBRS in both userspace and kernel
>>
>> The implementation was updated with input and suggestions from Andrea Arcangeli.
>>
>> Signed-off-by: Tim Chen <tim.c.chen@...ux.intel.com>
>> ---
> 
> <snip>
> 
>> index 0000000..4fda38b
>> --- /dev/null
>> +++ b/arch/x86/include/asm/spec_ctrl.h
>> @@ -0,0 +1,15 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
> 
> Thank you for these markings.
> 
>> --- /dev/null
>> +++ b/arch/x86/kernel/cpu/spec_ctrl.c
>> @@ -0,0 +1,160 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +
>> +#include <linux/mutex.h>
>> +#include <linux/debugfs.h>
>> +#include <linux/uaccess.h>
>> +
>> +#include <asm/spec_ctrl.h>
>> +#include <asm/cpufeature.h>
>> +
>> +unsigned int dynamic_ibrs __read_mostly;
>> +EXPORT_SYMBOL_GPL(dynamic_ibrs);
> 
> What kernel module needs this symbol?  A symbol can be "global" and not
> be exported, those are two different things.
> 
> And again, horrible name for a global symbol, if this really is being
> exported to modules (i.e. the world.)
> 
>> +
>> +enum {
>> +	IBRS_DISABLED,
> 
> As you are making a user/kernel API here, shouldn't you enforce the
> values this enum has "just to be sure"?
> 
>> +	/* in host kernel, disabled in guest and userland */
>> +	IBRS_ENABLED,
>> +	/* in host kernel and host userland, disabled in guest */
>> +	IBRS_ENABLED_USER,
>> +	IBRS_MAX = IBRS_ENABLED_USER,
>> +};
> 
> Also, this should be documented somewhere, Documentation/ABI/ perhaps?
> 
>> +static unsigned int ibrs_enabled;
>> +static bool ibrs_admin_disabled;
>> +
>> +/* mutex to serialize IBRS control changes */
>> +DEFINE_MUTEX(spec_ctrl_mutex);
> 
> static?
> 
>> +
>> +void scan_spec_ctrl_feature(struct cpuinfo_x86 *c)
>> +{
>> +	if ((!c->cpu_index) && (boot_cpu_has(X86_FEATURE_SPEC_CTRL))) {
>> +		if (!ibrs_admin_disabled) {
>> +			dynamic_ibrs = 1;
>> +			ibrs_enabled = IBRS_ENABLED;
>> +		}
>> +	}
>> +}
>> +EXPORT_SYMBOL_GPL(scan_spec_ctrl_feature);
> 
> Again, what module needs this?

Right now no module will need it.  I'll get rid of it.

> 
> Same for all the exports in this file, and again, if they are needed in
> modules, you need to get a better naming scheme.  I'm not trying to
> bikeshed, it really matters, our global symbol namespace is a mess,
> don't make it worse :)

Noted.  I'll add spec_ctrl in front of those.

> 
> thanks,
> 
> greg k-h
>