Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 19 Jun 2020 09:20:41 +0200
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     Daniel Gutson <daniel.gutson@eclypsium.com>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
        Arnd Bergmann <arnd@arndb.de>,
        Peter Zijlstra <peterz@infradead.org>,
        "David S. Miller" <davem@davemloft.net>,
        Rob Herring <robh@kernel.org>, Tony Luck <tony.luck@intel.com>,
        Rahul Tanwar <rahul.tanwar@linux.intel.com>,
        Xiaoyao Li <xiaoyao.li@intel.com>,
        Sean Christopherson <sean.j.christopherson@intel.com>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        linux-kernel@vger.kernel.org, Daniel Gutson <daniel@eclypsium.com>
Subject: Re: [PATCH] Ability to read the MKTME status from userspace
Message-ID: <20200619072041.GA2795@kroah.com>
References: <20200618210215.23602-1-daniel.gutson@eclypsium.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200618210215.23602-1-daniel.gutson@eclypsium.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Jun 18, 2020 at 06:02:15PM -0300, Daniel Gutson wrote:
> From: Daniel Gutson <daniel@eclypsium.com>
> 
> 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.
> 
> Signed-off-by: Daniel Gutson <daniel@eclypsium.com>

Code review that is agnostic as to the need for this at all, I figured
might as well as this needs work even if everyone agrees that the
function/feature is needed:

> ---
>  MAINTAINERS                 |  5 +++
>  arch/x86/include/asm/cpu.h  |  8 ++++
>  arch/x86/kernel/cpu/intel.c | 12 +++---
>  drivers/misc/Kconfig        | 11 +++++
>  drivers/misc/Makefile       |  1 +
>  drivers/misc/mktme_status.c | 81 +++++++++++++++++++++++++++++++++++++
>  6 files changed, 113 insertions(+), 5 deletions(-)
>  create mode 100644 drivers/misc/mktme_status.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 50659d76976b..dc3b3c0e4701 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11335,6 +11335,11 @@ W:	https://linuxtv.org
>  T:	git git://linuxtv.org/media_tree.git
>  F:	drivers/media/radio/radio-miropcm20*
>  
> +MKTME_STATUS MKTME STATUS READING SUPPORT

You list the same thing twice on one line?

> +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..8929c1240135 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,
> +	MKTME_DISABLED,
> +	MKTME_UNINITIALIZED
> +};

You are exporting these values to userspace, so you need to specify
exactly what these enums resolve to, AND put them in a userspace visable
.h file so that they can be checked/read/understood.

> +
> +extern enum mktme_status_type get_mktme_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 a19a680542ce..1f6054523226 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -489,11 +489,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)
>  {
> @@ -1107,6 +1103,12 @@ bool handle_user_split_lock(struct pt_regs *regs, long error_code)
>  	return true;
>  }
>  
> +enum mktme_status_type get_mktme_status(void)
> +{
> +	return mktme_status;
> +}
> +EXPORT_SYMBOL_GPL(get_mktme_status);

prefix of the subsystem first please:
	mktme_get_status

Or, better yet, why not just export the variable directly?  Why is this
a function at all?

> +
>  /*
>   * 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 99e151475d8f..0dc978efbbd5 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -465,6 +465,17 @@ 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 driver provides support for reading the MKTME status. The status
> +	  can be read 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).

name of the module, should you enable this, etc...

> +
>  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 9abf2923d831..f2f02efe34fd 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -58,3 +58,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..795993181e77
> --- /dev/null
> +++ b/drivers/misc/mktme_status.c
> @@ -0,0 +1,81 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * MKTME Status driver
> + *
> + * Copyright 2020 (c) Daniel Gutson (daniel.gutson@eclypsium.com)
> + *
> + * This file is licensed under  the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.

these sentances are not needed if you have the SPDX line above, so
please drop them.

> + */
> +
> +#include <linux/module.h>
> +#include <linux/security.h>
> +#include <asm/cpu.h>
> +
> +#ifdef pr_fmt
> +#undef pr_fmt
> +#endif
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

No need to check, just define the thing, BEFORE the #include lines.

> +
> +struct dentry *mktme_dir;
> +struct dentry *mktme_file;

static?

> +
> +/* Buffer to return: always 3 because of the following chars:
> + *     value \n \0
> + */
> +#define BUFFER_SIZE 3

Why a define?

> +
> +static ssize_t mktme_status_read(struct file *filp, char __user *buf,
> +				 size_t count, loff_t *ppos)
> +{
> +	char tmp[BUFFER_SIZE];
> +
> +	if (*ppos == BUFFER_SIZE)
> +		return 0; // nothing else to read

Why check this if you are using simple_read_from_buffer()?  Shouldn't
that handle this type of check for you correctly?  As it is, I don't
think you are doing this right anyway.

> +
> +	sprintf(tmp, "%d\n", (int)get_mktme_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 -1;

Don't make up random error numbers, use the EWHATEVER defines please.

> +	}
> +
> +	pr_info("mktme securityfs dir creation successful\n");

If code works properly, it should be quiet, do not do this.  Would you
want to see your kernel log full of this for every individual virtual
file that was created?


> +
> +	mktme_file = securityfs_create_file("status", 0600, mktme_dir, NULL,
> +					    &mktme_status_ops);
> +	if (IS_ERR(mktme_file)) {
> +		pr_err("Error creating sysfs file bioswe\n");

"bioswe", what is that?

And this isn't sysfs.

Did anyone actually review this?


> +		goto out_file;
> +	}
> +
> +	return 0;
> +
> +out_file:
> +	securityfs_remove(mktme_file);
> +	securityfs_remove(mktme_dir);
> +	return -1;

Again, random return values, please do not.

> +}
> +
> +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 driver");

Is this really a driver?

Also no Documentation/ABI/ update for your new userspace api that you
just created?

thanks,

greg k-h