[PATCH] x86/intel: Clear SGX bit if both SGX driver and KVM SGX are not enabled

[PATCH] x86/intel: Clear SGX bit if both SGX driver and KVM SGX are not enabled

[Kai Huang]

Currently on platform which has SGX enabled, if CONFIG_X86_SGX is not
enabled, the X86_FEATURE_SGX is not cleared, resulting in /proc/cpuinfo
shows "sgx" feature.  This is not desired.

Clear SGX feature bit if both SGX driver and KVM SGX are not enabled in
init_ia32_feat_ctl().

Signed-off-by: Kai Huang <kai.huang@intel.com>
---

Hi Dave, Sean, Jarkko,

Could you help to review?  Tested on SGX (BIOS) enabled machine with
CONFIG_X86_SGX unset.

This patch is generated on latest tip/master, but it applies to
tip/x86/sgx cleanly as well.

---
 arch/x86/kernel/cpu/feat_ctl.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/x86/kernel/cpu/feat_ctl.c b/arch/x86/kernel/cpu/feat_ctl.c
index 993697e71854..2f67409f5f00 100644
--- a/arch/x86/kernel/cpu/feat_ctl.c
+++ b/arch/x86/kernel/cpu/feat_ctl.c
@@ -191,6 +191,19 @@ void init_ia32_feat_ctl(struct cpuinfo_x86 *c)
 		return;
 	}
 
+	/*
+	 * By reaching here, it is certain that:
+	 *  - CPU supports SGX.
+	 *  - SGX is enabled by BIOS.
+	 *
+	 * However if both SGX driver and KVM SGX are not enabled, just
+	 * need to clear SGX feature bit.
+	 */
+	if (!enable_sgx_driver && !enable_sgx_kvm) {
+		clear_cpu_cap(c, X86_FEATURE_SGX);
+		return;
+	}
+
 	/*
 	 * VMX feature bit may be cleared due to being disabled in BIOS,
 	 * in which case SGX virtualization cannot be supported either.

base-commit: b8b09110cf290fdab4006b717da7a776ffb0cb73
-- 
2.37.1

Re: [PATCH] x86/intel: Clear SGX bit if both SGX driver and KVM SGX are not enabled

[Huang, Kai]

On Fri, 2022-09-09 at 20:08 +1200, Kai Huang wrote:
> Currently on platform which has SGX enabled, if CONFIG_X86_SGX is not
> enabled, the X86_FEATURE_SGX is not cleared, resulting in /proc/cpuinfo
> shows "sgx" feature.  This is not desired.
> 
> Clear SGX feature bit if both SGX driver and KVM SGX are not enabled in
> init_ia32_feat_ctl().
> 
> Signed-off-by: Kai Huang <kai.huang@intel.com>
> ---
> 
> Hi Dave, Sean, Jarkko,
> 
> Could you help to review?  Tested on SGX (BIOS) enabled machine with
> CONFIG_X86_SGX unset.
> 
> This patch is generated on latest tip/master, but it applies to
> tip/x86/sgx cleanly as well.
> 
> ---
>  arch/x86/kernel/cpu/feat_ctl.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/feat_ctl.c b/arch/x86/kernel/cpu/feat_ctl.c
> index 993697e71854..2f67409f5f00 100644
> --- a/arch/x86/kernel/cpu/feat_ctl.c
> +++ b/arch/x86/kernel/cpu/feat_ctl.c
> @@ -191,6 +191,19 @@ void init_ia32_feat_ctl(struct cpuinfo_x86 *c)
>  		return;
>  	}
>  
> +	/*
> +	 * By reaching here, it is certain that:
> +	 *  - CPU supports SGX.
> +	 *  - SGX is enabled by BIOS.
> +	 *
> +	 * However if both SGX driver and KVM SGX are not enabled, just
> +	 * need to clear SGX feature bit.
> +	 */
> +	if (!enable_sgx_driver && !enable_sgx_kvm) {
> +		clear_cpu_cap(c, X86_FEATURE_SGX);
> +		return;
> +	}
> +

Sorry my bad.  I sent out the patch too quickly.  This check should be moved
down after checking X86_FEATURE_VMX flag which may result in enable_sgx_kvm
being set to false.  I'll send out v2.

-- 
Thanks,
-Kai

Re: [PATCH] x86/intel: Clear SGX bit if both SGX driver and KVM SGX are not enabled

[Dave Hansen]

On 9/9/22 01:08, Kai Huang wrote:
> Currently on platform which has SGX enabled, if CONFIG_X86_SGX is not
> enabled, the X86_FEATURE_SGX is not cleared, resulting in /proc/cpuinfo
> shows "sgx" feature.  This is not desired.

Why is it not desired?

Re: [PATCH] x86/intel: Clear SGX bit if both SGX driver and KVM SGX are not enabled

[Huang, Kai]

On Fri, 2022-09-09 at 05:11 -0700, Dave Hansen wrote:
> On 9/9/22 01:08, Kai Huang wrote:
> > Currently on platform which has SGX enabled, if CONFIG_X86_SGX is not
> > enabled, the X86_FEATURE_SGX is not cleared, resulting in /proc/cpuinfo
> > shows "sgx" feature.  This is not desired.
> 
> Why is it not desired?

My understanding is if a feature is present in /proc/cpuinfo, that feature
should be supported by the kernel and should be usable to the userspace.  For
example, it seems AMD's SME was always present in /proc/cpuinfo at the very
beginning, but later it was cleared if SME is not activated by the kernel in
below commit:

commit 08f253ec3767bcfafc5d32617a92cee57c63968e (tag: x86_cpu_for_v5.18_rc1)
Author: Mario Limonciello <mario.limonciello@amd.com>
Date:   Tue Feb 15 21:44:46 2022 -0600

    x86/cpu: Clear SME feature flag when not in use
    
    Currently, the SME CPU feature flag is reflective of whether the CPU
    supports the feature but not whether it has been activated by the
    kernel.
    
    Change this around to clear the SME feature flag if the kernel is not
    using it so userspace can determine if it is available and in use
    from /proc/cpuinfo.
    
    As the feature flag is cleared on systems where SME isn't active, use
    CPUID 0x8000001f to confirm SME availability before calling
    native_wbinvd().
    
    Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
    Signed-off-by: Borislav Petkov <bp@suse.de>
    Acked-by: Tom Lendacky <thomas.lendacky@amd.com>
    Link:
https://lore.kernel.org/r/20220216034446.2430634-1-mario.limonciello@amd.com

I am not 100% sure whether this rule should always be applied to other features,
though.  But for SGX even the current upstream code clears SGX feature in some
conditions, for example, when SGX_LC is disabled by BIOS (in which case only KVM
SGX can be supported) and KVM SGX is also disabled, i.e. due to
CONFIG_X86_SGX_KVM isn't set:

    if (!(msr & FEAT_CTL_SGX_LC_ENABLED) && enable_sgx_driver) {
            if (!enable_sgx_kvm) {
                    pr_err_once("SGX Launch Control is locked. Disable SGX.\n");
                    clear_cpu_cap(c, X86_FEATURE_SGX);
	    else {
	            ...
	    }
    }

So I think it makes sense to clear SGX if both SGX driver and KVM SGX are not
enabled by the kernel.


-- 
Thanks,
-Kai

Re: [PATCH] x86/intel: Clear SGX bit if both SGX driver and KVM SGX are not enabled

[Jarkko Sakkinen]

On Fri, Sep 09, 2022 at 08:08:53PM +1200, Kai Huang wrote:
> Currently on platform which has SGX enabled, if CONFIG_X86_SGX is not
> enabled, the X86_FEATURE_SGX is not cleared, resulting in /proc/cpuinfo
> shows "sgx" feature.  This is not desired.
> 
> Clear SGX feature bit if both SGX driver and KVM SGX are not enabled in
> init_ia32_feat_ctl().
> 
> Signed-off-by: Kai Huang <kai.huang@intel.com>

Is it a pattern that flags are cleared when they are disabled by the
kernel? I don't know the answer for this.

I could imagine that sometimes you might want to know whether the CPU
supports a feature, even if e.g. your distribution kernel does not.

There's of course other ways to answer such qeustion, e.g. by using
cpuid utility.

BR, Jarkko

Re: [PATCH] x86/intel: Clear SGX bit if both SGX driver and KVM SGX are not enabled

[Reinette Chatre]

Hi Jarkko,

On 9/12/2022 3:47 AM, Jarkko Sakkinen wrote:
> On Fri, Sep 09, 2022 at 08:08:53PM +1200, Kai Huang wrote:
>> Currently on platform which has SGX enabled, if CONFIG_X86_SGX is not
>> enabled, the X86_FEATURE_SGX is not cleared, resulting in /proc/cpuinfo
>> shows "sgx" feature.  This is not desired.
>>
>> Clear SGX feature bit if both SGX driver and KVM SGX are not enabled in
>> init_ia32_feat_ctl().
>>
>> Signed-off-by: Kai Huang <kai.huang@intel.com>
> 
> Is it a pattern that flags are cleared when they are disabled by the
> kernel? I don't know the answer for this.

There appears to be relevant text in Documentation/x86/cpuinfo.rst:

tl;dr "In general, /proc/cpuinfo shows features which the kernel supports."

Longer quote:
"If users want to know if a feature is available on a given system, they
try to find the flag in /proc/cpuinfo. If a given flag is present, it
means that the kernel supports it and is currently making it available.
If such flag represents a hardware feature, it also means that the
hardware supports it.

If the expected flag does not appear in /proc/cpuinfo, things are murkier.
Users need to find out the reason why the flag is missing and find the way
how to enable it, which is not always easy. There are several factors that
can explain missing flags: the expected feature failed to enable, the feature
is missing in hardware, platform firmware did not enable it, the feature is
disabled at build or run time, an old kernel is in use, or the kernel does
not support the feature and thus has not enabled it. In general, /proc/cpuinfo
shows features which the kernel supports. For a full list of CPUID flags
which the CPU supports, use tools/arch/x86/kcpuid."


> 
> I could imagine that sometimes you might want to know whether the CPU
> supports a feature, even if e.g. your distribution kernel does not.

Indeed ... this seems to fall into the "murkier" part of the above quote.

Reinette

Re: [PATCH] x86/intel: Clear SGX bit if both SGX driver and KVM SGX are not enabled

[Dave Hansen]

On 9/12/22 07:31, Reinette Chatre wrote:
> 
> On 9/12/2022 3:47 AM, Jarkko Sakkinen wrote:
>> On Fri, Sep 09, 2022 at 08:08:53PM +1200, Kai Huang wrote:
>>> Currently on platform which has SGX enabled, if CONFIG_X86_SGX is not
>>> enabled, the X86_FEATURE_SGX is not cleared, resulting in /proc/cpuinfo
>>> shows "sgx" feature.  This is not desired.
>>>
>>> Clear SGX feature bit if both SGX driver and KVM SGX are not enabled in
>>> init_ia32_feat_ctl().
>>>
>>> Signed-off-by: Kai Huang <kai.huang@intel.com>
>> Is it a pattern that flags are cleared when they are disabled by the
>> kernel? I don't know the answer for this.

There's no good pattern.

But, one guideline is that the X86_FEATURE_*'s are for the *kernel*.
They are *not* for userspace.  The fact that the features are exposed to
userspace in cpuinfo is unfortunate, but it doesn't change our guideline.

So, if this patch is done only for the benefit of userspace, I don't
think we should apply it.

Re: [PATCH] x86/intel: Clear SGX bit if both SGX driver and KVM SGX are not enabled

[Huang, Kai]

On Mon, 2022-09-12 at 07:34 -0700, Dave Hansen wrote:
> On 9/12/22 07:31, Reinette Chatre wrote:
> > 
> > On 9/12/2022 3:47 AM, Jarkko Sakkinen wrote:
> > > On Fri, Sep 09, 2022 at 08:08:53PM +1200, Kai Huang wrote:
> > > > Currently on platform which has SGX enabled, if CONFIG_X86_SGX is not
> > > > enabled, the X86_FEATURE_SGX is not cleared, resulting in /proc/cpuinfo
> > > > shows "sgx" feature.  This is not desired.
> > > > 
> > > > Clear SGX feature bit if both SGX driver and KVM SGX are not enabled in
> > > > init_ia32_feat_ctl().
> > > > 
> > > > Signed-off-by: Kai Huang <kai.huang@intel.com>
> > > Is it a pattern that flags are cleared when they are disabled by the
> > > kernel? I don't know the answer for this.
> 
> There's no good pattern.
> 
> But, one guideline is that the X86_FEATURE_*'s are for the *kernel*.
> They are *not* for userspace.  The fact that the features are exposed to
> userspace in cpuinfo is unfortunate, but it doesn't change our guideline.
> 
> So, if this patch is done only for the benefit of userspace, I don't
> think we should apply it.

It's not done for the userspace.  AFAICT no one is complaining about this.  I
just happened to see this and thought it's a right thing to do.

Also, in my other reply to this thread I put one reason that I think it's
reasonable:

"
But for SGX even the current upstream code clears SGX feature in some
conditions, for example, when SGX_LC is disabled by BIOS (in which case only KVM
SGX can be supported) and KVM SGX is also disabled, i.e. due to
CONFIG_X86_SGX_KVM isn't set:

    if (!(msr & FEAT_CTL_SGX_LC_ENABLED) && enable_sgx_driver) {
            if (!enable_sgx_kvm) {
                    pr_err_once("SGX Launch Control is locked. Disable SGX.\n");
                    clear_cpu_cap(c, X86_FEATURE_SGX);
	    else {
	            ...
	    }
    }

So I think it makes sense to clear SGX if both SGX driver and KVM SGX are not
enabled by the kernel.
"

-- 
Thanks,
-Kai