| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200619072041.GA2795@kroah.com>
Date: Fri, 19 Jun 2020 09:20:41 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Daniel Gutson <daniel.gutson@...ypsium.com>
Cc: Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>,
Arnd Bergmann <arnd@...db.de>,
Peter Zijlstra <peterz@...radead.org>,
"David S. Miller" <davem@...emloft.net>,
Rob Herring <robh@...nel.org>, Tony Luck <tony.luck@...el.com>,
Rahul Tanwar <rahul.tanwar@...ux.intel.com>,
Xiaoyao Li <xiaoyao.li@...el.com>,
Sean Christopherson <sean.j.christopherson@...el.com>,
Dave Hansen <dave.hansen@...ux.intel.com>,
linux-kernel@...r.kernel.org, Daniel Gutson <daniel@...ypsium.com>
Subject: Re: [PATCH] Ability to read the MKTME status from userspace
On Thu, Jun 18, 2020 at 06:02:15PM -0300, Daniel Gutson wrote:
> From: Daniel Gutson <daniel@...ypsium.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@...ypsium.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@...ypsium.com>
> +S: Supported
> +F: drivers/misc/mktme_status.c
> +
> MMP SUPPORT
> R: Lubomir Rintel <lkundrak@...sk>
> L: linux-arm-kernel@...ts.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@...ypsium.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
Powered by blists - more mailing lists