Re: [PATCH] Ability to read the MKTME status from userspace (patch v2)
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
> ---
> .../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 ..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.gut...@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
> +S: Supported
> +F: drivers/misc/mktme_status.c
> +
> MMP SUPPORT
> R: Lubomir Rintel
> L: linux-arm-ker...@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) & 0x)/* Bits 63:48 */
> #define TME_ACTIVATE_CRYPTO_AES_XTS_128 1
>
> -/* Values for mktme_status (SW only construct) */
> -#define MKTME_ENABLED0
> -#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:
Re: [PATCH] Ability to read the MKTME status from userspace (patch v2)
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)
On Thu, Jun 25, 2020 at 2:38 PM Dave Hansen 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.gg32...@zn.tnic > > https://lkml.kernel.org/r/20200619161026.gf32...@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)
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.gg32...@zn.tnic > https://lkml.kernel.org/r/20200619161026.gf32...@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)
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)
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.gg32...@zn.tnic https://lkml.kernel.org/r/20200619161026.gf32...@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)
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
[PATCH] Ability to read the MKTME status from userspace (patch v2)
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
---
.../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 ..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.gut...@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
+S: Supported
+F: drivers/misc/mktme_status.c
+
MMP SUPPORT
R: Lubomir Rintel
L: linux-arm-ker...@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) & 0x)/* Bits 63:48 */
#define TME_ACTIVATE_CRYPTO_AES_XTS_1281
-/* Values for mktme_status (SW only construct) */
-#define MKTME_ENABLED 0
-#define MKTME_DISABLED 1
-#define MKTME_UNINITIALIZED2
-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
