[PATCH v2] platform/x86/intel/ifs: Allow non-default names for IFS image

[PATCH v2] platform/x86/intel/ifs: Allow non-default names for IFS image

[Jithu Joseph]

Existing implementation limits IFS images to be loaded only from
a default file-name /lib/firmware/intel/ifs/ff-mm-ss.scan.

But there are situations where there may be multiple scan files
that can be run on a particular system stored in /lib/firmware/intel/ifs

E.g.
1. Because test contents are larger than the memory reserved for IFS by BIOS
2. To provide increased test coverage
3. Custom test files to debug certain specific issues in the field

Renaming each of these to ff-mm-ss.scan and then loading might be
possible in some environments. But on systems where /lib is read-only
this is not a practical solution.

Modify the semantics of the driver file
/sys/devices/virtual/misc/intel_ifs_0/reload such that,
it interprets the input as the filename to be loaded.

Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>
---
Changes in v2
- drop treating "1" specially, i.e treat everything as a file-name

 drivers/platform/x86/intel/ifs/ifs.h          | 11 ++++----
 drivers/platform/x86/intel/ifs/core.c         |  2 +-
 drivers/platform/x86/intel/ifs/load.c         | 25 +++++++++++++------
 drivers/platform/x86/intel/ifs/sysfs.c        | 13 +++-------
 .../ABI/testing/sysfs-platform-intel-ifs      |  3 +--
 5 files changed, 29 insertions(+), 25 deletions(-)

diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
index 73c8e91cf144..577cee7db86a 100644
--- a/drivers/platform/x86/intel/ifs/ifs.h
+++ b/drivers/platform/x86/intel/ifs/ifs.h
@@ -34,12 +34,13 @@
  * socket in a two step process using writes to MSRs to first load the
  * SHA hashes for the test. Then the tests themselves. Status MSRs provide
  * feedback on the success/failure of these steps. When a new test file
- * is installed it can be loaded by writing to the driver reload file::
+ * is installed it can be loaded by writing the filename to the driver reload file::
  *
- *   # echo 1 > /sys/devices/virtual/misc/intel_ifs_0/reload
+ *   # echo mytest > /sys/devices/virtual/misc/intel_ifs_0/reload
  *
- * Similar to microcode, the current version of the scan tests is stored
- * in a fixed location: /lib/firmware/intel/ifs.0/family-model-stepping.scan
+ * The file will be loaded from /lib/firmware/intel/ifs/mytest
+ * The default file /lib/firmware/intel/ifs/family-model-stepping.scan
+ * will be loaded during module insertion.
  *
  * Running tests
  * -------------
@@ -225,7 +226,7 @@ static inline struct ifs_data *ifs_get_data(struct device *dev)
 	return &d->data;
 }
 
-void ifs_load_firmware(struct device *dev);
+int ifs_load_firmware(struct device *dev, const char *file_name);
 int do_core_test(int cpu, struct device *dev);
 const struct attribute_group **ifs_get_groups(void);
 
diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
index 27204e3d674d..9c319ada62d8 100644
--- a/drivers/platform/x86/intel/ifs/core.c
+++ b/drivers/platform/x86/intel/ifs/core.c
@@ -53,7 +53,7 @@ static int __init ifs_init(void)
 	if ((msrval & BIT(ifs_device.data.integrity_cap_bit)) &&
 	    !misc_register(&ifs_device.misc)) {
 		down(&ifs_sem);
-		ifs_load_firmware(ifs_device.misc.this_device);
+		ifs_load_firmware(ifs_device.misc.this_device, NULL);
 		up(&ifs_sem);
 		return 0;
 	}
diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
index d056617ddc85..89d76bd8b40a 100644
--- a/drivers/platform/x86/intel/ifs/load.c
+++ b/drivers/platform/x86/intel/ifs/load.c
@@ -232,17 +232,27 @@ static bool ifs_image_sanity_check(struct device *dev, const struct microcode_he
 
 /*
  * 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}.
+ * in the folder /lib/firmware/intel/ifs/
  */
-void ifs_load_firmware(struct device *dev)
+int ifs_load_firmware(struct device *dev, const char *file_name)
 {
 	struct ifs_data *ifsd = ifs_get_data(dev);
 	const struct firmware *fw;
-	char scan_path[32];
-	int ret;
-
-	snprintf(scan_path, sizeof(scan_path), "intel/ifs/%02x-%02x-%02x.scan",
-		 boot_cpu_data.x86, boot_cpu_data.x86_model, boot_cpu_data.x86_stepping);
+	char scan_path[64];
+	int ret = -EINVAL;
+	int file_name_len;
+
+	if (!file_name) {
+		snprintf(scan_path, sizeof(scan_path), "intel/ifs/%02x-%02x-%02x.scan",
+			 boot_cpu_data.x86, boot_cpu_data.x86_model, boot_cpu_data.x86_stepping);
+	} else {
+		if (strchr(file_name, '/'))
+			goto done;
+		file_name_len = strchrnul(file_name, '\n') - file_name;
+		if (snprintf(scan_path, sizeof(scan_path), "intel/ifs/%.*s",
+			     file_name_len, file_name) >= sizeof(scan_path))
+			goto done;
+	}
 
 	ret = request_firmware_direct(&fw, scan_path, dev);
 	if (ret) {
@@ -263,4 +273,5 @@ void ifs_load_firmware(struct device *dev)
 	release_firmware(fw);
 done:
 	ifsd->loaded = (ret == 0);
+	return ret;
 }
diff --git a/drivers/platform/x86/intel/ifs/sysfs.c b/drivers/platform/x86/intel/ifs/sysfs.c
index 37d8380d6fa8..b4716b7d36aa 100644
--- a/drivers/platform/x86/intel/ifs/sysfs.c
+++ b/drivers/platform/x86/intel/ifs/sysfs.c
@@ -94,23 +94,16 @@ static ssize_t reload_store(struct device *dev,
 			    struct device_attribute *attr,
 			    const char *buf, size_t count)
 {
-	struct ifs_data *ifsd = ifs_get_data(dev);
-	bool res;
-
-
-	if (kstrtobool(buf, &res))
-		return -EINVAL;
-	if (!res)
-		return count;
+	int ret;
 
 	if (down_interruptible(&ifs_sem))
 		return -EINTR;
 
-	ifs_load_firmware(dev);
+	ret = ifs_load_firmware(dev, buf);
 
 	up(&ifs_sem);
 
-	return ifsd->loaded ? count : -ENODEV;
+	return ret  ? ret : count;
 }
 
 static DEVICE_ATTR_WO(reload);
diff --git a/Documentation/ABI/testing/sysfs-platform-intel-ifs b/Documentation/ABI/testing/sysfs-platform-intel-ifs
index 486d6d2ff8a0..0b373f73a2b6 100644
--- a/Documentation/ABI/testing/sysfs-platform-intel-ifs
+++ b/Documentation/ABI/testing/sysfs-platform-intel-ifs
@@ -35,5 +35,4 @@ What:		/sys/devices/virtual/misc/intel_ifs_<N>/reload
 Date:		April 21 2022
 KernelVersion:	5.19
 Contact:	"Jithu Joseph" <jithu.joseph@intel.com>
-Description:	Write "1" (or "y" or "Y") to reload the IFS image from
-		/lib/firmware/intel/ifs/ff-mm-ss.scan.
+Description:	Write <file_name> to reload the IFS image from /lib/firmware/intel/<file_name>.

base-commit: 88084a3df1672e131ddc1b4e39eeacfd39864acf
-- 
2.25.1

Re: [PATCH v2] platform/x86/intel/ifs: Allow non-default names for IFS image

[Hans de Goede]

Hi,

On 7/10/22 18:00, Jithu Joseph wrote:
> Existing implementation limits IFS images to be loaded only from
> a default file-name /lib/firmware/intel/ifs/ff-mm-ss.scan.
> 
> But there are situations where there may be multiple scan files
> that can be run on a particular system stored in /lib/firmware/intel/ifs
> 
> E.g.
> 1. Because test contents are larger than the memory reserved for IFS by BIOS
> 2. To provide increased test coverage
> 3. Custom test files to debug certain specific issues in the field
> 
> Renaming each of these to ff-mm-ss.scan and then loading might be
> possible in some environments. But on systems where /lib is read-only
> this is not a practical solution.
> 
> Modify the semantics of the driver file
> /sys/devices/virtual/misc/intel_ifs_0/reload such that,
> it interprets the input as the filename to be loaded.

Much better, but I do wonder about the behavior of still loading
the default filename at module-init?   If there can be multiple
different "test-patterns" then does it not make more sense to
let the user always load the desired pattern before testing first?

Not doing the initial load at module-load time will also speed-up
the module initialization and thus booting the system. Especially
on many-core servers this might make a measurable difference
in module-init time.

Regards,

Hans



> 
> Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>
> ---
> Changes in v2
> - drop treating "1" specially, i.e treat everything as a file-name
> 
>  drivers/platform/x86/intel/ifs/ifs.h          | 11 ++++----
>  drivers/platform/x86/intel/ifs/core.c         |  2 +-
>  drivers/platform/x86/intel/ifs/load.c         | 25 +++++++++++++------
>  drivers/platform/x86/intel/ifs/sysfs.c        | 13 +++-------
>  .../ABI/testing/sysfs-platform-intel-ifs      |  3 +--
>  5 files changed, 29 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
> index 73c8e91cf144..577cee7db86a 100644
> --- a/drivers/platform/x86/intel/ifs/ifs.h
> +++ b/drivers/platform/x86/intel/ifs/ifs.h
> @@ -34,12 +34,13 @@
>   * socket in a two step process using writes to MSRs to first load the
>   * SHA hashes for the test. Then the tests themselves. Status MSRs provide
>   * feedback on the success/failure of these steps. When a new test file
> - * is installed it can be loaded by writing to the driver reload file::
> + * is installed it can be loaded by writing the filename to the driver reload file::
>   *
> - *   # echo 1 > /sys/devices/virtual/misc/intel_ifs_0/reload
> + *   # echo mytest > /sys/devices/virtual/misc/intel_ifs_0/reload
>   *
> - * Similar to microcode, the current version of the scan tests is stored
> - * in a fixed location: /lib/firmware/intel/ifs.0/family-model-stepping.scan
> + * The file will be loaded from /lib/firmware/intel/ifs/mytest
> + * The default file /lib/firmware/intel/ifs/family-model-stepping.scan
> + * will be loaded during module insertion.
>   *
>   * Running tests
>   * -------------
> @@ -225,7 +226,7 @@ static inline struct ifs_data *ifs_get_data(struct device *dev)
>  	return &d->data;
>  }
>  
> -void ifs_load_firmware(struct device *dev);
> +int ifs_load_firmware(struct device *dev, const char *file_name);
>  int do_core_test(int cpu, struct device *dev);
>  const struct attribute_group **ifs_get_groups(void);
>  
> diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c
> index 27204e3d674d..9c319ada62d8 100644
> --- a/drivers/platform/x86/intel/ifs/core.c
> +++ b/drivers/platform/x86/intel/ifs/core.c
> @@ -53,7 +53,7 @@ static int __init ifs_init(void)
>  	if ((msrval & BIT(ifs_device.data.integrity_cap_bit)) &&
>  	    !misc_register(&ifs_device.misc)) {
>  		down(&ifs_sem);
> -		ifs_load_firmware(ifs_device.misc.this_device);
> +		ifs_load_firmware(ifs_device.misc.this_device, NULL);
>  		up(&ifs_sem);
>  		return 0;
>  	}
> diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
> index d056617ddc85..89d76bd8b40a 100644
> --- a/drivers/platform/x86/intel/ifs/load.c
> +++ b/drivers/platform/x86/intel/ifs/load.c
> @@ -232,17 +232,27 @@ static bool ifs_image_sanity_check(struct device *dev, const struct microcode_he
>  
>  /*
>   * 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}.
> + * in the folder /lib/firmware/intel/ifs/
>   */
> -void ifs_load_firmware(struct device *dev)
> +int ifs_load_firmware(struct device *dev, const char *file_name)
>  {
>  	struct ifs_data *ifsd = ifs_get_data(dev);
>  	const struct firmware *fw;
> -	char scan_path[32];
> -	int ret;
> -
> -	snprintf(scan_path, sizeof(scan_path), "intel/ifs/%02x-%02x-%02x.scan",
> -		 boot_cpu_data.x86, boot_cpu_data.x86_model, boot_cpu_data.x86_stepping);
> +	char scan_path[64];
> +	int ret = -EINVAL;
> +	int file_name_len;
> +
> +	if (!file_name) {
> +		snprintf(scan_path, sizeof(scan_path), "intel/ifs/%02x-%02x-%02x.scan",
> +			 boot_cpu_data.x86, boot_cpu_data.x86_model, boot_cpu_data.x86_stepping);
> +	} else {
> +		if (strchr(file_name, '/'))
> +			goto done;
> +		file_name_len = strchrnul(file_name, '\n') - file_name;
> +		if (snprintf(scan_path, sizeof(scan_path), "intel/ifs/%.*s",
> +			     file_name_len, file_name) >= sizeof(scan_path))
> +			goto done;
> +	}
>  
>  	ret = request_firmware_direct(&fw, scan_path, dev);
>  	if (ret) {
> @@ -263,4 +273,5 @@ void ifs_load_firmware(struct device *dev)
>  	release_firmware(fw);
>  done:
>  	ifsd->loaded = (ret == 0);
> +	return ret;
>  }
> diff --git a/drivers/platform/x86/intel/ifs/sysfs.c b/drivers/platform/x86/intel/ifs/sysfs.c
> index 37d8380d6fa8..b4716b7d36aa 100644
> --- a/drivers/platform/x86/intel/ifs/sysfs.c
> +++ b/drivers/platform/x86/intel/ifs/sysfs.c
> @@ -94,23 +94,16 @@ static ssize_t reload_store(struct device *dev,
>  			    struct device_attribute *attr,
>  			    const char *buf, size_t count)
>  {
> -	struct ifs_data *ifsd = ifs_get_data(dev);
> -	bool res;
> -
> -
> -	if (kstrtobool(buf, &res))
> -		return -EINVAL;
> -	if (!res)
> -		return count;
> +	int ret;
>  
>  	if (down_interruptible(&ifs_sem))
>  		return -EINTR;
>  
> -	ifs_load_firmware(dev);
> +	ret = ifs_load_firmware(dev, buf);
>  
>  	up(&ifs_sem);
>  
> -	return ifsd->loaded ? count : -ENODEV;
> +	return ret  ? ret : count;
>  }
>  
>  static DEVICE_ATTR_WO(reload);
> diff --git a/Documentation/ABI/testing/sysfs-platform-intel-ifs b/Documentation/ABI/testing/sysfs-platform-intel-ifs
> index 486d6d2ff8a0..0b373f73a2b6 100644
> --- a/Documentation/ABI/testing/sysfs-platform-intel-ifs
> +++ b/Documentation/ABI/testing/sysfs-platform-intel-ifs
> @@ -35,5 +35,4 @@ What:		/sys/devices/virtual/misc/intel_ifs_<N>/reload
>  Date:		April 21 2022
>  KernelVersion:	5.19
>  Contact:	"Jithu Joseph" <jithu.joseph@intel.com>
> -Description:	Write "1" (or "y" or "Y") to reload the IFS image from
> -		/lib/firmware/intel/ifs/ff-mm-ss.scan.
> +Description:	Write <file_name> to reload the IFS image from /lib/firmware/intel/<file_name>.
> 
> base-commit: 88084a3df1672e131ddc1b4e39eeacfd39864acf

Re: [PATCH v2] platform/x86/intel/ifs: Allow non-default names for IFS image

[Joseph, Jithu]

On 7/10/2022 9:12 AM, Hans de Goede wrote:
> Hi,
> 
> On 7/10/22 18:00, Jithu Joseph wrote:
>> Existing implementation limits IFS images to be loaded only from
>> a default file-name /lib/firmware/intel/ifs/ff-mm-ss.scan.
>>
>> But there are situations where there may be multiple scan files
>> that can be run on a particular system stored in /lib/firmware/intel/ifs
>>
>> E.g.
>> 1. Because test contents are larger than the memory reserved for IFS by BIOS
>> 2. To provide increased test coverage
>> 3. Custom test files to debug certain specific issues in the field
>>
>> Renaming each of these to ff-mm-ss.scan and then loading might be
>> possible in some environments. But on systems where /lib is read-only
>> this is not a practical solution.
>>
>> Modify the semantics of the driver file
>> /sys/devices/virtual/misc/intel_ifs_0/reload such that,
>> it interprets the input as the filename to be loaded.
> 
> Much better, but I do wonder about the behavior of still loading
> the default filename at module-init?   If there can be multiple
> different "test-patterns" then does it not make more sense to
> let the user always load the desired pattern before testing first?

The default image provides bulk of the test coverage and the additional ones
provide marginal additional test coverage. That is why, we still kept
loading it by default

> 
> Not doing the initial load at module-load time will also speed-up
> the module initialization and thus booting the system. Especially
> on many-core servers this might make a measurable difference
> in module-init time.

Thanks for these inputs Hans, I do see your concern. Let me take these
concerns to internal folks, before I get back on this.

Jithu

Re: [PATCH v2] platform/x86/intel/ifs: Allow non-default names for IFS image

[Greg KH]

On Sun, Jul 10, 2022 at 09:00:11AM -0700, Jithu Joseph wrote:
> Existing implementation limits IFS images to be loaded only from
> a default file-name /lib/firmware/intel/ifs/ff-mm-ss.scan.

That was by design, why is this suddenly not acceptable?

> But there are situations where there may be multiple scan files
> that can be run on a particular system stored in /lib/firmware/intel/ifs

Again, this was by design.

> E.g.
> 1. Because test contents are larger than the memory reserved for IFS by BIOS

What does the BIOS have to do with this?

> 2. To provide increased test coverage

Test coverage of what?

> 3. Custom test files to debug certain specific issues in the field

Why can't you rename the existing file?

> Renaming each of these to ff-mm-ss.scan and then loading might be
> possible in some environments. But on systems where /lib is read-only
> this is not a practical solution.

What system puts /lib/ as read-only that you want to have loading
hardware firmware?  That kind of defeats the security implications of
having a read-only /lib/, right?

> Modify the semantics of the driver file
> /sys/devices/virtual/misc/intel_ifs_0/reload such that,
> it interprets the input as the filename to be loaded.

So you are now going to allow any file to be read from the system, in an
unknown filesystem namespace, by this driver?  How wise is that?  How
much has this been fuzzed and tested to ensure that it can not now be
instantly abused by anyone with access to this sysfs file?

Be aware that most systems have now locked down the ability for
userspace to pick filenames for stuff like this for good reason.  This
feels like a step backwards from those protections.  Are you _SURE_ you
want to do this?

> diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
> index 73c8e91cf144..577cee7db86a 100644
> --- a/drivers/platform/x86/intel/ifs/ifs.h
> +++ b/drivers/platform/x86/intel/ifs/ifs.h
> @@ -34,12 +34,13 @@
>   * socket in a two step process using writes to MSRs to first load the
>   * SHA hashes for the test. Then the tests themselves. Status MSRs provide
>   * feedback on the success/failure of these steps. When a new test file
> - * is installed it can be loaded by writing to the driver reload file::
> + * is installed it can be loaded by writing the filename to the driver reload file::
>   *
> - *   # echo 1 > /sys/devices/virtual/misc/intel_ifs_0/reload
> + *   # echo mytest > /sys/devices/virtual/misc/intel_ifs_0/reload
>   *
> - * Similar to microcode, the current version of the scan tests is stored
> - * in a fixed location: /lib/firmware/intel/ifs.0/family-model-stepping.scan
> + * The file will be loaded from /lib/firmware/intel/ifs/mytest
> + * The default file /lib/firmware/intel/ifs/family-model-stepping.scan
> + * will be loaded during module insertion.

So you have a default directory, what happened to your read-only /lib/ ?

This also does not properly document what the kernel code does.

{sigh}

Please do not rush this, it's not acceptable as-is at all.

> --- a/Documentation/ABI/testing/sysfs-platform-intel-ifs
> +++ b/Documentation/ABI/testing/sysfs-platform-intel-ifs
> @@ -35,5 +35,4 @@ What:		/sys/devices/virtual/misc/intel_ifs_<N>/reload
>  Date:		April 21 2022
>  KernelVersion:	5.19
>  Contact:	"Jithu Joseph" <jithu.joseph@intel.com>
> -Description:	Write "1" (or "y" or "Y") to reload the IFS image from
> -		/lib/firmware/intel/ifs/ff-mm-ss.scan.
> +Description:	Write <file_name> to reload the IFS image from /lib/firmware/intel/<file_name>.

"reload" should not be for specifying the filename, please use a sane
interface instead of this overloading logic.

greg k-h

Re: [PATCH v2] platform/x86/intel/ifs: Allow non-default names for IFS image

[Greg KH]

On Sun, Jul 10, 2022 at 09:00:11AM -0700, Jithu Joseph wrote:
> Existing implementation limits IFS images to be loaded only from
> a default file-name /lib/firmware/intel/ifs/ff-mm-ss.scan.
> 
> But there are situations where there may be multiple scan files
> that can be run on a particular system stored in /lib/firmware/intel/ifs
> 
> E.g.
> 1. Because test contents are larger than the memory reserved for IFS by BIOS
> 2. To provide increased test coverage
> 3. Custom test files to debug certain specific issues in the field
> 
> Renaming each of these to ff-mm-ss.scan and then loading might be
> possible in some environments. But on systems where /lib is read-only
> this is not a practical solution.
> 
> Modify the semantics of the driver file
> /sys/devices/virtual/misc/intel_ifs_0/reload such that,
> it interprets the input as the filename to be loaded.
> 
> Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>
> ---

Also note that this does not follow the required Intel rules for
submitting kernel patches.  Why the rush and circumventing of the normal
process that we have in place for good reasons?

greg k-h

Re: [PATCH v2] platform/x86/intel/ifs: Allow non-default names for IFS image

[Luck, Tony]

On Sun, Jul 10, 2022 at 06:12:44PM +0200, Hans de Goede wrote:
> Not doing the initial load at module-load time will also speed-up
> the module initialization and thus booting the system. Especially
> on many-core servers this might make a measurable difference
> in module-init time.

The load is per-socket, not per core. So "many-core" doesn't matter
as much as number of sockets.

-Tony

Re: [PATCH v2] platform/x86/intel/ifs: Allow non-default names for IFS image

[Hans de Goede]

Hi,

On 7/10/22 18:59, Greg KH wrote:
> On Sun, Jul 10, 2022 at 09:00:11AM -0700, Jithu Joseph wrote:
>> Existing implementation limits IFS images to be loaded only from
>> a default file-name /lib/firmware/intel/ifs/ff-mm-ss.scan.
> 
> That was by design, why is this suddenly not acceptable?
> 
>> But there are situations where there may be multiple scan files
>> that can be run on a particular system stored in /lib/firmware/intel/ifs
> 
> Again, this was by design.
> 
>> E.g.
>> 1. Because test contents are larger than the memory reserved for IFS by BIOS
> 
> What does the BIOS have to do with this?
> 
>> 2. To provide increased test coverage
> 
> Test coverage of what?
> 
>> 3. Custom test files to debug certain specific issues in the field
> 
> Why can't you rename the existing file?
> 
>> Renaming each of these to ff-mm-ss.scan and then loading might be
>> possible in some environments. But on systems where /lib is read-only
>> this is not a practical solution.
> 
> What system puts /lib/ as read-only that you want to have loading
> hardware firmware?  That kind of defeats the security implications of
> having a read-only /lib/, right?
> 
>> Modify the semantics of the driver file
>> /sys/devices/virtual/misc/intel_ifs_0/reload such that,
>> it interprets the input as the filename to be loaded.
> 
> So you are now going to allow any file to be read from the system, in an
> unknown filesystem namespace, by this driver?

@Intel folks to me this is the big blocker which needs to be solved before
we can move forward with figuring out how to allow loading multiple
different sets of test patterns through IFS.

Which in turn is required to remove the broken marking...

How about echoing a suffix to be appended to the default filename to
the reload attribute? This suffix would then need to be sanity checked
to only contain [a-z][0-9] (we don't want '/' but it seems better to
limit this further) to avoid directory traversal attacks. 

(and echoing an empty suffix can be used to force reloading the
default test-patterns after a linux-firmware pkg upgrade)

This way we avoid the allowing user to load an arbitrary file issue.

Greg, would using a suffix appended to the default filename be
acceptable to you as a solution to solving the load arbitrary
file issue?

Also as Greg has mentioned for the next version the commit message
+ ABI docs need to a better job of explaining why support for more
then one set of test patterns per CPU model is necessary.

Since there has been no progress on this I'm going to drop this
as well as the "[PATCH 0/2] Two fixes for IFS" patch series from
my patch queue. Lets continue discussing the userspace API issue
in this thread and then once it is solved please post a v3 patch/series
addressing all the issues.

Regards,

Hans




> How wise is that?  How
> much has this been fuzzed and tested to ensure that it can not now be
> instantly abused by anyone with access to this sysfs file?
> 
> Be aware that most systems have now locked down the ability for
> userspace to pick filenames for stuff like this for good reason.  This
> feels like a step backwards from those protections.  Are you _SURE_ you
> want to do this?
> 
>> diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
>> index 73c8e91cf144..577cee7db86a 100644
>> --- a/drivers/platform/x86/intel/ifs/ifs.h
>> +++ b/drivers/platform/x86/intel/ifs/ifs.h
>> @@ -34,12 +34,13 @@
>>   * socket in a two step process using writes to MSRs to first load the
>>   * SHA hashes for the test. Then the tests themselves. Status MSRs provide
>>   * feedback on the success/failure of these steps. When a new test file
>> - * is installed it can be loaded by writing to the driver reload file::
>> + * is installed it can be loaded by writing the filename to the driver reload file::
>>   *
>> - *   # echo 1 > /sys/devices/virtual/misc/intel_ifs_0/reload
>> + *   # echo mytest > /sys/devices/virtual/misc/intel_ifs_0/reload
>>   *
>> - * Similar to microcode, the current version of the scan tests is stored
>> - * in a fixed location: /lib/firmware/intel/ifs.0/family-model-stepping.scan
>> + * The file will be loaded from /lib/firmware/intel/ifs/mytest
>> + * The default file /lib/firmware/intel/ifs/family-model-stepping.scan
>> + * will be loaded during module insertion.
> 
> So you have a default directory, what happened to your read-only /lib/ ?
> 
> This also does not properly document what the kernel code does.
> 
> {sigh}
> 
> Please do not rush this, it's not acceptable as-is at all.
> 
>> --- a/Documentation/ABI/testing/sysfs-platform-intel-ifs
>> +++ b/Documentation/ABI/testing/sysfs-platform-intel-ifs
>> @@ -35,5 +35,4 @@ What:		/sys/devices/virtual/misc/intel_ifs_<N>/reload
>>  Date:		April 21 2022
>>  KernelVersion:	5.19
>>  Contact:	"Jithu Joseph" <jithu.joseph@intel.com>
>> -Description:	Write "1" (or "y" or "Y") to reload the IFS image from
>> -		/lib/firmware/intel/ifs/ff-mm-ss.scan.
>> +Description:	Write <file_name> to reload the IFS image from /lib/firmware/intel/<file_name>.
> 
> "reload" should not be for specifying the filename, please use a sane
> interface instead of this overloading logic.
> 
> greg k-h
> 

Re: [PATCH v2] platform/x86/intel/ifs: Allow non-default names for IFS image

[Greg KH]

On Thu, Jul 28, 2022 at 01:57:25PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 7/10/22 18:59, Greg KH wrote:
> > On Sun, Jul 10, 2022 at 09:00:11AM -0700, Jithu Joseph wrote:
> >> Existing implementation limits IFS images to be loaded only from
> >> a default file-name /lib/firmware/intel/ifs/ff-mm-ss.scan.
> > 
> > That was by design, why is this suddenly not acceptable?
> > 
> >> But there are situations where there may be multiple scan files
> >> that can be run on a particular system stored in /lib/firmware/intel/ifs
> > 
> > Again, this was by design.
> > 
> >> E.g.
> >> 1. Because test contents are larger than the memory reserved for IFS by BIOS
> > 
> > What does the BIOS have to do with this?
> > 
> >> 2. To provide increased test coverage
> > 
> > Test coverage of what?
> > 
> >> 3. Custom test files to debug certain specific issues in the field
> > 
> > Why can't you rename the existing file?
> > 
> >> Renaming each of these to ff-mm-ss.scan and then loading might be
> >> possible in some environments. But on systems where /lib is read-only
> >> this is not a practical solution.
> > 
> > What system puts /lib/ as read-only that you want to have loading
> > hardware firmware?  That kind of defeats the security implications of
> > having a read-only /lib/, right?
> > 
> >> Modify the semantics of the driver file
> >> /sys/devices/virtual/misc/intel_ifs_0/reload such that,
> >> it interprets the input as the filename to be loaded.
> > 
> > So you are now going to allow any file to be read from the system, in an
> > unknown filesystem namespace, by this driver?
> 
> @Intel folks to me this is the big blocker which needs to be solved before
> we can move forward with figuring out how to allow loading multiple
> different sets of test patterns through IFS.
> 
> Which in turn is required to remove the broken marking...
> 
> How about echoing a suffix to be appended to the default filename to
> the reload attribute? This suffix would then need to be sanity checked
> to only contain [a-z][0-9] (we don't want '/' but it seems better to
> limit this further) to avoid directory traversal attacks. 
> 
> (and echoing an empty suffix can be used to force reloading the
> default test-patterns after a linux-firmware pkg upgrade)
> 
> This way we avoid the allowing user to load an arbitrary file issue.
> 
> Greg, would using a suffix appended to the default filename be
> acceptable to you as a solution to solving the load arbitrary
> file issue?

Not really, a suffix doesn't protect much at all.

This really feels like a "test the product in the factory" type of
option that someone seems to want to do without just renaming the
firmware file.  Why this is unique from all other firmware file loading
in the kernel needs to really be explained here in order to even
consider justifying this type of change.

thanks,

greg k-h

Re: [PATCH v2] platform/x86/intel/ifs: Allow non-default names for IFS image

[Hans de Goede]

Hi,

On 7/28/22 14:07, Greg KH wrote:
> On Thu, Jul 28, 2022 at 01:57:25PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 7/10/22 18:59, Greg KH wrote:
>>> On Sun, Jul 10, 2022 at 09:00:11AM -0700, Jithu Joseph wrote:
>>>> Existing implementation limits IFS images to be loaded only from
>>>> a default file-name /lib/firmware/intel/ifs/ff-mm-ss.scan.
>>>
>>> That was by design, why is this suddenly not acceptable?
>>>
>>>> But there are situations where there may be multiple scan files
>>>> that can be run on a particular system stored in /lib/firmware/intel/ifs
>>>
>>> Again, this was by design.
>>>
>>>> E.g.
>>>> 1. Because test contents are larger than the memory reserved for IFS by BIOS
>>>
>>> What does the BIOS have to do with this?
>>>
>>>> 2. To provide increased test coverage
>>>
>>> Test coverage of what?
>>>
>>>> 3. Custom test files to debug certain specific issues in the field
>>>
>>> Why can't you rename the existing file?
>>>
>>>> Renaming each of these to ff-mm-ss.scan and then loading might be
>>>> possible in some environments. But on systems where /lib is read-only
>>>> this is not a practical solution.
>>>
>>> What system puts /lib/ as read-only that you want to have loading
>>> hardware firmware?  That kind of defeats the security implications of
>>> having a read-only /lib/, right?
>>>
>>>> Modify the semantics of the driver file
>>>> /sys/devices/virtual/misc/intel_ifs_0/reload such that,
>>>> it interprets the input as the filename to be loaded.
>>>
>>> So you are now going to allow any file to be read from the system, in an
>>> unknown filesystem namespace, by this driver?
>>
>> @Intel folks to me this is the big blocker which needs to be solved before
>> we can move forward with figuring out how to allow loading multiple
>> different sets of test patterns through IFS.
>>
>> Which in turn is required to remove the broken marking...
>>
>> How about echoing a suffix to be appended to the default filename to
>> the reload attribute? This suffix would then need to be sanity checked
>> to only contain [a-z][0-9] (we don't want '/' but it seems better to
>> limit this further) to avoid directory traversal attacks. 
>>
>> (and echoing an empty suffix can be used to force reloading the
>> default test-patterns after a linux-firmware pkg upgrade)
>>
>> This way we avoid the allowing user to load an arbitrary file issue.
>>
>> Greg, would using a suffix appended to the default filename be
>> acceptable to you as a solution to solving the load arbitrary
>> file issue?
> 
> Not really, a suffix doesn't protect much at all.

?

Currently the driver will always load:

/lib/firmware/intel/ifs/%02x-%02x-%02x.scan

with the "%02x" bits being fixed parts of the CPU model.

My suggestion is to make it:

/lib/firmware/intel/ifs/%02x-%02x-%02x%s.scan

Where the "%s" bit can be supplied by userspace, but it may
only contain [a-z] and [0-9] so no '/' (or other special chars)
so that an attacker cannot use directory traversal to get a file
from below the /lib/firmware/intel/ifs/ dir.

This means that an attacker can only cause a bad file to be
loaded if they have write access to /lib/firmware/intel/ifs/  at
which point they can also just replace the default file. So I
don't see how just allowing userspace to just add a suffix
is a possible problem. Where as the previous arbitrary filename
approach obviously was a problem ?

> This really feels like a "test the product in the factory" type of
> option that someone seems to want to do without just renaming the
> firmware file.  Why this is unique from all other firmware file loading
> in the kernel needs to really be explained here in order to even
> consider justifying this type of change.

First of all I really wish some of the Intel folks would elaborate
more on why multiple test-pattern files are necessary. Ping
anyone@intel, you have all been very quiet in this thread which
is not helpful (not helpful at all really).

Speculating myself as far as I understand IFS is not for factory
tests but for testing in the fields since big cloud vendors have
found that sometimes there are hard to catch CPU defects which
they only find out by running statistics which show that certain
tasks only crash when run on machine a, socket b, core c.

IFS allows them to periodically run a set of CPU selftests to
detect CPU defects (which may sometimes also only show up
over time). AFAIK the multiple test-pattern files thing is
necessary now because it is not possible to put all
possible tests on a single file due to size constraints of
the special RAM into which the test-patterns are loaded.

So the default firmware file will contain a set of defaiult
tests. But there will also be e.g. a special file which
only exercises the avx512 parts of a core, but then more
thoroughly. Again this is all speculation from my side, but
this is my understanding of this all.

Regards,

Hans

Re: [PATCH v2] platform/x86/intel/ifs: Allow non-default names for IFS image

[Greg KH]

On Thu, Jul 28, 2022 at 02:52:06PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 7/28/22 14:07, Greg KH wrote:
> > On Thu, Jul 28, 2022 at 01:57:25PM +0200, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 7/10/22 18:59, Greg KH wrote:
> >>> On Sun, Jul 10, 2022 at 09:00:11AM -0700, Jithu Joseph wrote:
> >>>> Existing implementation limits IFS images to be loaded only from
> >>>> a default file-name /lib/firmware/intel/ifs/ff-mm-ss.scan.
> >>>
> >>> That was by design, why is this suddenly not acceptable?
> >>>
> >>>> But there are situations where there may be multiple scan files
> >>>> that can be run on a particular system stored in /lib/firmware/intel/ifs
> >>>
> >>> Again, this was by design.
> >>>
> >>>> E.g.
> >>>> 1. Because test contents are larger than the memory reserved for IFS by BIOS
> >>>
> >>> What does the BIOS have to do with this?
> >>>
> >>>> 2. To provide increased test coverage
> >>>
> >>> Test coverage of what?
> >>>
> >>>> 3. Custom test files to debug certain specific issues in the field
> >>>
> >>> Why can't you rename the existing file?
> >>>
> >>>> Renaming each of these to ff-mm-ss.scan and then loading might be
> >>>> possible in some environments. But on systems where /lib is read-only
> >>>> this is not a practical solution.
> >>>
> >>> What system puts /lib/ as read-only that you want to have loading
> >>> hardware firmware?  That kind of defeats the security implications of
> >>> having a read-only /lib/, right?
> >>>
> >>>> Modify the semantics of the driver file
> >>>> /sys/devices/virtual/misc/intel_ifs_0/reload such that,
> >>>> it interprets the input as the filename to be loaded.
> >>>
> >>> So you are now going to allow any file to be read from the system, in an
> >>> unknown filesystem namespace, by this driver?
> >>
> >> @Intel folks to me this is the big blocker which needs to be solved before
> >> we can move forward with figuring out how to allow loading multiple
> >> different sets of test patterns through IFS.
> >>
> >> Which in turn is required to remove the broken marking...
> >>
> >> How about echoing a suffix to be appended to the default filename to
> >> the reload attribute? This suffix would then need to be sanity checked
> >> to only contain [a-z][0-9] (we don't want '/' but it seems better to
> >> limit this further) to avoid directory traversal attacks. 
> >>
> >> (and echoing an empty suffix can be used to force reloading the
> >> default test-patterns after a linux-firmware pkg upgrade)
> >>
> >> This way we avoid the allowing user to load an arbitrary file issue.
> >>
> >> Greg, would using a suffix appended to the default filename be
> >> acceptable to you as a solution to solving the load arbitrary
> >> file issue?
> > 
> > Not really, a suffix doesn't protect much at all.
> 
> ?
> 
> Currently the driver will always load:
> 
> /lib/firmware/intel/ifs/%02x-%02x-%02x.scan
> 
> with the "%02x" bits being fixed parts of the CPU model.
> 
> My suggestion is to make it:
> 
> /lib/firmware/intel/ifs/%02x-%02x-%02x%s.scan

Ah, sorry, I skimmed that, you are right, that would be fine.  But still
odd to ever be needed in a real system.

> > This really feels like a "test the product in the factory" type of
> > option that someone seems to want to do without just renaming the
> > firmware file.  Why this is unique from all other firmware file loading
> > in the kernel needs to really be explained here in order to even
> > consider justifying this type of change.
> 
> First of all I really wish some of the Intel folks would elaborate
> more on why multiple test-pattern files are necessary. Ping
> anyone@intel, you have all been very quiet in this thread which
> is not helpful (not helpful at all really).
> 
> Speculating myself as far as I understand IFS is not for factory
> tests but for testing in the fields since big cloud vendors have
> found that sometimes there are hard to catch CPU defects which
> they only find out by running statistics which show that certain
> tasks only crash when run on machine a, socket b, core c.

Who knows, Intel doesn't say so we can't really guess :(

greg k-h

Re: [PATCH v2] platform/x86/intel/ifs: Allow non-default names for IFS image

[Hans de Goede]

Hi,

On 7/28/22 14:59, Greg KH wrote:
> On Thu, Jul 28, 2022 at 02:52:06PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 7/28/22 14:07, Greg KH wrote:
>>> On Thu, Jul 28, 2022 at 01:57:25PM +0200, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 7/10/22 18:59, Greg KH wrote:
>>>>> On Sun, Jul 10, 2022 at 09:00:11AM -0700, Jithu Joseph wrote:
>>>>>> Existing implementation limits IFS images to be loaded only from
>>>>>> a default file-name /lib/firmware/intel/ifs/ff-mm-ss.scan.
>>>>>
>>>>> That was by design, why is this suddenly not acceptable?
>>>>>
>>>>>> But there are situations where there may be multiple scan files
>>>>>> that can be run on a particular system stored in /lib/firmware/intel/ifs
>>>>>
>>>>> Again, this was by design.
>>>>>
>>>>>> E.g.
>>>>>> 1. Because test contents are larger than the memory reserved for IFS by BIOS
>>>>>
>>>>> What does the BIOS have to do with this?
>>>>>
>>>>>> 2. To provide increased test coverage
>>>>>
>>>>> Test coverage of what?
>>>>>
>>>>>> 3. Custom test files to debug certain specific issues in the field
>>>>>
>>>>> Why can't you rename the existing file?
>>>>>
>>>>>> Renaming each of these to ff-mm-ss.scan and then loading might be
>>>>>> possible in some environments. But on systems where /lib is read-only
>>>>>> this is not a practical solution.
>>>>>
>>>>> What system puts /lib/ as read-only that you want to have loading
>>>>> hardware firmware?  That kind of defeats the security implications of
>>>>> having a read-only /lib/, right?
>>>>>
>>>>>> Modify the semantics of the driver file
>>>>>> /sys/devices/virtual/misc/intel_ifs_0/reload such that,
>>>>>> it interprets the input as the filename to be loaded.
>>>>>
>>>>> So you are now going to allow any file to be read from the system, in an
>>>>> unknown filesystem namespace, by this driver?
>>>>
>>>> @Intel folks to me this is the big blocker which needs to be solved before
>>>> we can move forward with figuring out how to allow loading multiple
>>>> different sets of test patterns through IFS.
>>>>
>>>> Which in turn is required to remove the broken marking...
>>>>
>>>> How about echoing a suffix to be appended to the default filename to
>>>> the reload attribute? This suffix would then need to be sanity checked
>>>> to only contain [a-z][0-9] (we don't want '/' but it seems better to
>>>> limit this further) to avoid directory traversal attacks. 
>>>>
>>>> (and echoing an empty suffix can be used to force reloading the
>>>> default test-patterns after a linux-firmware pkg upgrade)
>>>>
>>>> This way we avoid the allowing user to load an arbitrary file issue.
>>>>
>>>> Greg, would using a suffix appended to the default filename be
>>>> acceptable to you as a solution to solving the load arbitrary
>>>> file issue?
>>>
>>> Not really, a suffix doesn't protect much at all.
>>
>> ?
>>
>> Currently the driver will always load:
>>
>> /lib/firmware/intel/ifs/%02x-%02x-%02x.scan
>>
>> with the "%02x" bits being fixed parts of the CPU model.
>>
>> My suggestion is to make it:
>>
>> /lib/firmware/intel/ifs/%02x-%02x-%02x%s.scan
> 
> Ah, sorry, I skimmed that, you are right, that would be fine.  But still
> odd to ever be needed in a real system.

Ok, good. So Intel folks this seems to be a way to move forward
with this. Please prepare a version 3 using this approach.

>>> This really feels like a "test the product in the factory" type of
>>> option that someone seems to want to do without just renaming the
>>> firmware file.  Why this is unique from all other firmware file loading
>>> in the kernel needs to really be explained here in order to even
>>> consider justifying this type of change.
>>
>> First of all I really wish some of the Intel folks would elaborate
>> more on why multiple test-pattern files are necessary. Ping
>> anyone@intel, you have all been very quiet in this thread which
>> is not helpful (not helpful at all really).
>>
>> Speculating myself as far as I understand IFS is not for factory
>> tests but for testing in the fields since big cloud vendors have
>> found that sometimes there are hard to catch CPU defects which
>> they only find out by running statistics which show that certain
>> tasks only crash when run on machine a, socket b, core c.
> 
> Who knows, Intel doesn't say so we can't really guess :(

Right, for version 3 the commit message and ABI documentation changes
really need to clarify why multiple test-pattern files may be needed
mucy better. If possible please also include 1 or 2 _clear_ examples
of cases where more then 1 test-pattern file may be used.

Regards,

Hans

RE: [PATCH v2] platform/x86/intel/ifs: Allow non-default names for IFS image

[Luck, Tony]

>>> Speculating myself as far as I understand IFS is not for factory
>>> tests but for testing in the fields since big cloud vendors have
>>> found that sometimes there are hard to catch CPU defects which
>>> they only find out by running statistics which show that certain
>>> tasks only crash when run on machine a, socket b, core c.
>>
>> Who knows, Intel doesn't say so we can't really guess :(
>
>Right, for version 3 the commit message and ABI documentation changes
>really need to clarify why multiple test-pattern files may be needed
>mucy better. If possible please also include 1 or 2 _clear_ examples
>of cases where more then 1 test-pattern file may be used.

Sorry for the radio silence. We took Greg's suggestion to go back and
thinks this out completely to heart. As he said, there is no rush to get
this in. We need to do it right.

Your summary above on how this works is completely correct.

The reason for adding more files is to cover more transistors in the
core. The base file that we started with gets mumble-mumble percent
of the transistors checked. Adding a few more files will increase that
quite significantly.

So testing a system may look like:

	for each scan file
	do
		load the scan file
		for each core
		do
			test the core with this set of tests
		done
	done

Our internal discussions on naming are following the same direction that
you suggested, but likely even more restrictive. The "suffix" may just be
a two-digit hex number (allowing for up to 256 files ... though for Sapphire
Rapids we are looking at just 6).

So our current direction is to name six "parts" something like this:

	06-8f-06-00.scan
	06-8f-06-01.scan
	06-8f-06-02.scan
	06-8f-06-03.scan
	06-8f-06-04.scan
	06-8f-06-05.scan

but we are still checking to make sure this will work for future CPUs. Once
we have something solid we will come back to the mailing list.

As also suggested in earlier thread we will change the name of the "reload"
file (since skipping to a new file isn't a "reload"). The "load a scan file" will
write the "part" number to this new file.

-Tony

Re: [PATCH v2] platform/x86/intel/ifs: Allow non-default names for IFS image

[Hans de Goede]

Hi Tony,

On 7/28/22 17:12, Luck, Tony wrote:
>>>> Speculating myself as far as I understand IFS is not for factory
>>>> tests but for testing in the fields since big cloud vendors have
>>>> found that sometimes there are hard to catch CPU defects which
>>>> they only find out by running statistics which show that certain
>>>> tasks only crash when run on machine a, socket b, core c.
>>>
>>> Who knows, Intel doesn't say so we can't really guess :(
>>
>> Right, for version 3 the commit message and ABI documentation changes
>> really need to clarify why multiple test-pattern files may be needed
>> mucy better. If possible please also include 1 or 2 _clear_ examples
>> of cases where more then 1 test-pattern file may be used.
> 
> Sorry for the radio silence. We took Greg's suggestion to go back and
> thinks this out completely to heart. As he said, there is no rush to get
> this in. We need to do it right.

That (taking your time to get this right) is good to hear, thanks.

> Your summary above on how this works is completely correct.
> 
> The reason for adding more files is to cover more transistors in the
> core. The base file that we started with gets mumble-mumble percent
> of the transistors checked. Adding a few more files will increase that
> quite significantly.
> 
> So testing a system may look like:
> 
> 	for each scan file
> 	do
> 		load the scan file
> 		for each core
> 		do
> 			test the core with this set of tests
> 		done
> 	done
> 
> Our internal discussions on naming are following the same direction that
> you suggested, but likely even more restrictive. The "suffix" may just be
> a two-digit hex number (allowing for up to 256 files ... though for Sapphire
> Rapids we are looking at just 6).
> 
> So our current direction is to name six "parts" something like this:
> 
> 	06-8f-06-00.scan
> 	06-8f-06-01.scan
> 	06-8f-06-02.scan
> 	06-8f-06-03.scan
> 	06-8f-06-04.scan
> 	06-8f-06-05.scan
> 
> but we are still checking to make sure this will work for future CPUs. Once
> we have something solid we will come back to the mailing list.

Thanks, this sounds good to me.

> As also suggested in earlier thread we will change the name of the "reload"
> file (since skipping to a new file isn't a "reload"). The "load a scan file" will
> write the "part" number to this new file.

Regards,

Hans