| 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
| ||
|
Date: Fri, 15 Sep 2023 19:46:38 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Jithu Joseph <jithu.joseph@...el.com>
cc: hdegoede@...hat.com, markgross@...nel.org, tglx@...utronix.de,
mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com,
x86@...nel.org, hpa@...or.com, rostedt@...dmis.org,
ashok.raj@...el.com, tony.luck@...el.com,
linux-kernel@...r.kernel.org, platform-driver-x86@...r.kernel.org,
patches@...ts.linux.dev, ravi.v.shankar@...el.com,
pengfei.xu@...el.com
Subject: Re: [PATCH 03/10] platform/x86/intel/ifs: Image loading for new
generations
On Wed, 13 Sep 2023, Jithu Joseph wrote:
> Scan image loading flow for newer IFS generations (1 and 2) are slightly
> different from that of current generation (0). In newer schemes,
> loading need not be done once for each socket as was done in gen0.
>
> Also the width of CHUNK related bitfields in SCAN_HASHES_STATUS MSR has
> increased from 8 -> 16 bits. Similarly there are width differences
> for CHUNK_AUTHENTICATION_STATUS too.
>
> Further the parameter to AUTHENTICATE_AND_COPY_CHUNK is passed
> differently in newer generations.
>
> Signed-off-by: Jithu Joseph <jithu.joseph@...el.com>
> Reviewed-by: Tony Luck <tony.luck@...el.com>
> Tested-by: Pengfei Xu <pengfei.xu@...el.com>
> ---
> drivers/platform/x86/intel/ifs/ifs.h | 27 ++++++
> drivers/platform/x86/intel/ifs/load.c | 113 +++++++++++++++++++++++++-
> 2 files changed, 138 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
> index d666aeed20fc..886dc74de57d 100644
> --- a/drivers/platform/x86/intel/ifs/ifs.h
> +++ b/drivers/platform/x86/intel/ifs/ifs.h
> @@ -137,6 +137,8 @@
> #define MSR_CHUNKS_AUTHENTICATION_STATUS 0x000002c5
> #define MSR_ACTIVATE_SCAN 0x000002c6
> #define MSR_SCAN_STATUS 0x000002c7
> +#define MSR_SAF_CTRL 0x000004f0
> +
> #define SCAN_NOT_TESTED 0
> #define SCAN_TEST_PASS 1
> #define SCAN_TEST_FAIL 2
> @@ -158,6 +160,19 @@ union ifs_scan_hashes_status {
> };
> };
>
> +union ifs_scan_hashes_status_gen2 {
> + u64 data;
> + struct {
> + u16 chunk_size;
> + u16 num_chunks;
> + u8 error_code;
> + u32 chunks_in_stride :9;
> + u32 rsvd :2;
> + u32 max_core_limit :12;
> + u32 valid :1;
This doesn't look it would be guaranteed to provide the alignment you seem
to want for the fields.
> + };
> +};
> +
> /* MSR_CHUNKS_AUTH_STATUS bit fields */
> union ifs_chunks_auth_status {
> u64 data;
> @@ -170,6 +185,16 @@ union ifs_chunks_auth_status {
> };
> };
>
> +union ifs_chunks_auth_status_gen2 {
> + u64 data;
> + struct {
> + u16 valid_chunks;
> + u16 total_chunks;
> + u8 error_code;
> + u32 rsvd :24;
Ditto.
> + };
> +};
> +
> /* MSR_ACTIVATE_SCAN bit fields */
> union ifs_scan {
> u64 data;
> @@ -230,6 +255,7 @@ struct ifs_test_caps {
> * @scan_details: opaque scan status code from h/w
> * @cur_batch: number indicating the currently loaded test file
> * @generation: IFS test generation enumerated by hardware
> + * @chunk_size: size of a test chunk
> */
> struct ifs_data {
> int loaded_version;
> @@ -240,6 +266,7 @@ struct ifs_data {
> u64 scan_details;
> u32 cur_batch;
> u32 generation;
> + u32 chunk_size;
> };
>
> struct ifs_work {
> diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
> index 851c97cc6a6b..e8fb03dd8bcf 100644
> --- a/drivers/platform/x86/intel/ifs/load.c
> +++ b/drivers/platform/x86/intel/ifs/load.c
> @@ -26,6 +26,11 @@ union meta_data {
>
> #define IFS_HEADER_SIZE (sizeof(struct microcode_header_intel))
> #define META_TYPE_IFS 1
> +#define INVALIDATE_STRIDE (0x1UL)
Unnecessary parenthesis.
Align.
> +#define IFS_GEN_STRIDE_AWARE 2
> +#define AUTH_INTERRUPTED_ERROR 5
> +#define IFS_AUTH_RETRY_CT 10
> +
> static struct microcode_header_intel *ifs_header_ptr; /* pointer to the ifs image header */
> static u64 ifs_hash_ptr; /* Address of ifs metadata (hash) */
> static u64 ifs_test_image_ptr; /* 256B aligned address of test pattern */
> @@ -44,7 +49,10 @@ static const char * const scan_hash_status[] = {
> static const char * const scan_authentication_status[] = {
> [0] = "No error reported",
> [1] = "Attempt to authenticate a chunk which is already marked as authentic",
> - [2] = "Chunk authentication error. The hash of chunk did not match expected value"
> + [2] = "Chunk authentication error. The hash of chunk did not match expected value",
> + [3] = "Reserved",
> + [4] = "Chunk outside the current stride",
> + [5] = "Authentication flow interrupted"
Add the trailing comma to avoid the need to touch the line later if more
entries are added.
> };
>
> #define MC_HEADER_META_TYPE_END (0)
> @@ -154,6 +162,104 @@ static void copy_hashes_authenticate_chunks(struct work_struct *work)
> complete(&ifs_done);
> }
>
> +static int get_num_chunks(int gen, union ifs_scan_hashes_status_gen2 status)
> +{
> + return gen >= IFS_GEN_STRIDE_AWARE ? status.chunks_in_stride : status.num_chunks;
Remove extra space.
> +}
> +
> +static bool need_copy_scan_hashes(struct ifs_data *ifsd)
> +{
> + if (!ifsd->loaded || ifsd->generation < IFS_GEN_STRIDE_AWARE ||
> + ifsd->loaded_version != ifs_header_ptr->rev) {
> + return true;
> + }
> + return false;
IMO, this would be easier to read:
return !ifsd->loaded ||
ifsd->generation < IFS_GEN_STRIDE_AWARE ||
ifsd->loaded_version != ifs_header_ptr->rev;
> +}
> +
> +static int copy_hashes_authenticate_chunks_gen2(struct device *dev)
> +{
> + union ifs_scan_hashes_status_gen2 hashes_status;
> + union ifs_chunks_auth_status_gen2 chunk_status;
> + u32 err_code, valid_chunks, total_chunks;
> + int i, num_chunks, chunk_size;
> + union meta_data *ifs_meta;
> + int starting_chunk_nr;
> + struct ifs_data *ifsd;
> + u64 linear_addr, base;
> + u64 chunk_table[2];
> + int retry_count;
> +
> + ifsd = ifs_get_data(dev);
> +
> + if (need_copy_scan_hashes(ifsd)) {
> + wrmsrl(MSR_COPY_SCAN_HASHES, ifs_hash_ptr);
> + rdmsrl(MSR_SCAN_HASHES_STATUS, hashes_status.data);
> +
> + /* enumerate the scan image information */
> + chunk_size = hashes_status.chunk_size * 1024;
SZ_1K ?
> + err_code = hashes_status.error_code;
> +
> + num_chunks = get_num_chunks(ifsd->generation, hashes_status);
> +
> + if (!hashes_status.valid) {
> + hashcopy_err_message(dev, err_code);
> + return -EIO;
> + }
> + ifsd->loaded_version = ifs_header_ptr->rev;
> + ifsd->chunk_size = chunk_size;
> + } else {
> + num_chunks = ifsd->valid_chunks;
> + chunk_size = ifsd->chunk_size;
> + }
> +
> + if (ifsd->generation >= IFS_GEN_STRIDE_AWARE) {
> + wrmsrl(MSR_SAF_CTRL, INVALIDATE_STRIDE);
> + rdmsrl(MSR_CHUNKS_AUTHENTICATION_STATUS, chunk_status.data);
> + if (chunk_status.valid_chunks != 0) {
> + dev_err(dev, "Couldn't invalidate installed stride - %d\n",
> + chunk_status.valid_chunks);
> + return -EIO;
> + }
> + }
> +
> + base = ifs_test_image_ptr;
> + ifs_meta = (union meta_data *)find_meta_data(ifs_header_ptr, META_TYPE_IFS);
> + starting_chunk_nr = ifs_meta->starting_chunk;
> +
> + /* scan data authentication and copy chunks to secured memory */
> + for (i = 0; i < num_chunks; i++) {
> + retry_count = IFS_AUTH_RETRY_CT;
> + linear_addr = base + i * chunk_size;
> +
> + chunk_table[0] = starting_chunk_nr + i;
> + chunk_table[1] = linear_addr;
> +auth_retry:
> + wrmsrl(MSR_AUTHENTICATE_AND_COPY_CHUNK, (u64)chunk_table);
> + rdmsrl(MSR_CHUNKS_AUTHENTICATION_STATUS, chunk_status.data);
> + err_code = chunk_status.error_code;
> + if (err_code == AUTH_INTERRUPTED_ERROR && --retry_count)
> + goto auth_retry;
do {
} while ();
> + if (err_code) {
> + ifsd->loading_error = true;
> + auth_err_message(dev, err_code);
> + return -EIO;
> + }
> + }
> +
> + valid_chunks = chunk_status.valid_chunks;
> + total_chunks = chunk_status.total_chunks;
> +
> + if (valid_chunks != total_chunks) {
> + ifsd->loading_error = true;
> + dev_err(dev, "Couldn't authenticate all the chunks.Authenticated %d total %d.\n",
Missing whitespace.
> + valid_chunks, total_chunks);
> + return -EIO;
> + }
> + ifsd->valid_chunks = valid_chunks;
> +
> + return 0;
> +}
> +
> static int validate_ifs_metadata(struct device *dev)
> {
> struct ifs_data *ifsd = ifs_get_data(dev);
> @@ -206,7 +312,9 @@ static int scan_chunks_sanity_check(struct device *dev)
> return ret;
>
> ifsd->loading_error = false;
> - ifsd->loaded_version = ifs_header_ptr->rev;
> +
> + if (ifsd->generation > 0)
> + return copy_hashes_authenticate_chunks_gen2(dev);
>
> /* copy the scan hash and authenticate per package */
> cpus_read_lock();
> @@ -226,6 +334,7 @@ static int scan_chunks_sanity_check(struct device *dev)
> ifs_pkg_auth[curr_pkg] = 1;
> }
> ret = 0;
> + ifsd->loaded_version = ifs_header_ptr->rev;
> out:
> cpus_read_unlock();
>
>
--
i.
Powered by blists - more mailing lists