diff mbox series

[09/14] platform/x86/intel/ifs: Use generic microcode headers and functions

Message ID 20221021203413.1220137-10-jithu.joseph@intel.com (mailing list archive)
State Changes Requested, archived
Headers show
Series IFS multi test image support and misc changes | expand

Commit Message

Joseph, Jithu Oct. 21, 2022, 8:34 p.m. UTC 
Existing implementation (broken) of IFS used a header format (for IFS
test images) which was very similar to microcode format, but didn’t
accommodate extended signatures. This meant same IFS test image had to
be duplicated for different steppings and the validation code in the
driver was only looking at the primary header parameters. Going forward
IFS test image headers has been tweaked to become fully compatible with
microcode format.

Newer IFS test image headers will use  microcode_header_intel->hdrver = 2,
so as to distinguish it from microcode images and older IFS test images.

In light of the above, use struct microcode_header_intel directly in
IFS driver instead of duplicating into a separate ifs_header structure.
Further use existing microcode_sanity_check() and find_matching_signature()
directly instead of implementing separate ones within the driver.

More IFS specific checks will be added subsequently.

Reviewed-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>
---
 drivers/platform/x86/intel/ifs/load.c | 102 +++++---------------------
 1 file changed, 19 insertions(+), 83 deletions(-)

Comments

Sohil Mehta Nov. 1, 2022, 6:37 p.m. UTC | #1 
On 10/21/2022 1:34 PM, Jithu Joseph wrote:
> Newer IFS test image headers will use  microcode_header_intel->hdrver = 2,
> so as to distinguish it from microcode images and older IFS test images.
> 

IIUC, older IFS test images would no longer be supported. Have they been 
released publicly?

What would happen if someone tries to load one? I am guessing one of the 
error checks would catch it. It might be useful to describe this error 
signature in the commit message.

>   
> -	if ((data_size + MC_HEADER_SIZE > total_size) || (total_size % sizeof(u32))) {
> -		dev_err(dev, "bad ifs data file size.\n");
> +	if (data->hdrver != IFS_HEADER_VER) {
> +		dev_err(dev, "Header version %d not supported\n", data->hdrver);
>   		return -EINVAL;
>   	}
>   
> -	if (mc_header->ldrver != 1 || mc_header->hdrver != 1) {
> -		dev_err(dev, "invalid/unknown ifs update format.\n");
> +	if (microcode_intel_sanity_check((void *)data, true, IFS_HEADER_VER)) {

I referred to this in a another patch. The data->hdrver is already 
verified above, why is there a need to pass it as a parameter as well.

> +		dev_err(dev, "sanity check failed\n");
>   		return -EINVAL;
>   	}
>   
> -	mc = (u32 *)mc_header;
> -	sum = 0;
> -	for (int i = 0; i < total_size / sizeof(u32); i++)
> -		sum += mc[i];
> +	intel_cpu_collect_info(&uci);
>   
> -	if (sum) {
> -		dev_err(dev, "bad ifs data checksum, aborting.\n");
> +	if (!microcode_intel_find_matching_signature((void *)data,
> +						     uci.cpu_sig.sig,
> +						     uci.cpu_sig.pf)) {
> +		dev_err(dev, "cpu signature, pf not matching\n");

What does pf stand for? It would be good to avoid abbreviations for 
error logging.


>   /*
>    * Load ifs image. Before loading ifs module, the ifs image must be located
>    * in /lib/firmware/intel/ifs and named as {family/model/stepping}.{testname}.
> @@ -252,12 +189,11 @@ int ifs_load_firmware(struct device *dev)
>   		goto done;
>   	}
>   
> -	if (!ifs_image_sanity_check(dev, (struct microcode_header_intel *)fw->data)) {
> -		dev_err(dev, "ifs header sanity check failed\n");
> +	ret = ifs_image_sanity_check(dev, (struct microcode_header_intel *)fw->data);
> +	if (ret)
>   		goto release;
> -	}
>   
> -	ifs_header_ptr = (struct ifs_header *)fw->data;
> +	ifs_header_ptr = (struct microcode_header_intel *)fw->data;

The use of a global ifs_header_ptr seems problematic. The semaphore 
operation before calling ifs_load_firmware() makes it seem concurrency 
is expected. Can ifs_load_firmware() really be called concurrently?

If that is not true can we use a mutex for synchronization?

Sohil
Joseph, Jithu Nov. 1, 2022, 9:07 p.m. UTC | #2 
On 11/1/2022 11:37 AM, Sohil Mehta wrote:
> On 10/21/2022 1:34 PM, Jithu Joseph wrote:
>> Newer IFS test image headers will use  microcode_header_intel->hdrver = 2,
>> so as to distinguish it from microcode images and older IFS test images.
>>
> 
> IIUC, older IFS test images would no longer be supported. Have they been released publicly?

This is true. The modified driver would need compatible images. It has not been widely release (note that the driver is under CONFIG_BROKEN today)

> 
> What would happen if someone tries to load one? I am guessing one of the error checks would catch it. It might be useful to describe this error signature in the commit message.

This will be caught by the sanity_check()

	if (microcode_intel_sanity_check((void *)data, true, IFS_HEADER_VER)) {
		dev_err(dev, "sanity check failed\n");
		return -EINVAL;
	}

Further the version mismatch dev_err from microcode_intel_sanity_check() would also be visible

> 
>>   -    if ((data_size + MC_HEADER_SIZE > total_size) || (total_size % sizeof(u32))) {
>> -        dev_err(dev, "bad ifs data file size.\n");
>> +    if (data->hdrver != IFS_HEADER_VER) {
>> +        dev_err(dev, "Header version %d not supported\n", data->hdrver);
>>           return -EINVAL;
>>       }
>>   -    if (mc_header->ldrver != 1 || mc_header->hdrver != 1) {
>> -        dev_err(dev, "invalid/unknown ifs update format.\n");
>> +    if (microcode_intel_sanity_check((void *)data, true, IFS_HEADER_VER)) {
> 
> I referred to this in a another patch. The data->hdrver is already verified above, why is there a need to pass it as a parameter as well.

Yes,  I noted the rationale in my response

> 
>> +        dev_err(dev, "sanity check failed\n");
>>           return -EINVAL;
>>       }
>>   -    mc = (u32 *)mc_header;
>> -    sum = 0;
>> -    for (int i = 0; i < total_size / sizeof(u32); i++)
>> -        sum += mc[i];
>> +    intel_cpu_collect_info(&uci);
>>   -    if (sum) {
>> -        dev_err(dev, "bad ifs data checksum, aborting.\n");
>> +    if (!microcode_intel_find_matching_signature((void *)data,
>> +                             uci.cpu_sig.sig,
>> +                             uci.cpu_sig.pf)) {
>> +        dev_err(dev, "cpu signature, pf not matching\n");
> 
> What does pf stand for? It would be good to avoid abbreviations for error logging.
> 

intel_cpu_collect_info() comments call it as "processor flags from MSR 0x17" ... I will 
expand "pf" to "processor flags" in the above message

> 
>>   /*
>>    * Load ifs image. Before loading ifs module, the ifs image must be located
>>    * in /lib/firmware/intel/ifs and named as {family/model/stepping}.{testname}.
>> @@ -252,12 +189,11 @@ int ifs_load_firmware(struct device *dev)
>>           goto done;
>>       }
>>   -    if (!ifs_image_sanity_check(dev, (struct microcode_header_intel *)fw->data)) {
>> -        dev_err(dev, "ifs header sanity check failed\n");
>> +    ret = ifs_image_sanity_check(dev, (struct microcode_header_intel *)fw->data);
>> +    if (ret)
>>           goto release;
>> -    }
>>   -    ifs_header_ptr = (struct ifs_header *)fw->data;
>> +    ifs_header_ptr = (struct microcode_header_intel *)fw->data;
> 
> The use of a global ifs_header_ptr seems problematic. The semaphore operation before calling ifs_load_firmware() makes it seem concurrency is expected. Can ifs_load_firmware() really be called concurrently?

Multiple simultaneous loads or simultaneous loads and run_tests should not be allowed from IFS device standpoint

Synchronization in the form of "down_interruptible(&ifs_sem)" is in place at the sysfs entry points (run_test_store() and current_batch_store()). If that was not present there is nothing preventing multiple loads (current_batch_store()) or runtests from being called concurrently.


> 
> If that is not true can we use a mutex for synchronization?

Since we are using the semaphore initalized to 1 (using DEFINE_SEMAPHORE), I believe it can be replaced by mutex APIs (mutex_lock_interruptible() in place of aforementioned down_interruptible()) . However I feel this change can be taken up separately as current series doesn't introduce any synchronization mechanisms and the existing locking in place seems sufficient for the synchronization needs of the device and its operations.


Jithu
diff mbox series

Patch

diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
index b88db0765311..3cb13a7aa74b 100644
--- a/drivers/platform/x86/intel/ifs/load.c
+++ b/drivers/platform/x86/intel/ifs/load.c
@@ -8,22 +8,9 @@ 
 
 #include "ifs.h"
 
-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 */
+#define IFS_HEADER_SIZE	(sizeof(struct microcode_header_intel))
+#define IFS_HEADER_VER	2
+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 */
 static DECLARE_COMPLETION(ifs_done);
@@ -118,33 +105,18 @@  static void copy_hashes_authenticate_chunks(struct work_struct *work)
  */
 static int scan_chunks_sanity_check(struct device *dev)
 {
-	int metadata_size, curr_pkg, cpu, ret = -ENOMEM;
 	struct ifs_data *ifsd = ifs_get_data(dev);
+	int curr_pkg, cpu, ret = -ENOMEM;
 	bool *package_authenticated;
 	struct ifs_work local_work;
-	char *test_ptr;
 
 	package_authenticated = kcalloc(topology_max_packages(), sizeof(bool), GFP_KERNEL);
 	if (!package_authenticated)
 		return ret;
 
-	metadata_size = ifs_header_ptr->metadata_size;
 
-	/* Spec says that if the Meta Data Size = 0 then it should be treated as 2000 */
-	if (metadata_size == 0)
-		metadata_size = 2000;
-
-	/* Scan chunk start must be 256 byte aligned */
-	if ((metadata_size + IFS_HEADER_SIZE) % 256) {
-		dev_err(dev, "Scan pattern offset within the binary is not 256 byte aligned\n");
-		return -EINVAL;
-	}
-
-	test_ptr = (char *)ifs_header_ptr + IFS_HEADER_SIZE + metadata_size;
 	ifsd->loading_error = false;
-
-	ifs_test_image_ptr = (u64)test_ptr;
-	ifsd->loaded_version = ifs_header_ptr->blob_revision;
+	ifsd->loaded_version = ifs_header_ptr->rev;
 
 	/* copy the scan hash and authenticate per package */
 	cpus_read_lock();
@@ -171,67 +143,32 @@  static int scan_chunks_sanity_check(struct device *dev)
 	return ret;
 }
 
-static int ifs_sanity_check(struct device *dev,
-			    const struct microcode_header_intel *mc_header)
+static int ifs_image_sanity_check(struct device *dev, const struct microcode_header_intel *data)
 {
-	unsigned long total_size, data_size;
-	u32 sum, *mc;
-
-	total_size = get_totalsize(mc_header);
-	data_size = get_datasize(mc_header);
+	struct ucode_cpu_info uci;
 
-	if ((data_size + MC_HEADER_SIZE > total_size) || (total_size % sizeof(u32))) {
-		dev_err(dev, "bad ifs data file size.\n");
+	if (data->hdrver != IFS_HEADER_VER) {
+		dev_err(dev, "Header version %d not supported\n", data->hdrver);
 		return -EINVAL;
 	}
 
-	if (mc_header->ldrver != 1 || mc_header->hdrver != 1) {
-		dev_err(dev, "invalid/unknown ifs update format.\n");
+	if (microcode_intel_sanity_check((void *)data, true, IFS_HEADER_VER)) {
+		dev_err(dev, "sanity check failed\n");
 		return -EINVAL;
 	}
 
-	mc = (u32 *)mc_header;
-	sum = 0;
-	for (int i = 0; i < total_size / sizeof(u32); i++)
-		sum += mc[i];
+	intel_cpu_collect_info(&uci);
 
-	if (sum) {
-		dev_err(dev, "bad ifs data checksum, aborting.\n");
+	if (!microcode_intel_find_matching_signature((void *)data,
+						     uci.cpu_sig.sig,
+						     uci.cpu_sig.pf)) {
+		dev_err(dev, "cpu signature, pf not matching\n");
 		return -EINVAL;
 	}
 
 	return 0;
 }
 
-static bool find_ifs_matching_signature(struct device *dev, struct ucode_cpu_info *uci,
-					const struct microcode_header_intel *shdr)
-{
-	unsigned int mc_size;
-
-	mc_size = get_totalsize(shdr);
-
-	if (!mc_size || ifs_sanity_check(dev, shdr) < 0) {
-		dev_err(dev, "ifs sanity check failure\n");
-		return false;
-	}
-
-	if (!intel_cpu_signatures_match(uci->cpu_sig.sig, uci->cpu_sig.pf, shdr->sig, shdr->pf)) {
-		dev_err(dev, "ifs signature, pf not matching\n");
-		return false;
-	}
-
-	return true;
-}
-
-static bool ifs_image_sanity_check(struct device *dev, const struct microcode_header_intel *data)
-{
-	struct ucode_cpu_info uci;
-
-	intel_cpu_collect_info(&uci);
-
-	return find_ifs_matching_signature(dev, &uci, data);
-}
-
 /*
  * Load ifs image. Before loading ifs module, the ifs image must be located
  * in /lib/firmware/intel/ifs and named as {family/model/stepping}.{testname}.
@@ -252,12 +189,11 @@  int ifs_load_firmware(struct device *dev)
 		goto done;
 	}
 
-	if (!ifs_image_sanity_check(dev, (struct microcode_header_intel *)fw->data)) {
-		dev_err(dev, "ifs header sanity check failed\n");
+	ret = ifs_image_sanity_check(dev, (struct microcode_header_intel *)fw->data);
+	if (ret)
 		goto release;
-	}
 
-	ifs_header_ptr = (struct ifs_header *)fw->data;
+	ifs_header_ptr = (struct microcode_header_intel *)fw->data;
 	ifs_hash_ptr = (u64)(ifs_header_ptr + 1);
 
 	ret = scan_chunks_sanity_check(dev);