Re: [PATCH] Ability to read the MKTME status from userspace (patch v2)

[Alison Schofield]

On Thu, Jun 25, 2020 at 06:10:29PM -0300, Daniel Gutson wrote:
> The intent of this patch is to provide visibility of the
> MKTME status to userspace. This is an important factor for
> firmware security related applilcations.
> 
> Changes since v1:
> * Documentation/ABI/stable/securityfs-mktme-status (new file)
> * include/uapi/misc/mktme_status.h (new file)
> * Fixed MAINTAINER title.
> * cpu.h: added values to the enumerands
> * Renamed the function from get_mktme_status to mktme_get_status
> * Improved Kconfig help
> * Removed unnecessary license-related lines since there is a SPDX line
> * Moved pr_fmt macro before the includes
> * Turned global variables (mktme_dir, mktme_file) as static
> * Removed BUFFER_SIZE macro
> * No longer returning -1 for error, but using the previously error.
> * No more pr_info for the normal behavior.
> * Renamed this from a kernel driver to a kernel module.
> 
> Signed-off-by: Daniel Gutson <daniel.gutson@xxxxxxxxxxxxx>
> ---
>  .../ABI/stable/securityfs-mktme-status        | 12 ++++
>  MAINTAINERS                                   |  5 ++
>  arch/x86/include/asm/cpu.h                    |  8 +++
>  arch/x86/kernel/cpu/intel.c                   | 12 ++--
>  drivers/misc/Kconfig                          | 15 +++++
>  drivers/misc/Makefile                         |  1 +
>  drivers/misc/mktme_status.c                   | 67 +++++++++++++++++++
>  include/uapi/misc/mktme_status.h              | 12 ++++
>  8 files changed, 127 insertions(+), 5 deletions(-)
>  create mode 100644 Documentation/ABI/stable/securityfs-mktme-status
>  create mode 100644 drivers/misc/mktme_status.c
>  create mode 100644 include/uapi/misc/mktme_status.h
> 
> diff --git a/Documentation/ABI/stable/securityfs-mktme-status b/Documentation/ABI/stable/securityfs-mktme-status
> new file mode 100644
> index 000000000000..a791c6ef2c15
> --- /dev/null
> +++ b/Documentation/ABI/stable/securityfs-mktme-status
> @@ -0,0 +1,12 @@
> +What: 		/securityfs/mktme/status
> +Date:		24-Jun-2020
> +KernelVersion:	v5.7
> +Contact:	daniel.gutson@xxxxxxxxxxxxx
> +Description: 	The mktme securityfs interface exports the BIOS status
> +		of the MKTME.
> +Values:         For possible values see arch/x86/include/asm/cpu.h.
> +
> +		Currently these values are:
> +		0 Enabled by BIOS
> +		1 Disabled by BIOS
> +		2 Uninitialized
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7b5ffd646c6b..e034e8ab6fe1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11477,6 +11477,11 @@ W:	https://linuxtv.org
>  T:	git git://linuxtv.org/media_tree.git
>  F:	drivers/media/radio/radio-miropcm20*
>  
> +MKTME STATUS READING SUPPORT
> +M:	Daniel Gutson <daniel.gutson@xxxxxxxxxxxxx>
> +S:	Supported
> +F:	drivers/misc/mktme_status.c
> +
>  MMP SUPPORT
>  R:	Lubomir Rintel <lkundrak@xxxxx>
>  L:	linux-arm-kernel@xxxxxxxxxxxxxxxxxxx (moderated for non-subscribers)
> diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h
> index dd17c2da1af5..80d2896a3f43 100644
> --- a/arch/x86/include/asm/cpu.h
> +++ b/arch/x86/include/asm/cpu.h
> @@ -26,6 +26,14 @@ struct x86_cpu {
>  	struct cpu cpu;
>  };
>  
> +enum mktme_status_type {
> +	MKTME_ENABLED = 0,
> +	MKTME_DISABLED = 1,
> +	MKTME_UNINITIALIZED = 2
> +};
> +
> +extern enum mktme_status_type mktme_get_status(void);
> +
>  #ifdef CONFIG_HOTPLUG_CPU
>  extern int arch_register_cpu(int num);
>  extern void arch_unregister_cpu(int);
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index c25a67a34bd3..134a88685bc3 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -495,11 +495,7 @@ static void srat_detect_node(struct cpuinfo_x86 *c)
>  #define TME_ACTIVATE_CRYPTO_ALGS(x)	((x >> 48) & 0xffff)	/* Bits 63:48 */
>  #define TME_ACTIVATE_CRYPTO_AES_XTS_128	1
>  
> -/* Values for mktme_status (SW only construct) */
> -#define MKTME_ENABLED			0
> -#define MKTME_DISABLED			1
> -#define MKTME_UNINITIALIZED		2
> -static int mktme_status = MKTME_UNINITIALIZED;
> +static enum mktme_status_type mktme_status = MKTME_UNINITIALIZED;
>  
>  static void detect_tme(struct cpuinfo_x86 *c)
>  {
> @@ -1114,6 +1110,12 @@ bool handle_user_split_lock(struct pt_regs *regs, long error_code)
>  	return true;
>  }
>  
> +enum mktme_status_type mktme_get_status(void)
> +{
> +	return mktme_status;
> +}
> +EXPORT_SYMBOL_GPL(mktme_get_status);
> +

Hi Daniel,

It's not clear this code is getting what it says its getting.
(putting aside for the moment the other feedback)

I think you said you want to know if TME is activated in BIOS. I get that.
You don't want the system to be up and the customer can't tell if they
actually turned on TME or not.

If you only look for MKTME enabled status, you may miss the TME enabled
status. We can have TME on and MKTME off. (Can't have opposite)

Here are the boot message I observe based on BIOS selections:

Select TME-off MKTME-off
[] x86/tme: not enabled by BIOS

Select TME-on MKTME-off
[] x86/tme: enabled by BIOS
[] x86/mktme: No known encryption algorithm is supported: 0x0
[] x86/mktme: disabled by BIOS

Select TME-on MKTME-on
[] x86/tme: enabled by BIOS
[] x86/mktme: enabled by BIOS
[] x86/mktme: 63 KeyIDs available

It's that second case above, (TME_on MKTME-off) that this code seems
to be miss. TME will be enabled, just not MKTME.

Alison