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

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

[Daniel Gutson]

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@eclypsium.com>
---
 .../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@eclypsium.com
+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@eclypsium.com>
+S:	Supported
+F:	drivers/misc/mktme_status.c
+
 MMP SUPPORT
 R:	Lubomir Rintel <lkundrak@v3.sk>
 L:	linux-arm-kernel@lists.infradead.org (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);
+
 /*
  * This function is called only when switching between tasks with
  * different split-lock detection modes. It sets the MSR for the
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index e1b1ba5e2b92..c9715bbdd8e5 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -456,6 +456,21 @@ config PVPANIC
 	  a paravirtualized device provided by QEMU; it lets a virtual machine
 	  (guest) communicate panic events to the host.
 
+config MKTME_STATUS
+	tristate "MKTME status reading support"
+	depends on X86_64 && SECURITYFS
+	help
+	  This module provides support for reading the MKTME status.
+	  Enable this configuration option to enable reading the MKTME status
+	  from the mktme virtual file in the securityfs filesystem,
+	  under the mktme directory.
+
+	  The MKTME (Multi-Key Total Memory Encryption) status can be
+	  0 (enabled), 1 (disabled), or 3 (uninitialized).
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called mktme_status.
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index c7bd01ac6291..98c971c096a6 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -57,3 +57,4 @@ obj-$(CONFIG_PVPANIC)   	+= pvpanic.o
 obj-$(CONFIG_HABANA_AI)		+= habanalabs/
 obj-$(CONFIG_UACCE)		+= uacce/
 obj-$(CONFIG_XILINX_SDFEC)	+= xilinx_sdfec.o
+obj-$(CONFIG_MKTME_STATUS)	+= mktme_status.o
diff --git a/drivers/misc/mktme_status.c b/drivers/misc/mktme_status.c
new file mode 100644
index 000000000000..22c14966b4e9
--- /dev/null
+++ b/drivers/misc/mktme_status.c
@@ -0,0 +1,67 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * MKTME Status driver
+ *
+ * Copyright 2020 (c) Daniel Gutson (daniel.gutson@eclypsium.com)
+ *
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>
+#include <linux/security.h>
+#include <asm/cpu.h>
+
+static struct dentry *mktme_dir;
+static struct dentry *mktme_file;
+
+static ssize_t mktme_status_read(struct file *filp, char __user *buf,
+				 size_t count, loff_t *ppos)
+{
+	/* Buffer size 3 because of the following chars:
+	 *     value \n \0
+	 */
+	char tmp[3];
+
+	sprintf(tmp, "%d\n", (int)mktme_get_status() & 1);
+	return simple_read_from_buffer(buf, count, ppos, tmp, sizeof(tmp));
+}
+
+static const struct file_operations mktme_status_ops = {
+	.read = mktme_status_read,
+};
+
+static int __init mod_init(void)
+{
+	mktme_dir = securityfs_create_dir("mktme", NULL);
+	if (IS_ERR(mktme_dir)) {
+		pr_err("Couldn't create mktme sysfs dir\n");
+		return PTR_ERR(mktme_dir);
+	}
+
+	mktme_file = securityfs_create_file("status", 0600, mktme_dir, NULL,
+					    &mktme_status_ops);
+	if (IS_ERR(mktme_file)) {
+		pr_err("Error creating securityfs file 'status'\n");
+		goto out_file;
+	}
+
+	return 0;
+
+out_file:
+	securityfs_remove(mktme_dir);
+	return PTR_ERR(mktme_file);
+}
+
+static void __exit mod_exit(void)
+{
+	securityfs_remove(mktme_file);
+	securityfs_remove(mktme_dir);
+}
+
+module_init(mod_init);
+module_exit(mod_exit);
+
+MODULE_DESCRIPTION("MKTME Status kernel module");
+MODULE_AUTHOR("Daniel Gutson <daniel.gutson@eclypsium.com>");
+MODULE_LICENSE("GPL");
diff --git a/include/uapi/misc/mktme_status.h b/include/uapi/misc/mktme_status.h
new file mode 100644
index 000000000000..04b992d2757f
--- /dev/null
+++ b/include/uapi/misc/mktme_status.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+
+#ifndef __MKTME_STATUS_H__
+#define __MKTME_STATUS_H__
+
+enum mktme_status_type {
+	MKTME_ENABLED = 0,
+	MKTME_DISABLED = 1,
+	MKTME_UNINITIALIZED = 2
+};
+
+#endif /* __MKTME_STATUS_H__ */
-- 
2.25.1

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

[Borislav Petkov]

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.

No, we don't want a module driver which, as Dave explained, tells you
exactly nothing whether encryption is actually enabled on the system.
Didn't that become clear from the thread last time?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Borislav Petkov]

On Thu, Jun 25, 2020 at 06:16:12PM -0300, Daniel Gutson wrote:
> What didn't become clear from the thread last time is the direction to
> proceed. Concrete suggestion?

Here are two:

https://lkml.kernel.org/r/20200619161752.GG32683@zn.tnic
https://lkml.kernel.org/r/20200619161026.GF32683@zn.tnic

but before that happens, I'd like to hear Dave confirm that when we
expose all that information to userspace, it will actually be true and
show the necessary bits which *actually* tell you that encryption is
enabled.

If you're still unclear, go over the thread again pls.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Dave Hansen]

On 6/25/20 2:27 PM, Borislav Petkov wrote:
> On Thu, Jun 25, 2020 at 06:16:12PM -0300, Daniel Gutson wrote:
>> What didn't become clear from the thread last time is the direction to
>> proceed. Concrete suggestion?
> Here are two:
> 
> https://lkml.kernel.org/r/20200619161752.GG32683@zn.tnic
> https://lkml.kernel.org/r/20200619161026.GF32683@zn.tnic
> 
> but before that happens, I'd like to hear Dave confirm that when we
> expose all that information to userspace, it will actually be true and
> show the necessary bits which *actually* tell you that encryption is
> enabled.
> 
> If you're still unclear, go over the thread again pls.

It boils down to this: we shouldn't expose low-level, vendor-specific
implementation details if we can avoid it.  Let's expose something that
app can actually use.

Something that will work for all of the TME, MKTME and SEV platforms
that I know of and continue to work for a while would be to have a
per-numa-node (/sys/devices/system/node[X]/file) that says: "user data
on this node is protected by memory encryption".

SEV guests would always have a 1 in all nodes.

TME systems with no platform screwiness like PMEM would always have a 1.

Old systems would have a 0 in there.

TME systems which also have PMEM-only nodes would set 0 in PMEM nodes
and 1 on DRAM nodes.

Systems with screwy EFI_MEMORY_CPU_CRYPTO mixing within NUMA nodes would
turn it off for the screwy nodes.

Is that concrete enough?

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

[Andy Lutomirski]

On Thu, Jun 25, 2020 at 2:38 PM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 6/25/20 2:27 PM, Borislav Petkov wrote:
> > On Thu, Jun 25, 2020 at 06:16:12PM -0300, Daniel Gutson wrote:
> >> What didn't become clear from the thread last time is the direction to
> >> proceed. Concrete suggestion?
> > Here are two:
> >
> > https://lkml.kernel.org/r/20200619161752.GG32683@zn.tnic
> > https://lkml.kernel.org/r/20200619161026.GF32683@zn.tnic
> >
> > but before that happens, I'd like to hear Dave confirm that when we
> > expose all that information to userspace, it will actually be true and
> > show the necessary bits which *actually* tell you that encryption is
> > enabled.
> >
> > If you're still unclear, go over the thread again pls.
>
> It boils down to this: we shouldn't expose low-level, vendor-specific
> implementation details if we can avoid it.  Let's expose something that
> app can actually use.
>
> Something that will work for all of the TME, MKTME and SEV platforms
> that I know of and continue to work for a while would be to have a
> per-numa-node (/sys/devices/system/node[X]/file) that says: "user data
> on this node is protected by memory encryption".
>
> SEV guests would always have a 1 in all nodes.
>
> TME systems with no platform screwiness like PMEM would always have a 1.
>
> Old systems would have a 0 in there.
>
> TME systems which also have PMEM-only nodes would set 0 in PMEM nodes
> and 1 on DRAM nodes.
>
> Systems with screwy EFI_MEMORY_CPU_CRYPTO mixing within NUMA nodes would
> turn it off for the screwy nodes.
>
> Is that concrete enough?

What about MKTME platforms that (using hypothetical future kernel
support) have encryption enabled for a node but have disabled it for
specific pages using madvise()?  Or that have any other nontrivial
policy like that?

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

[Dave Hansen]

On 6/25/20 2:39 PM, Andy Lutomirski wrote:
> What about MKTME platforms that (using hypothetical future kernel
> support) have encryption enabled for a node but have disabled it for
> specific pages using madvise()?  Or that have any other nontrivial
> policy like that?

I think it's fine if the magic new bit means "normal allocations get
hardware encryption".  If we have a way for users to opt out of that,
that's fine with me because the default is to provide it and a user must
have gone through _some_ hoop to undo the protection.

BTW, although the MKTME hardware and architecture support disabling
encryption, we don't have any plans to expose that to applications.

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

[Dave Hansen]

On 6/25/20 2:10 PM, 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.

We need more specifics than this.  It's an important factor for what,
exactly?  Who will consume this and what will they do with it?

I'm also not sure we want to have an Intel product name in the ABI.  If
we're meaning to tell folks if hardware memory encryption is available
on the platform, let's say _that_, rather than talk about MKTME.

Also, MKTME enabling isn't all that interesting.  TME is much more
interesting and much more opaque.

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@eclypsium.com>
> ---
>  .../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@eclypsium.com
> +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@eclypsium.com>
> +S:	Supported
> +F:	drivers/misc/mktme_status.c
> +
>  MMP SUPPORT
>  R:	Lubomir Rintel <lkundrak@v3.sk>
>  L:	linux-arm-kernel@lists.infradead.org (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