| 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: <6550946b1c7aafedf0525766deb96710938e85b8.camel@intel.com>
Date: Thu, 3 Mar 2022 02:58:21 +0000
From: "Williams, Dan J" <dan.j.williams@...el.com>
To: "hdegoede@...hat.com" <hdegoede@...hat.com>,
"markgross@...nel.org" <markgross@...nel.org>,
"Joseph, Jithu" <jithu.joseph@...el.com>
CC: "corbet@....net" <corbet@....net>,
"Raj, Ashok" <ashok.raj@...el.com>,
"Luck, Tony" <tony.luck@...el.com>,
"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
"patches@...ts.linux.dev" <patches@...ts.linux.dev>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"mingo@...hat.com" <mingo@...hat.com>,
"rostedt@...dmis.org" <rostedt@...dmis.org>,
"Shankar, Ravi V" <ravi.v.shankar@...el.com>,
"tglx@...utronix.de" <tglx@...utronix.de>,
"platform-driver-x86@...r.kernel.org"
<platform-driver-x86@...r.kernel.org>,
"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
"hpa@...or.com" <hpa@...or.com>, "bp@...en8.de" <bp@...en8.de>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"andriy.shevchenko@...ux.intel.com"
<andriy.shevchenko@...ux.intel.com>,
"x86@...nel.org" <x86@...nel.org>
Subject: Re: [RFC 04/10] platform/x86/intel/ifs: Load IFS Image
On Tue, 2022-03-01 at 11:54 -0800, Jithu Joseph wrote:
> IFS uses a test image that can be regarded as firmware. The image is
> specific to a processor family, model and stepping. IFS requires that a
> test image be loaded before any ifs test is initiated. Load the image
> that matches processor signature. The IFS image is signed by Intel.
>
> The IFS image file follows a similar naming convention as used for
> Intel CPU microcode files. The file must be located in the firmware
> directory where the microcode files are placed and named as {family/model
> /stepping}.scan as below:
>
> /lib/firmware/intel/ifs/{ff-mm-ss}.scan
>
> Originally-by: Kyung Min Park <kyung.min.park@...el.com>
> Signed-off-by: Jithu Joseph <jithu.joseph@...el.com>
Sender signoff last, please.
> Reviewed-by: Ashok Raj <ashok.raj@...el.com>
> Reviewed-by: Tony Luck <tony.luck@...el.com>
> ---
> drivers/platform/x86/intel/ifs/Makefile | 2 +-
> drivers/platform/x86/intel/ifs/core.c | 8 +++
> drivers/platform/x86/intel/ifs/ifs.h | 13 ++++
> drivers/platform/x86/intel/ifs/load.c | 95 +++++++++++++++++++++++++
> 4 files changed, 117 insertions(+), 1 deletion(-)
> create mode 100644 drivers/platform/x86/intel/ifs/load.c
>
> diff --git a/drivers/platform/x86/intel/ifs/Makefile b/drivers/platform/x86/intel/ifs/Makefile
> index 05b4925402b4..105b377de410 100644
> --- a/drivers/platform/x86/intel/ifs/Makefile
> +++ b/drivers/platform/x86/intel/ifs/Makefile
> @@ -4,4 +4,4 @@
>
> obj-$(CONFIG_INTEL_IFS) += intel_ifs.o
>
> -intel_ifs-objs := core.o
> +intel_ifs-objs := core.o load.o
> diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
> index fb3c864d3085..765d9a2c4683 100644
> --- a/drivers/platform/x86/intel/ifs/core.c
> +++ b/drivers/platform/x86/intel/ifs/core.c
> @@ -8,6 +8,7 @@
> #include <asm/cpu_device_id.h>
>
> #include "ifs.h"
> +struct ifs_params ifs_params;
>
> #define X86_MATCH(model) \
> X86_MATCH_VENDOR_FAM_MODEL_FEATURE(INTEL, 6, \
> @@ -24,6 +25,7 @@ static int __init ifs_init(void)
> {
> const struct x86_cpu_id *m;
> u64 ia32_core_caps;
> + int ret;
>
> /* ifs capability check */
> m = x86_match_cpu(ifs_cpu_ids);
> @@ -34,6 +36,12 @@ static int __init ifs_init(void)
> if (!(ia32_core_caps & MSR_IA32_CORE_CAPS_INTEGRITY))
> return -ENODEV;
>
> + ret = load_ifs_binary();
> + if (ret) {
> + pr_err("loading ifs binaries failed\n");
What's wrong the error code returned to echo, why spam the log?
Similar comment I forgot to add on the pr_info() upon unloading the
module in the previous patch. What's wrong with the error code returned
to "modprobe -r"?
> + return ret;
> + }
> +
> return 0;
> }
>
> diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
> index f3f924fced06..f2daf2cfd3e6 100644
> --- a/drivers/platform/x86/intel/ifs/ifs.h
> +++ b/drivers/platform/x86/intel/ifs/ifs.h
> @@ -7,8 +7,21 @@
> #ifndef _IFS_H_
> #define _IFS_H_
>
> +#undef pr_fmt
> +#define pr_fmt(fmt) "ifs: " fmt
It's unfortunate that this is needed when dev_{err,info,dbg} family of
functions would scope the messages appropriately automatically. If only
there was a 'struct device' this driver could reference. More on this
below...
> +
> /* These bits are in the IA32_CORE_CAPABILITIES MSR */
> #define MSR_IA32_CORE_CAPS_INTEGRITY_BIT 2
> #define MSR_IA32_CORE_CAPS_INTEGRITY BIT(MSR_IA32_CORE_CAPS_INTEGRITY_BIT)
>
> +/**
> + * struct ifs_params - global ifs parameter for all cpus.
> + * @loaded_version: stores the currently loaded ifs image version.
> + */
> +struct ifs_params {
> + int loaded_version;
> +};
> +
> +int load_ifs_binary(void);
> +extern struct ifs_params ifs_params;
> #endif
> diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
> new file mode 100644
> index 000000000000..1a5e906c51af
> --- /dev/null
> +++ b/drivers/platform/x86/intel/ifs/load.c
> @@ -0,0 +1,95 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2021 Intel Corporation.
> + *
> + * Author: Jithu Joseph <jithu.joseph@...el.com>
> + */
> +
> +#include <linux/firmware.h>
> +#include <linux/platform_device.h>
> +
> +#include "ifs.h"
> +static const char *ifs_path = "intel/ifs/";
> +
> +struct ifs_header {
> + u32 header_ver;
> + u32 blob_revision;
> + u32 date;
> + u32 processor_sig;
> + u32 check_sum;
> + u32 loader_rev;
> + u32 processor_flags;
> + u32 metadata_size;
> + u32 total_size;
> + u32 fusa_info;
> + u64 reserved;
> +};
> +
> +#define IFS_HEADER_SIZE (sizeof(struct ifs_header))
> +static struct ifs_header *ifs_header_ptr; /* pointer to the ifs image header */
> +static u64 ifs_hash_ptr; /* Address of ifs metadata (hash) */
> +
> +static const struct firmware *load_binary(const char *path)
> +{
> + struct platform_device *ifs_pdev;
> + const struct firmware *fw;
> + int err;
> +
> + ifs_pdev = platform_device_register_simple("ifs", -1, NULL, 0);
This looks like an abuse of the platform_device_register_simple() API
to me, i.e. to register and tear down a device every time someone
echoes a value to a sysfs file. This registration process fires off a
KOBJ_ADD event to tell udev a new device has appeared, and before udev
scripts have a chance to do anything useful this device is gone again.
If I were someone that wanted to automate testing my CPUs as uevent
notifying me when the "ifs" device has arrived would be useful.
If ifs_init() registered a platform device to represent the ifs
interface it would also provide a private place to hang the ifs sysfs
interface rather than glomming onto the /sys/devices/system/cpu core
ABI. I personally don't think ifs functionality belongs under
/sys/devices/system/cpu. Also, with statically defined sysfs attributes
automation scripts would be able to safely assume that the sysfs
interface is ready coincident with the KOBJ_ADD event.
Powered by blists - more mailing lists