Right now the only way to check this is by greping the kernel logs,
which is inconvenient. 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 (and it also broke backwards
compatibility [1]) for sme/sev I think it's good to have this for
Intel too.
Another option to prevent greping the logs would be a file in
sysfs. I'm open to suggestions to where to place this infomartion. I
saw a proposal about a firmware security filesystem [2]; that would
fit perfectly.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/patch/?id=08f253ec3767bcfafc5d32617a92cee57c63968e
[2] https://lore.kernel.org/all/[email protected]/
Changelog since v1
Clear the flag not only for BSP but for every cpu in the system.
Signed-off-by: Martin Fernandez <[email protected]>
---
arch/x86/kernel/cpu/intel.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index fd5dead8371c..17f23e23f911 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -570,6 +570,7 @@ 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");
+ clear_cpu_cap(c, X86_FEATURE_TME);
mktme_status = MKTME_DISABLED;
return;
}
--
2.30.2
On Mon, 2022-07-04 at 11:22 -0300, Martin Fernandez wrote:
> Right now the only way to check this is by greping the kernel logs,
> which is inconvenient. 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 (and it also broke backwards
> compatibility [1]) for sme/sev I think it's good to have this for
> Intel too.
>
> Another option to prevent greping the logs would be a file in
> sysfs. I'm open to suggestions to where to place this infomartion. I
> saw a proposal about a firmware security filesystem [2]; that would
> fit perfectly.
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/patch/?id=08f253ec3767bcfafc5d32617a92cee57c63968e
>
> [2] https://lore.kernel.org/all/[email protected]/
Leave above to others, but...
>
> Changelog since v1
>
> Clear the flag not only for BSP but for every cpu in the system.
... the changelog history shouldn't be in the commit message.
You can put one additional '---' after your 'Signed-off-by' and add your
changelog after it. The content between two '---'s will be stripped away by
'git am' when maintainer takes the patch:
Signed-off-by: Martin Fernandez <[email protected]>
---
v1->v2:
xxx
---
arch/x86/kernel/cpu/intel.c | 1 +
1 file changed, 1 insertion(+)
>
> Signed-off-by: Martin Fernandez <[email protected]>
> ---
> arch/x86/kernel/cpu/intel.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index fd5dead8371c..17f23e23f911 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -570,6 +570,7 @@ 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");
> + clear_cpu_cap(c, X86_FEATURE_TME);
> mktme_status = MKTME_DISABLED;
> return;
This code change itself looks good to me.
But, TME actually supports bypassing TME encryption/decryption by setting 1 to
bit 31 to IA32_TME_ACTIVATE MSR. See 'Table 4-2 IA32_TME_ACTIVATE MSR' in MKTME
spec below:
https://edc.intel.com/content/www/us/en/design/ipla/software-development-platforms/client/platforms/alder-lake-desktop/12th-generation-intel-core-processors-datasheet-volume-1-of-2/002/intel-multi-key-total-memory-encryption/
When bit 31 is set, the TME is bypassed (no encryption/decryption for KeyID 0).
So looks userspace also needs to check this if it wants to truly check whether
"TME memory encryption" is active.
But perhaps it's arguable whether we can also clear TME flag in this case.
So:
Acked-by: Kai Huang <[email protected]>
--
Thanks,
-Kai
On 7/5/22, Kai Huang <[email protected]> wrote:
> On Mon, 2022-07-04 at 11:22 -0300, Martin Fernandez wrote:
>> Right now the only way to check this is by greping the kernel logs,
>> which is inconvenient. 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 (and it also broke backwards
>> compatibility [1]) for sme/sev I think it's good to have this for
>> Intel too.
>>
>> Another option to prevent greping the logs would be a file in
>> sysfs. I'm open to suggestions to where to place this infomartion. I
>> saw a proposal about a firmware security filesystem [2]; that would
>> fit perfectly.
>>
>> [1]
>> https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/patch/?id=08f253ec3767bcfafc5d32617a92cee57c63968e
>>
>> [2]
>> https://lore.kernel.org/all/[email protected]/
>
> Leave above to others, but...
>>
>> Changelog since v1
>>
>> Clear the flag not only for BSP but for every cpu in the system.
>
> ... the changelog history shouldn't be in the commit message.
>
> You can put one additional '---' after your 'Signed-off-by' and add your
> changelog after it. The content between two '---'s will be stripped away
> by
> 'git am' when maintainer takes the patch:
>
> Signed-off-by: Martin Fernandez <[email protected]>
> ---
> v1->v2:
> xxx
> ---
> arch/x86/kernel/cpu/intel.c | 1 +
> 1 file changed, 1 insertion(+)'
Thanks!, didn't know about it, makes sense.
>>
>> Signed-off-by: Martin Fernandez <[email protected]>
>> ---
>> arch/x86/kernel/cpu/intel.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
>> index fd5dead8371c..17f23e23f911 100644
>> --- a/arch/x86/kernel/cpu/intel.c
>> +++ b/arch/x86/kernel/cpu/intel.c
>> @@ -570,6 +570,7 @@ 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");
>> + clear_cpu_cap(c, X86_FEATURE_TME);
>> mktme_status = MKTME_DISABLED;
>> return;
>
> This code change itself looks good to me.
>
> But, TME actually supports bypassing TME encryption/decryption by setting 1
> to
> bit 31 to IA32_TME_ACTIVATE MSR. See 'Table 4-2 IA32_TME_ACTIVATE MSR' in
> MKTME
> spec below:
>
> https://edc.intel.com/content/www/us/en/design/ipla/software-development-platforms/client/platforms/alder-lake-desktop/12th-generation-intel-core-processors-datasheet-volume-1-of-2/002/intel-multi-key-total-memory-encryption/
>
> When bit 31 is set, the TME is bypassed (no encryption/decryption for KeyID
> 0).
>
> So looks userspace also needs to check this if it wants to truly check
> whether
> "TME memory encryption" is active.
>
> But perhaps it's arguable whether we can also clear TME flag in this case.
Yep, that's what I thought.
> So:
>
> Acked-by: Kai Huang <[email protected]>
>
>
> --
> Thanks,
> -Kai
>
>
>
On Tue, Jul 05, 2022, Martin Fernandez wrote:
> On 7/5/22, Kai Huang <[email protected]> wrote:
> > On Mon, 2022-07-04 at 11:22 -0300, Martin Fernandez wrote:
> >> Changelog since v1
> >>
> >> Clear the flag not only for BSP but for every cpu in the system.
...
> >> ---
> >> arch/x86/kernel/cpu/intel.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> >> index fd5dead8371c..17f23e23f911 100644
> >> --- a/arch/x86/kernel/cpu/intel.c
> >> +++ b/arch/x86/kernel/cpu/intel.c
> >> @@ -570,6 +570,7 @@ 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");
> >> + clear_cpu_cap(c, X86_FEATURE_TME);
This misses the case where the TME_ACTIVATE_KEYID_BITS() is zero. AFAICT, that's
allowed, i.e. won't #GP on WRMSR. TME_ACTIVATE_KEYID_BITS() can't be non-zero if
TME_ACTIVATE_ENABLED() is false, but the reverse is allowed.
> >> mktme_status = MKTME_DISABLED;
> >> return;
> >
> > This code change itself looks good to me.
> >
> > But, TME actually supports bypassing TME encryption/decryption by setting 1
> > to bit 31 to IA32_TME_ACTIVATE MSR. See 'Table 4-2 IA32_TME_ACTIVATE MSR'
> > in MKTME spec below:
> >
> > https://edc.intel.com/content/www/us/en/design/ipla/software-development-platforms/client/platforms/alder-lake-desktop/12th-generation-intel-core-processors-datasheet-volume-1-of-2/002/intel-multi-key-total-memory-encryption/
> >
> > When bit 31 is set, the TME is bypassed (no encryption/decryption for KeyID 0).
> >
> > So looks userspace also needs to check this if it wants to truly check
> > whether "TME memory encryption" is active.
> >
> > But perhaps it's arguable whether we can also clear TME flag in this case.
>
> Yep, that's what I thought.
IMO, this entire function needs to be reworked to have a cohesive strategy for
enumerting TME; not just enumerating to userspace, but internal to the kernel as
well.
E.g. forcing "mktme_status = MKTME_DISABLED" on an AP is nonsensical. If an AP's
basic MKTME enabling doesn't align with the BSP (activate, algorithm, and keyid0
bypass settings match), then there's no way an AP is going to reach detect_tme().
Any discrepancy in encryption for keyid0 will cause the AP will read garbage on
wakeup, and barring a miracle, will triple fault and never call in.
Conversely, if basic enabling matches but something else mismatches, e.g. an AP
was configured with fewer keys, then forcing "mktme_status = MKTME_DISABLED" may
be misleading as MKTME may be fully enabled and in use for keyid0, it just won't
be used for keyid!=0. But that's a moot point because as is, the kernel _never_
uses keyid!=0.
And this code is also bogus. Just because the kernel doesn't know the encryption
algorithm doesn't magically turn off encryption for keyid0. Again, mktme_status
confuses "memory is encrypted" with "MKTME is theoretically usable for keyid!=0".
tme_crypto_algs = TME_ACTIVATE_CRYPTO_ALGS(tme_activate);
if (!(tme_crypto_algs & TME_ACTIVATE_CRYPTO_AES_XTS_128)) {
pr_err("x86/mktme: No known encryption algorithm is supported: %#llx\n",
tme_crypto_algs);
mktme_status = MKTME_DISABLED;
}
The mktme_status variable seems completely pointless. It's not used anywhere
except to detect that CPU0 vs. APs.
Something like this seems like a sane approach.
---
#define MSR_IA32_TME_ACTIVATE 0x982
/* Helpers to access TME_ACTIVATE MSR */
#define TME_ACTIVATE_LOCKED(x) (x & 0x1)
#define TME_ACTIVATE_ENABLED(x) (x & 0x2)
#define TME_ACTIVATE_KEYID0_BYPASS(x) (x & BIT(31))
#define TME_ACTIVATE_POLICY(x) ((x >> 4) & 0xf) /* Bits 7:4 */
#define TME_ACTIVATE_POLICY_AES_XTS_128 0
#define TME_ACTIVATE_KEYID_BITS(x) ((x >> 32) & 0xf) /* Bits 35:32 */
#define TME_ACTIVATE_CRYPTO_ALGS(x) ((x >> 48) & 0xffff) /* Bits 63:48 */
#define TME_ACTIVATE_CRYPTO_AES_XTS_128 1
static int tme_keyid_bits_cpu0 = -1;
static u64 tme_activate_cpu0;
static void detect_tme(struct cpuinfo_x86 *c)
{
u64 tme_activate, tme_policy, tme_crypto_algs;
rdmsrl(MSR_IA32_TME_ACTIVATE, tme_activate);
if (tme_keyid_bits_cpu0 >= 0) {
/* Broken BIOS? */
if (tme_activate != tme_activate_cpu0)
pr_err_once("x86/tme: configuration is inconsistent between CPUs\n");
/*
* Proceed, stolen keyid bits still needed to be excluded from
* x86_phys_bits. The divergence is all but guaranteed to be
* benign, else this CPU would have died during bringup.
*/
goto adjust_phys_bits;
}
tme_activate_cpu0 = tme_activate;
if (!TME_ACTIVATE_LOCKED(tme_activate) ||
!TME_ACTIVATE_ENABLED(tme_activate))
tme_keyid_bits_cpu0 = 0;
else
tme_keyid_bits_cpu0 = TME_ACTIVATE_KEYID_BITS(tme_activate);
if (!tme_keyid_bits_cpu0) {
pr_info("x86/tme: not enabled by BIOS\n");
setup_clear_cpu_cap(X86_FEATURE_TME);
return;
}
pr_info("x86/tme: enabled by BIOS\n");
if (TME_ACTIVATE_KEYID0_BYPASS(tme_activate)) {
pr_info("x86/tme: KeyID=0 encryption bypass enabled\n");
/*
* Clear the feature flag, memory for keyid0 isn't encrypted so
* for all intents and purposes MKTME is unused.
*/
setup_clear_cpu_cap(X86_FEATURE_TME);
goto adjust_phys_bits;
}
tme_policy = TME_ACTIVATE_POLICY(tme_activate);
if (tme_policy != TME_ACTIVATE_POLICY_AES_XTS_128)
pr_warn("x86/tme: Unknown policy is active: %#llx\n", tme_policy);
tme_crypto_algs = TME_ACTIVATE_CRYPTO_ALGS(tme_activate);
if (!(tme_crypto_algs & TME_ACTIVATE_CRYPTO_AES_XTS_128))
pr_warn("x86/mktme: Unknown encryption algorithm is active: %#llx\n",
tme_crypto_algs);
adjust_phys_bits:
/*
* KeyID bits effectively lower the number of physical address bits.
* Update cpuinfo_x86::x86_phys_bits accordingly. Always use CPU0's
* info for the adjustment. If CPU0 steals more bits, then aligning
* with CPU0 gives the highest chance of survival. If CPU0 steals
* fewer bits, adjusting this CPU's x86_phys_bits won't retroactively
* fix all the calculations done using CPU0's information
*/
c->x86_phys_bits -= tme_keyid_bits_cpu0;
}
On Mon, 2022-07-11 at 17:08 +0000, Sean Christopherson wrote:
> On Tue, Jul 05, 2022, Martin Fernandez wrote:
> > On 7/5/22, Kai Huang <[email protected]> wrote:
> > > On Mon, 2022-07-04 at 11:22 -0300, Martin Fernandez wrote:
> > > > Changelog since v1
> > > >
> > > > Clear the flag not only for BSP but for every cpu in the system.
>
> ...
>
> > > > ---
> > > > arch/x86/kernel/cpu/intel.c | 1 +
> > > > 1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> > > > index fd5dead8371c..17f23e23f911 100644
> > > > --- a/arch/x86/kernel/cpu/intel.c
> > > > +++ b/arch/x86/kernel/cpu/intel.c
> > > > @@ -570,6 +570,7 @@ 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");
> > > > + clear_cpu_cap(c, X86_FEATURE_TME);
>
> This misses the case where the TME_ACTIVATE_KEYID_BITS() is zero. AFAICT, that's
> allowed, i.e. won't #GP on WRMSR. TME_ACTIVATE_KEYID_BITS() can't be non-zero if
> TME_ACTIVATE_ENABLED() is false, but the reverse is allowed.
But this logic applies to "whether MKTME is enabled", but not "TME is enabled",
right?
If either LOCKED or ENABLED bit isn't set, then TME is not enabled. My
understanding is this particular patch is trying to fix the issue that TME flag
isn't cleared when TME is disabled by BIOS.
>
> > > > mktme_status = MKTME_DISABLED;
> > > > return;
> > >
> > > This code change itself looks good to me.
> > >
> > > But, TME actually supports bypassing TME encryption/decryption by setting 1
> > > to bit 31 to IA32_TME_ACTIVATE MSR. See 'Table 4-2 IA32_TME_ACTIVATE MSR'
> > > in MKTME spec below:
> > >
> > > https://edc.intel.com/content/www/us/en/design/ipla/software-development-platforms/client/platforms/alder-lake-desktop/12th-generation-intel-core-processors-datasheet-volume-1-of-2/002/intel-multi-key-total-memory-encryption/
> > >
> > > When bit 31 is set, the TME is bypassed (no encryption/decryption for KeyID 0).
> > >
> > > So looks userspace also needs to check this if it wants to truly check
> > > whether "TME memory encryption" is active.
> > >
> > > But perhaps it's arguable whether we can also clear TME flag in this case.
> >
> > Yep, that's what I thought.
>
> IMO, this entire function needs to be reworked to have a cohesive strategy for
> enumerting TME; not just enumerating to userspace, but internal to the kernel as
> well.
>
> E.g. forcing "mktme_status = MKTME_DISABLED" on an AP is nonsensical. If an AP's
> basic MKTME enabling doesn't align with the BSP (activate, algorithm, and keyid0
> bypass settings match), then there's no way an AP is going to reach detect_tme().
> Any discrepancy in encryption for keyid0 will cause the AP will read garbage on
> wakeup, and barring a miracle, will triple fault and never call in.
>
> Conversely, if basic enabling matches but something else mismatches, e.g. an AP
> was configured with fewer keys, then forcing "mktme_status = MKTME_DISABLED" may
> be misleading as MKTME may be fully enabled and in use for keyid0, it just won't
> be used for keyid!=0. But that's a moot point because as is, the kernel _never_
> uses keyid!=0.
>
> And this code is also bogus. Just because the kernel doesn't know the encryption
> algorithm doesn't magically turn off encryption for keyid0. Again, mktme_status
> confuses "memory is encrypted" with "MKTME is theoretically usable for keyid!=0".
>
> tme_crypto_algs = TME_ACTIVATE_CRYPTO_ALGS(tme_activate);
> if (!(tme_crypto_algs & TME_ACTIVATE_CRYPTO_AES_XTS_128)) {
> pr_err("x86/mktme: No known encryption algorithm is supported: %#llx\n",
> tme_crypto_algs);
> mktme_status = MKTME_DISABLED;
> }
>
> The mktme_status variable seems completely pointless. It's not used anywhere
> except to detect that CPU0 vs. APs.
I think your above saying makes sense, but this is a different topic and should
be in a separate patch IMHO.
This patch basically tries to fix the issue that TME flag isn't cleared when TME
is disabled by BIOS. And fir this purpose, the code change in this patch looks
reasonable to me. Unless I am mistaken, detect_tme() will be called for all
cpus if TME is supported in CPUID but isn't enabled by BIOS (either LOCKED or
ENABLED bit isn't set).
>
>
> Something like this seems like a sane approach.
>
> ---
>
> #define MSR_IA32_TME_ACTIVATE 0x982
>
> /* Helpers to access TME_ACTIVATE MSR */
> #define TME_ACTIVATE_LOCKED(x) (x & 0x1)
> #define TME_ACTIVATE_ENABLED(x) (x & 0x2)
>
> #define TME_ACTIVATE_KEYID0_BYPASS(x) (x & BIT(31))
>
> #define TME_ACTIVATE_POLICY(x) ((x >> 4) & 0xf) /* Bits 7:4 */
> #define TME_ACTIVATE_POLICY_AES_XTS_128 0
>
> #define TME_ACTIVATE_KEYID_BITS(x) ((x >> 32) & 0xf) /* Bits 35:32 */
>
> #define TME_ACTIVATE_CRYPTO_ALGS(x) ((x >> 48) & 0xffff) /* Bits 63:48 */
> #define TME_ACTIVATE_CRYPTO_AES_XTS_128 1
>
> static int tme_keyid_bits_cpu0 = -1;
> static u64 tme_activate_cpu0;
>
> static void detect_tme(struct cpuinfo_x86 *c)
> {
> u64 tme_activate, tme_policy, tme_crypto_algs;
>
> rdmsrl(MSR_IA32_TME_ACTIVATE, tme_activate);
>
> if (tme_keyid_bits_cpu0 >= 0) {
> /* Broken BIOS? */
> if (tme_activate != tme_activate_cpu0)
> pr_err_once("x86/tme: configuration is inconsistent between CPUs\n");
>
> /*
> * Proceed, stolen keyid bits still needed to be excluded from
> * x86_phys_bits. The divergence is all but guaranteed to be
> * benign, else this CPU would have died during bringup.
> */
> goto adjust_phys_bits;
> }
>
> tme_activate_cpu0 = tme_activate;
>
> if (!TME_ACTIVATE_LOCKED(tme_activate) ||
> !TME_ACTIVATE_ENABLED(tme_activate))
> tme_keyid_bits_cpu0 = 0;
> else
> tme_keyid_bits_cpu0 = TME_ACTIVATE_KEYID_BITS(tme_activate);
>
> if (!tme_keyid_bits_cpu0) {
> pr_info("x86/tme: not enabled by BIOS\n");
> setup_clear_cpu_cap(X86_FEATURE_TME);
> return;
> }
>
> pr_info("x86/tme: enabled by BIOS\n");
>
> if (TME_ACTIVATE_KEYID0_BYPASS(tme_activate)) {
> pr_info("x86/tme: KeyID=0 encryption bypass enabled\n");
>
> /*
> * Clear the feature flag, memory for keyid0 isn't encrypted so
> * for all intents and purposes MKTME is unused.
> */
> setup_clear_cpu_cap(X86_FEATURE_TME);
> goto adjust_phys_bits;
> }
>
> tme_policy = TME_ACTIVATE_POLICY(tme_activate);
> if (tme_policy != TME_ACTIVATE_POLICY_AES_XTS_128)
> pr_warn("x86/tme: Unknown policy is active: %#llx\n", tme_policy);
>
> tme_crypto_algs = TME_ACTIVATE_CRYPTO_ALGS(tme_activate);
> if (!(tme_crypto_algs & TME_ACTIVATE_CRYPTO_AES_XTS_128))
> pr_warn("x86/mktme: Unknown encryption algorithm is active: %#llx\n",
> tme_crypto_algs);
>
> adjust_phys_bits:
> /*
> * KeyID bits effectively lower the number of physical address bits.
> * Update cpuinfo_x86::x86_phys_bits accordingly. Always use CPU0's
> * info for the adjustment. If CPU0 steals more bits, then aligning
> * with CPU0 gives the highest chance of survival. If CPU0 steals
> * fewer bits, adjusting this CPU's x86_phys_bits won't retroactively
> * fix all the calculations done using CPU0's information
> */
> c->x86_phys_bits -= tme_keyid_bits_cpu0;
> }
On Tue, Jul 12, 2022, Kai Huang wrote:
> On Mon, 2022-07-11 at 17:08 +0000, Sean Christopherson wrote:
> > On Tue, Jul 05, 2022, Martin Fernandez wrote:
> > > On 7/5/22, Kai Huang <[email protected]> wrote:
> > > > On Mon, 2022-07-04 at 11:22 -0300, Martin Fernandez wrote:
> > > > > Changelog since v1
> > > > >
> > > > > Clear the flag not only for BSP but for every cpu in the system.
> >
> > ...
> >
> > > > > ---
> > > > > arch/x86/kernel/cpu/intel.c | 1 +
> > > > > 1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> > > > > index fd5dead8371c..17f23e23f911 100644
> > > > > --- a/arch/x86/kernel/cpu/intel.c
> > > > > +++ b/arch/x86/kernel/cpu/intel.c
> > > > > @@ -570,6 +570,7 @@ 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");
> > > > > + clear_cpu_cap(c, X86_FEATURE_TME);
> >
> > This misses the case where the TME_ACTIVATE_KEYID_BITS() is zero. AFAICT, that's
> > allowed, i.e. won't #GP on WRMSR. TME_ACTIVATE_KEYID_BITS() can't be non-zero if
> > TME_ACTIVATE_ENABLED() is false, but the reverse is allowed.
>
> But this logic applies to "whether MKTME is enabled", but not "TME is enabled",
> right?
Ah, right, duh.
> > IMO, this entire function needs to be reworked to have a cohesive strategy for
> > enumerting TME; not just enumerating to userspace, but internal to the kernel as
> > well.
> >
> > E.g. forcing "mktme_status = MKTME_DISABLED" on an AP is nonsensical. If an AP's
> > basic MKTME enabling doesn't align with the BSP (activate, algorithm, and keyid0
> > bypass settings match), then there's no way an AP is going to reach detect_tme().
> > Any discrepancy in encryption for keyid0 will cause the AP will read garbage on
> > wakeup, and barring a miracle, will triple fault and never call in.
> >
> > Conversely, if basic enabling matches but something else mismatches, e.g. an AP
> > was configured with fewer keys, then forcing "mktme_status = MKTME_DISABLED" may
> > be misleading as MKTME may be fully enabled and in use for keyid0, it just won't
> > be used for keyid!=0. But that's a moot point because as is, the kernel _never_
> > uses keyid!=0.
> >
> > And this code is also bogus. Just because the kernel doesn't know the encryption
> > algorithm doesn't magically turn off encryption for keyid0. Again, mktme_status
> > confuses "memory is encrypted" with "MKTME is theoretically usable for keyid!=0".
> >
> > tme_crypto_algs = TME_ACTIVATE_CRYPTO_ALGS(tme_activate);
> > if (!(tme_crypto_algs & TME_ACTIVATE_CRYPTO_AES_XTS_128)) {
> > pr_err("x86/mktme: No known encryption algorithm is supported: %#llx\n",
> > tme_crypto_algs);
> > mktme_status = MKTME_DISABLED;
> > }
> >
> > The mktme_status variable seems completely pointless. It's not used anywhere
> > except to detect that CPU0 vs. APs.
>
> I think your above saying makes sense, but this is a different topic and should
> be in a separate patch IMHO.
Yeah, definitely need multiple patches.
> This patch basically tries to fix the issue that TME flag isn't cleared when TME
> is disabled by BIOS. And fir this purpose, the code change in this patch looks
> reasonable to me. Unless I am mistaken, detect_tme() will be called for all
> cpus if TME is supported in CPUID but isn't enabled by BIOS (either LOCKED or
> ENABLED bit isn't set).
But this patch doesn't handle the bypass bit, which _does_ effectively disable
TME when set. E.g. the MKTME spec says:
Software must inspect the Hardware Encryption Enable (bit 1) and TME Encryption
Bypass Enable (bit 31) to determine if TME encryption is enabled.
>
> > This patch basically tries to fix the issue that TME flag isn't cleared when TME
> > is disabled by BIOS. And fir this purpose, the code change in this patch looks
> > reasonable to me. Unless I am mistaken, detect_tme() will be called for all
> > cpus if TME is supported in CPUID but isn't enabled by BIOS (either LOCKED or
> > ENABLED bit isn't set).
>
> But this patch doesn't handle the bypass bit, which _does_ effectively disable
> TME when set. E.g. the MKTME spec says:
>
> Software must inspect the Hardware Encryption Enable (bit 1) and TME Encryption
> Bypass Enable (bit 31) to determine if TME encryption is enabled.
Yeah so my original reply said:
"But perhaps it's arguable whether we can also clear TME flag in this case."
And I only gave my Acked-by.
It completely depends on the purpose of this patch, or what does this patch
claim to do. If it only claims to clear TME bit if BIOS doesn't enable it, then
looks fine to me. If it wants to achieve "clear TME feature flag if encryption
isn't active", then yes you are right.
But as I said perhaps "whether we should clear TME flag when bypass is enabled"
is arguable. After all, what does TME flag in /proc/cpuinfo imply?
On 7/11/22, Kai Huang <[email protected]> wrote:
>
>>
>> > This patch basically tries to fix the issue that TME flag isn't cleared
>> > when TME
>> > is disabled by BIOS. And fir this purpose, the code change in this
>> > patch looks
>> > reasonable to me. Unless I am mistaken, detect_tme() will be called for
>> > all
>> > cpus if TME is supported in CPUID but isn't enabled by BIOS (either
>> > LOCKED or
>> > ENABLED bit isn't set).
>>
>> But this patch doesn't handle the bypass bit, which _does_ effectively
>> disable
>> TME when set. E.g. the MKTME spec says:
>>
>> Software must inspect the Hardware Encryption Enable (bit 1) and TME
>> Encryption
>> Bypass Enable (bit 31) to determine if TME encryption is enabled.
>
> Yeah so my original reply said:
>
> "But perhaps it's arguable whether we can also clear TME flag in this
> case."
>
> And I only gave my Acked-by.
>
> It completely depends on the purpose of this patch, or what does this patch
> claim to do. If it only claims to clear TME bit if BIOS doesn't enable it,
> then
> looks fine to me. If it wants to achieve "clear TME feature flag if
> encryption
> isn't active", then yes you are right.
>
> But as I said perhaps "whether we should clear TME flag when bypass is
> enabled"
> is arguable. After all, what does TME flag in /proc/cpuinfo imply?
>
What we want with this patch is to check whether some kind of memory
encryption is active. Right now we are doing it by checking the "tme
active in BIOS" log, so we are not checking the bypass.
Can you change this bypass bit at runtime? ie, does it make sense to
check it only once at boot time?
If no, then maybe it's ok to check that bit in detect_tme and consider
it for cpuinfo,
If it can change, then probably it's ok to leave this patch as is, and
for our use case maybe we can add a sysfs file that reads that msr.
On Tue, Jul 12, 2022, Martin Fernandez wrote:
> On 7/11/22, Kai Huang <[email protected]> wrote:
> >
> >>
> >> > This patch basically tries to fix the issue that TME flag isn't cleared
> >> > when TME
> >> > is disabled by BIOS. And fir this purpose, the code change in this
> >> > patch looks
> >> > reasonable to me. Unless I am mistaken, detect_tme() will be called for
> >> > all
> >> > cpus if TME is supported in CPUID but isn't enabled by BIOS (either
> >> > LOCKED or
> >> > ENABLED bit isn't set).
> >>
> >> But this patch doesn't handle the bypass bit, which _does_ effectively
> >> disable
> >> TME when set. E.g. the MKTME spec says:
> >>
> >> Software must inspect the Hardware Encryption Enable (bit 1) and TME
> >> Encryption
> >> Bypass Enable (bit 31) to determine if TME encryption is enabled.
> >
> > Yeah so my original reply said:
> >
> > "But perhaps it's arguable whether we can also clear TME flag in this
> > case."
> >
> > And I only gave my Acked-by.
> >
> > It completely depends on the purpose of this patch, or what does this patch
> > claim to do. If it only claims to clear TME bit if BIOS doesn't enable it,
> > then
> > looks fine to me. If it wants to achieve "clear TME feature flag if
> > encryption
> > isn't active", then yes you are right.
> >
> > But as I said perhaps "whether we should clear TME flag when bypass is
> > enabled"
> > is arguable. After all, what does TME flag in /proc/cpuinfo imply?
> >
>
> What we want with this patch is to check whether some kind of memory
> encryption is active. Right now we are doing it by checking the "tme
> active in BIOS" log, so we are not checking the bypass.
>
> Can you change this bypass bit at runtime? ie, does it make sense to
> check it only once at boot time?
No, the MSR has write-once behavior. The LOCK bit is set on the first succesful
WRMSR (or amusingly, on the first SMI).
> If no, then maybe it's ok to check that bit in detect_tme and consider
> it for cpuinfo,
>
> If it can change, then probably it's ok to leave this patch as is, and
> for our use case maybe we can add a sysfs file that reads that msr.