[PATCH] x86/cpuinfo: Clear X86_FEATURE_TME if TME/MKTME is disabled by BIOS

[PATCH] x86/cpuinfo: Clear X86_FEATURE_TME if TME/MKTME is disabled by BIOS

[Martin Fernandez]

Right now the only way to check this is by greping the kernel logs,
which is inconvinient. This is currently checked for fwupd for
example.

I understand that cpuinfo is supposed to report every feature in the
cpu but since AMD is doing the same for sme/sev I think is good to
have this for Intel too.

Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com>
---
 arch/x86/kernel/cpu/intel.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index fd5dead8371c..7311172aceaf 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -570,6 +570,8 @@ static void detect_tme(struct cpuinfo_x86 *c)

 	if (!TME_ACTIVATE_LOCKED(tme_activate) || !TME_ACTIVATE_ENABLED(tme_activate)) {
 		pr_info_once("x86/tme: not enabled by BIOS\n");
+		if (mktme_status == MKTME_UNINITIALIZED)
+			clear_cpu_cap(c, X86_FEATURE_TME);
 		mktme_status = MKTME_DISABLED;
 		return;
 	}
--
2.30.2

Re: [PATCH] x86/cpuinfo: Clear X86_FEATURE_TME if TME/MKTME is disabled by BIOS

[Richard Hughes]

Tested-by: Richard Hughes <richard@hughsie.com>

On Tue, 14 Jun 2022 at 22:02, Martin Fernandez
<martin.fernandez@eclypsium.com> wrote:
>
> Right now the only way to check this is by greping the kernel logs,
> which is inconvinient. This is currently checked for fwupd for
> example.
>
> I understand that cpuinfo is supposed to report every feature in the
> cpu but since AMD is doing the same for sme/sev I think is good to
> have this for Intel too.
>
> Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com>
> ---
>  arch/x86/kernel/cpu/intel.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index fd5dead8371c..7311172aceaf 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -570,6 +570,8 @@ static void detect_tme(struct cpuinfo_x86 *c)
>
>         if (!TME_ACTIVATE_LOCKED(tme_activate) || !TME_ACTIVATE_ENABLED(tme_activate)) {
>                 pr_info_once("x86/tme: not enabled by BIOS\n");
> +               if (mktme_status == MKTME_UNINITIALIZED)
> +                       clear_cpu_cap(c, X86_FEATURE_TME);
>                 mktme_status = MKTME_DISABLED;
>                 return;
>         }
> --
> 2.30.2
>

Re: [PATCH] x86/cpuinfo: Clear X86_FEATURE_TME if TME/MKTME is disabled by BIOS

[Alison Schofield]

On Tue, Jun 14, 2022 at 06:02:17PM -0300, Martin Fernandez wrote:
> Right now the only way to check this is by greping the kernel logs,
> which is inconvinient. This is currently checked for fwupd for
> example.
> 
> I understand that cpuinfo is supposed to report every feature in the
> cpu but since AMD is doing the same for sme/sev I think is good to
> have this for Intel too.

Martin,

Can you tell, or point me to, more info about your use case?

My first reaction is lying about the cpuinfo is not a soln, since
it creates a problem for a users currently relying on cpuinfo to be
the source of truth for TME. Are we to tell them to go look in the
log now, because fwupd folks didn't want to ;)

What were your other options?

Alison

> 
> Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com>
> ---
>  arch/x86/kernel/cpu/intel.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index fd5dead8371c..7311172aceaf 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -570,6 +570,8 @@ static void detect_tme(struct cpuinfo_x86 *c)
> 
>  	if (!TME_ACTIVATE_LOCKED(tme_activate) || !TME_ACTIVATE_ENABLED(tme_activate)) {
>  		pr_info_once("x86/tme: not enabled by BIOS\n");
> +		if (mktme_status == MKTME_UNINITIALIZED)
> +			clear_cpu_cap(c, X86_FEATURE_TME);
>  		mktme_status = MKTME_DISABLED;
>  		return;
>  	}
> --
> 2.30.2
> 

Re: [PATCH] x86/cpuinfo: Clear X86_FEATURE_TME if TME/MKTME is disabled by BIOS

[Richard Hughes]

On Wed, 15 Jun 2022 at 20:06, Alison Schofield
<alison.schofield@intel.com> wrote:
> My first reaction is lying about the cpuinfo is not a soln, since
> it creates a problem for a users currently relying on cpuinfo to be
> the source of truth for TME.

I think you have to qualify "source of truth". At the moment the CPU
reports "Yes! I support TME!" and then for one reason or another the
platform turns it off and actually there's no memory encryption of
your secrets at all. There's seemingly no userspace way of telling if
TME is actually active. We were told that we shouldn't export the
"platform has disabled a CPU feature" in sysfs and just to clear the
cpuid flag that gets exported (like AMD is currently doing) which is
what Martin proposed here. Programs want to know the true CPU
capability can do __get_cpuid_count() like they can for the SME/SEV
capabilities.

> Are we to tell them to go look in the
> log now, because fwupd folks didn't want to ;)

We're not telling anyone to use the log; grepping megabytes of
unformatted kernel logs is a terrible (and slow) way to get one
boolean value.

Richard.

Re: [PATCH] x86/cpuinfo: Clear X86_FEATURE_TME if TME/MKTME is disabled by BIOS

[Alison Schofield]

On Wed, Jun 15, 2022 at 08:34:58PM +0100, Richard Hughes wrote:
> On Wed, 15 Jun 2022 at 20:06, Alison Schofield
> <alison.schofield@intel.com> wrote:
> > My first reaction is lying about the cpuinfo is not a soln, since
> > it creates a problem for a users currently relying on cpuinfo to be
> > the source of truth for TME.
> 
> I think you have to qualify "source of truth". At the moment the CPU
> reports "Yes! I support TME!" and then for one reason or another the
> platform turns it off and actually there's no memory encryption of
> your secrets at all. There's seemingly no userspace way of telling if
> TME is actually active. We were told that we shouldn't export the
> "platform has disabled a CPU feature" in sysfs and just to clear the
> cpuid flag that gets exported (like AMD is currently doing) which is
> what Martin proposed here. Programs want to know the true CPU
> capability can do __get_cpuid_count() like they can for the SME/SEV
> capabilities.
>
Disagree on sending folks to use __get_cpuid_count() when they already
have cpuinfo.

Why is a sysfs entry TME-enabled 0/1 a bad thing?  It can be documented
to have the same meaning as the log message.

You keep referring to AMD. How is their exception documented?

Alison

> > Are we to tell them to go look in the
> > log now, because fwupd folks didn't want to ;)
> 
> We're not telling anyone to use the log; grepping megabytes of
> unformatted kernel logs is a terrible (and slow) way to get one
> boolean value.
> 
> Richard.

Re: [PATCH] x86/cpuinfo: Clear X86_FEATURE_TME if TME/MKTME is disabled by BIOS

[Daniel Gutson]

On Wed, Jun 15, 2022 at 4:54 PM Alison Schofield
<alison.schofield@intel.com> wrote:
>
> On Wed, Jun 15, 2022 at 08:34:58PM +0100, Richard Hughes wrote:
> > On Wed, 15 Jun 2022 at 20:06, Alison Schofield
> > <alison.schofield@intel.com> wrote:
> > > My first reaction is lying about the cpuinfo is not a soln, since
> > > it creates a problem for a users currently relying on cpuinfo to be
> > > the source of truth for TME.
> >
> > I think you have to qualify "source of truth". At the moment the CPU
> > reports "Yes! I support TME!" and then for one reason or another the
> > platform turns it off and actually there's no memory encryption of
> > your secrets at all. There's seemingly no userspace way of telling if
> > TME is actually active. We were told that we shouldn't export the
> > "platform has disabled a CPU feature" in sysfs and just to clear the
> > cpuid flag that gets exported (like AMD is currently doing) which is
> > what Martin proposed here. Programs want to know the true CPU
> > capability can do __get_cpuid_count() like they can for the SME/SEV
> > capabilities.
> >
> Disagree on sending folks to use __get_cpuid_count() when they already
> have cpuinfo.
>
> Why is a sysfs entry TME-enabled 0/1 a bad thing?

:)))
This was my very first patch, and I got half of the community complaining
It was so long ago that I don't recall everything, maybe Martín does?
or Richard?

  It can be documented
> to have the same meaning as the log message.
>
> You keep referring to AMD. How is their exception documented?
>
> Alison
>
> > > Are we to tell them to go look in the
> > > log now, because fwupd folks didn't want to ;)
> >
> > We're not telling anyone to use the log; grepping megabytes of
> > unformatted kernel logs is a terrible (and slow) way to get one
> > boolean value.
> >
> > Richard.

Re: [PATCH] x86/cpuinfo: Clear X86_FEATURE_TME if TME/MKTME is disabled by BIOS

[Martin Fernandez]

On 6/15/22, Daniel Gutson <daniel.gutson@eclypsium.com> wrote:
> On Wed, Jun 15, 2022 at 4:54 PM Alison Schofield
> <alison.schofield@intel.com> wrote:
>>
>> On Wed, Jun 15, 2022 at 08:34:58PM +0100, Richard Hughes wrote:
>> > On Wed, 15 Jun 2022 at 20:06, Alison Schofield
>> > <alison.schofield@intel.com> wrote:
>> > > My first reaction is lying about the cpuinfo is not a soln, since
>> > > it creates a problem for a users currently relying on cpuinfo to be
>> > > the source of truth for TME.
>> >
>> > I think you have to qualify "source of truth". At the moment the CPU
>> > reports "Yes! I support TME!" and then for one reason or another the
>> > platform turns it off and actually there's no memory encryption of
>> > your secrets at all. There's seemingly no userspace way of telling if
>> > TME is actually active. We were told that we shouldn't export the
>> > "platform has disabled a CPU feature" in sysfs and just to clear the
>> > cpuid flag that gets exported (like AMD is currently doing) which is
>> > what Martin proposed here. Programs want to know the true CPU
>> > capability can do __get_cpuid_count() like they can for the SME/SEV
>> > capabilities.
>> >
>> Disagree on sending folks to use __get_cpuid_count() when they already
>> have cpuinfo.
>>
>> Why is a sysfs entry TME-enabled 0/1 a bad thing?
>
> :)))
> This was my very first patch, and I got half of the community complaining
> It was so long ago that I don't recall everything, maybe Martín does?
> or Richard?

The discussion triggered the fact that checking that TME is active is
not enough to tell if memory is being encrypted or not (which we
thought it was true by that time), and that triggered a series of patches to
address the other checks required, which is currently going nowhere
[1].

The sysfs _wasn't_ discarded perse, but since Boris suggested the
change in cpuinfo (several times now that I recalled that Daniel patch
[2]) I think that is cleaner, besides the backwards compatibility.

[1] https://lore.kernel.org/linux-efi/20220429201717.1946178-1-martin.fernandez@eclypsium.com/

[2] https://lkml.iu.edu/hypermail/linux/kernel/2006.2/05231.html

>> It can be documented
>> to have the same meaning as the log message.
>>
>> You keep referring to AMD. How is their exception documented?
>>
>> Alison
>>
>> > > Are we to tell them to go look in the
>> > > log now, because fwupd folks didn't want to ;)
>> >
>> > We're not telling anyone to use the log; grepping megabytes of
>> > unformatted kernel logs is a terrible (and slow) way to get one
>> > boolean value.
>> >
>> > Richard.
>

Re: [PATCH] x86/cpuinfo: Clear X86_FEATURE_TME if TME/MKTME is disabled by BIOS

[Alison Schofield]

On Wed, Jun 15, 2022 at 05:48:34PM -0300, Martin Fernandez wrote:
> On 6/15/22, Daniel Gutson <daniel.gutson@eclypsium.com> wrote:
> > On Wed, Jun 15, 2022 at 4:54 PM Alison Schofield
> > <alison.schofield@intel.com> wrote:
> >>
> >> On Wed, Jun 15, 2022 at 08:34:58PM +0100, Richard Hughes wrote:
> >> > On Wed, 15 Jun 2022 at 20:06, Alison Schofield
> >> > <alison.schofield@intel.com> wrote:
> >> > > My first reaction is lying about the cpuinfo is not a soln, since
> >> > > it creates a problem for a users currently relying on cpuinfo to be
> >> > > the source of truth for TME.
> >> >
> >> > I think you have to qualify "source of truth". At the moment the CPU
> >> > reports "Yes! I support TME!" and then for one reason or another the
> >> > platform turns it off and actually there's no memory encryption of
> >> > your secrets at all. There's seemingly no userspace way of telling if
> >> > TME is actually active. We were told that we shouldn't export the
> >> > "platform has disabled a CPU feature" in sysfs and just to clear the
> >> > cpuid flag that gets exported (like AMD is currently doing) which is
> >> > what Martin proposed here. Programs want to know the true CPU
> >> > capability can do __get_cpuid_count() like they can for the SME/SEV
> >> > capabilities.
> >> >
> >> Disagree on sending folks to use __get_cpuid_count() when they already
> >> have cpuinfo.
> >>
> >> Why is a sysfs entry TME-enabled 0/1 a bad thing?
> >
> > :)))
> > This was my very first patch, and I got half of the community complaining
> > It was so long ago that I don't recall everything, maybe Martín does?
> > or Richard?
> 
> The discussion triggered the fact that checking that TME is active is
> not enough to tell if memory is being encrypted or not (which we
> thought it was true by that time), and that triggered a series of patches to
> address the other checks required, which is currently going nowhere
> [1].
> 
> The sysfs _wasn't_ discarded perse, but since Boris suggested the
> change in cpuinfo (several times now that I recalled that Daniel patch
> [2]) I think that is cleaner, besides the backwards compatibility.
> 
> [1] https://lore.kernel.org/linux-efi/20220429201717.1946178-1-martin.fernandez@eclypsium.com/
> 
> [2] https://lkml.iu.edu/hypermail/linux/kernel/2006.2/05231.html
> 

Martin,
The commit message here seemed to assume that we've all been following
along on this journey with you. Perhaps a commit message that explains
the need and includes alternatives considered/rejected. Links are good!
Alison


> >> It can be documented
> >> to have the same meaning as the log message.
> >>
> >> You keep referring to AMD. How is their exception documented?
> >>
> >> Alison
> >>
> >> > > Are we to tell them to go look in the
> >> > > log now, because fwupd folks didn't want to ;)
> >> >
> >> > We're not telling anyone to use the log; grepping megabytes of
> >> > unformatted kernel logs is a terrible (and slow) way to get one
> >> > boolean value.
> >> >
> >> > Richard.
> >

Re: [PATCH] x86/cpuinfo: Clear X86_FEATURE_TME if TME/MKTME is disabled by BIOS

[Kai Huang]

On Tue, 2022-06-14 at 18:02 -0300, Martin Fernandez wrote:
> Right now the only way to check this is by greping the kernel logs,
> which is inconvinient. This is currently checked for fwupd for
> example.
> 
> I understand that cpuinfo is supposed to report every feature in the
> cpu but since AMD is doing the same for sme/sev I think is good to
> have this for Intel too.
> 
> Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com>
> ---
>  arch/x86/kernel/cpu/intel.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index fd5dead8371c..7311172aceaf 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -570,6 +570,8 @@ static void detect_tme(struct cpuinfo_x86 *c)
> 
>  	if (!TME_ACTIVATE_LOCKED(tme_activate) || !TME_ACTIVATE_ENABLED(tme_activate)) {
>  		pr_info_once("x86/tme: not enabled by BIOS\n");
> +		if (mktme_status == MKTME_UNINITIALIZED)
> +			clear_cpu_cap(c, X86_FEATURE_TME);

The above code seems only clear TME on BSP, if I read correctly.  Why not
unconditionally clear this flag?  If TME is detected as not enabled by BIOS on
*this* cpu, then the flag for *this* cpu should be cleared.

Also could you CC Kirill A. Shutemov <kirill.shutemov@linux.intel.com>, who is
the original author of this code?


>  		mktme_status = MKTME_DISABLED;
>  		return;
>  	}
> --
> 2.30.2
> 

-- 
Thanks,
-Kai

Re: [PATCH] x86/cpuinfo: Clear X86_FEATURE_TME if TME/MKTME is disabled by BIOS

[Martin Fernandez]

On 6/15/22, Kai Huang <kai.huang@intel.com> wrote:
> On Tue, 2022-06-14 at 18:02 -0300, Martin Fernandez wrote:
>> Right now the only way to check this is by greping the kernel logs,
>> which is inconvinient. This is currently checked for fwupd for
>> example.
>>
>> I understand that cpuinfo is supposed to report every feature in the
>> cpu but since AMD is doing the same for sme/sev I think is good to
>> have this for Intel too.
>>
>> Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com>
>> ---
>>  arch/x86/kernel/cpu/intel.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
>> index fd5dead8371c..7311172aceaf 100644
>> --- a/arch/x86/kernel/cpu/intel.c
>> +++ b/arch/x86/kernel/cpu/intel.c
>> @@ -570,6 +570,8 @@ static void detect_tme(struct cpuinfo_x86 *c)
>>
>>  	if (!TME_ACTIVATE_LOCKED(tme_activate) ||
>> !TME_ACTIVATE_ENABLED(tme_activate)) {
>>  		pr_info_once("x86/tme: not enabled by BIOS\n");
>> +		if (mktme_status == MKTME_UNINITIALIZED)
>> +			clear_cpu_cap(c, X86_FEATURE_TME);
>
> The above code seems only clear TME on BSP, if I read correctly.  Why not
> unconditionally clear this flag?  If TME is detected as not enabled by BIOS
> on
> *this* cpu, then the flag for *this* cpu should be cleared.

Yes, I think you're right. Thanks.

> Also could you CC Kirill A. Shutemov <kirill.shutemov@linux.intel.com>, who
> is
> the original author of this code?

I'll add Kirill for the next version, thank you for mention it.

>>  		mktme_status = MKTME_DISABLED;
>>  		return;
>>  	}
>> --
>> 2.30.2
>>
>
> --
> Thanks,
> -Kai
>
>
>

[PATCH] x86/cpuinfo: Clear X86_FEATURE_TME if TME/MKTME is disabled by BIOS

[Martin Fernandez]

Right now the only way to check this is by greping the kernel logs,
which is inconvinient. This is currently checked for fwupd for
example.

I understand that cpuinfo is supposed to report every feature in the
cpu but since AMD is doing the same for sme/sev I think is good to
have this for Intel too.

Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com>
---
 arch/x86/kernel/cpu/intel.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index fd5dead8371c..7311172aceaf 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -570,6 +570,8 @@ static void detect_tme(struct cpuinfo_x86 *c)

 	if (!TME_ACTIVATE_LOCKED(tme_activate) || !TME_ACTIVATE_ENABLED(tme_activate)) {
 		pr_info_once("x86/tme: not enabled by BIOS\n");
+		if (mktme_status == MKTME_UNINITIALIZED)
+			clear_cpu_cap(c, X86_FEATURE_TME);
 		mktme_status = MKTME_DISABLED;
 		return;
 	}
--
2.30.2

Re: [PATCH] x86/cpuinfo: Clear X86_FEATURE_TME if TME/MKTME is disabled by BIOS

[Martin Fernandez]

On 7/4/22, Martin Fernandez <martin.fernandez@eclypsium.com> wrote:
> Right now the only way to check this is by greping the kernel logs,
> which is inconvinient. This is currently checked for fwupd for
> example.
>
> I understand that cpuinfo is supposed to report every feature in the
> cpu but since AMD is doing the same for sme/sev I think is good to
> have this for Intel too.
>
> Signed-off-by: Martin Fernandez <martin.fernandez@eclypsium.com>
> ---
>  arch/x86/kernel/cpu/intel.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index fd5dead8371c..7311172aceaf 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -570,6 +570,8 @@ static void detect_tme(struct cpuinfo_x86 *c)
>
>  	if (!TME_ACTIVATE_LOCKED(tme_activate) ||
> !TME_ACTIVATE_ENABLED(tme_activate)) {
>  		pr_info_once("x86/tme: not enabled by BIOS\n");
> +		if (mktme_status == MKTME_UNINITIALIZED)
> +			clear_cpu_cap(c, X86_FEATURE_TME);
>  		mktme_status = MKTME_DISABLED;
>  		return;
>  	}
> --
> 2.30.2
>
>

I'm sorry, disregard this patch. I wrongly sent an old one. My apologies.