[PATCH] firmware: export x86_64 platform flash bios region via sysfs

[PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Hans-Gert Dahmen]

Make the 16MiB long memory-mapped BIOS region of the platform SPI flash
on X86_64 system available via /sys/kernel/firmware/flash_mmap/bios_region
for pen-testing, security analysis and malware detection on kernels
which restrict module loading and/or access to /dev/mem.

It will be used by the open source Converged Security Suite.
https://github.com/9elements/converged-security-suite

Signed-off-by: Hans-Gert Dahmen <hans-gert.dahmen@immu.ne>
---
 drivers/firmware/Kconfig             |  9 ++++
 drivers/firmware/Makefile            |  1 +
 drivers/firmware/x86_64_flash_mmap.c | 65 ++++++++++++++++++++++++++++
 3 files changed, 75 insertions(+)
 create mode 100644 drivers/firmware/x86_64_flash_mmap.c

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index db0ea2d2d75a..bd77ca2b4fa6 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -296,6 +296,15 @@ config TURRIS_MOX_RWTM
 	  other manufacturing data and also utilize the Entropy Bit Generator
 	  for hardware random number generation.
 
+config X86_64_FLASH_MMAP
+	tristate "Export platform flash memory-mapped BIOS region"
+	depends on X86_64
+	help
+	  Export the memory-mapped BIOS region of the platform SPI flash as
+	  a read-only sysfs binary attribute on X86_64 systems. The first 16MiB
+	  will be accessible via /sys/kernel/firmware/flash_mmap/bios_region
+	  for security and malware analysis for example.
+
 source "drivers/firmware/broadcom/Kconfig"
 source "drivers/firmware/google/Kconfig"
 source "drivers/firmware/efi/Kconfig"
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 5e013b6a3692..eb7483c5a2ac 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_QCOM_SCM)		+= qcom_scm.o qcom_scm-smc.o qcom_scm-legacy.o
 obj-$(CONFIG_TI_SCI_PROTOCOL)	+= ti_sci.o
 obj-$(CONFIG_TRUSTED_FOUNDATIONS) += trusted_foundations.o
 obj-$(CONFIG_TURRIS_MOX_RWTM)	+= turris-mox-rwtm.o
+obj-$(CONFIG_X86_64_FLASH_MMAP)	+= x86_64_flash_mmap.o
 
 obj-y				+= arm_scmi/
 obj-y				+= broadcom/
diff --git a/drivers/firmware/x86_64_flash_mmap.c b/drivers/firmware/x86_64_flash_mmap.c
new file mode 100644
index 000000000000..f9d871a8b516
--- /dev/null
+++ b/drivers/firmware/x86_64_flash_mmap.c
@@ -0,0 +1,65 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Export the memory-mapped BIOS region of the platform SPI flash as
+ * a read-only sysfs binary attribute on X86_64 systems.
+ *
+ * Copyright © 2021 immune GmbH
+ */
+
+#include <linux/version.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/sysfs.h>
+#include <linux/kobject.h>
+
+#define FLASH_REGION_START 0xFF000000ULL
+#define FLASH_REGION_SIZE 0x1000000ULL
+#define FLASH_REGION_MASK (FLASH_REGION_SIZE - 1)
+
+struct kobject *kobj_ref;
+
+static ssize_t bios_region_read(struct file *file, struct kobject *kobj,
+				struct bin_attribute *bin_attr, char *buffer,
+				loff_t offset, size_t count)
+{
+	resource_size_t pa = FLASH_REGION_START + (offset & FLASH_REGION_MASK);
+	void __iomem *va = ioremap(pa, PAGE_SIZE);
+
+	memcpy_fromio(buffer, va, PAGE_SIZE);
+	iounmap(va);
+
+	return min(count, PAGE_SIZE);
+}
+
+BIN_ATTR_RO(bios_region, FLASH_REGION_SIZE);
+
+static int __init flash_mmap_init(void)
+{
+	int ret = 0;
+
+	kobj_ref = kobject_create_and_add("flash_mmap", firmware_kobj);
+	ret = sysfs_create_bin_file(kobj_ref, &bin_attr_bios_region);
+	if (ret) {
+		pr_err("sysfs_create_bin_file failed\n");
+		goto error;
+	}
+
+	return ret;
+
+error:
+	kobject_put(kobj_ref);
+	return ret;
+}
+
+static void __exit flash_mmap_exit(void)
+{
+	sysfs_remove_bin_file(kernel_kobj, &bin_attr_bios_region);
+	kobject_put(kobj_ref);
+}
+
+module_init(flash_mmap_init);
+module_exit(flash_mmap_exit);
+MODULE_DESCRIPTION("Export SPI platform flash memory mapped region via sysfs");
+MODULE_AUTHOR("Hans-Gert Dahmen <hans-gert.dahmen@immu.ne>");
+MODULE_LICENSE("GPL");
-- 
2.30.2

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Greg KH]

On Tue, Jun 22, 2021 at 04:23:34PM +0200, Hans-Gert Dahmen wrote:
> Make the 16MiB long memory-mapped BIOS region of the platform SPI flash
> on X86_64 system available via /sys/kernel/firmware/flash_mmap/bios_region
> for pen-testing, security analysis and malware detection on kernels
> which restrict module loading and/or access to /dev/mem.
> 
> It will be used by the open source Converged Security Suite.
> https://github.com/9elements/converged-security-suite
> 
> Signed-off-by: Hans-Gert Dahmen <hans-gert.dahmen@immu.ne>
> ---
>  drivers/firmware/Kconfig             |  9 ++++
>  drivers/firmware/Makefile            |  1 +
>  drivers/firmware/x86_64_flash_mmap.c | 65 ++++++++++++++++++++++++++++
>  3 files changed, 75 insertions(+)
>  create mode 100644 drivers/firmware/x86_64_flash_mmap.c
> 
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index db0ea2d2d75a..bd77ca2b4fa6 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -296,6 +296,15 @@ config TURRIS_MOX_RWTM
>  	  other manufacturing data and also utilize the Entropy Bit Generator
>  	  for hardware random number generation.
>  
> +config X86_64_FLASH_MMAP
> +	tristate "Export platform flash memory-mapped BIOS region"
> +	depends on X86_64
> +	help
> +	  Export the memory-mapped BIOS region of the platform SPI flash as
> +	  a read-only sysfs binary attribute on X86_64 systems. The first 16MiB
> +	  will be accessible via /sys/kernel/firmware/flash_mmap/bios_region
> +	  for security and malware analysis for example.

Module name information here?

Can this be auto-loaded based on hardware-specific values somewhere?
Otherwise it just looks like if this module loads, it will "always
work"?

And why would you want to map the bios into userspace?

What bios, UEFI?

And you need a Documentation/ABI/ update for new sysfs files.


> +
>  source "drivers/firmware/broadcom/Kconfig"
>  source "drivers/firmware/google/Kconfig"
>  source "drivers/firmware/efi/Kconfig"
> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
> index 5e013b6a3692..eb7483c5a2ac 100644
> --- a/drivers/firmware/Makefile
> +++ b/drivers/firmware/Makefile
> @@ -21,6 +21,7 @@ obj-$(CONFIG_QCOM_SCM)		+= qcom_scm.o qcom_scm-smc.o qcom_scm-legacy.o
>  obj-$(CONFIG_TI_SCI_PROTOCOL)	+= ti_sci.o
>  obj-$(CONFIG_TRUSTED_FOUNDATIONS) += trusted_foundations.o
>  obj-$(CONFIG_TURRIS_MOX_RWTM)	+= turris-mox-rwtm.o
> +obj-$(CONFIG_X86_64_FLASH_MMAP)	+= x86_64_flash_mmap.o
>  
>  obj-y				+= arm_scmi/
>  obj-y				+= broadcom/
> diff --git a/drivers/firmware/x86_64_flash_mmap.c b/drivers/firmware/x86_64_flash_mmap.c
> new file mode 100644
> index 000000000000..f9d871a8b516
> --- /dev/null
> +++ b/drivers/firmware/x86_64_flash_mmap.c
> @@ -0,0 +1,65 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Export the memory-mapped BIOS region of the platform SPI flash as
> + * a read-only sysfs binary attribute on X86_64 systems.
> + *
> + * Copyright © 2021 immune GmbH
> + */
> +
> +#include <linux/version.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/sysfs.h>
> +#include <linux/kobject.h>
> +
> +#define FLASH_REGION_START 0xFF000000ULL
> +#define FLASH_REGION_SIZE 0x1000000ULL

Where do these values come from?

> +#define FLASH_REGION_MASK (FLASH_REGION_SIZE - 1)
> +
> +struct kobject *kobj_ref;

Only 1?  Not per-hardware-device?

> +
> +static ssize_t bios_region_read(struct file *file, struct kobject *kobj,
> +				struct bin_attribute *bin_attr, char *buffer,
> +				loff_t offset, size_t count)
> +{
> +	resource_size_t pa = FLASH_REGION_START + (offset & FLASH_REGION_MASK);
> +	void __iomem *va = ioremap(pa, PAGE_SIZE);

Why PAGE_SIZE?

> +
> +	memcpy_fromio(buffer, va, PAGE_SIZE);
> +	iounmap(va);
> +
> +	return min(count, PAGE_SIZE);
> +}
> +
> +BIN_ATTR_RO(bios_region, FLASH_REGION_SIZE);
> +
> +static int __init flash_mmap_init(void)
> +{
> +	int ret = 0;
> +
> +	kobj_ref = kobject_create_and_add("flash_mmap", firmware_kobj);
> +	ret = sysfs_create_bin_file(kobj_ref, &bin_attr_bios_region);

You just raced with userspace and lost :(

Please make a sysfs attribute part of a default "group" for a kobject.
But as you are using a "raw" kobject here, that feels really wrong to
me.  Isn't this some sort of platform device really?  Why not go that
way, why tie this to the firmware subsystem?

> +	if (ret) {
> +		pr_err("sysfs_create_bin_file failed\n");
> +		goto error;
> +	}
> +
> +	return ret;

So this just "always works"?  That feels VERY dangerous.

As this is a x86 thing, you should also cc: the x86 maintainers.

thanks,

greg k-h

RE: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[David Laight]

From: Hans-Gert Dahmen
> Sent: 22 June 2021 15:24
> 
> Make the 16MiB long memory-mapped BIOS region of the platform SPI flash
> on X86_64 system available via /sys/kernel/firmware/flash_mmap/bios_region
> for pen-testing, security analysis and malware detection on kernels
> which restrict module loading and/or access to /dev/mem.

Are you saying that my 15 year old 64bit Athlon cpu and bios
have this large SPI flash and the required hardware to
convert bus cycles to serial spi reads?

I very much doubt it.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Hans-Gert Dahmen]

Hi,

these are some good points.

On 23.06.21 00:18, David Laight wrote:
> Are you saying that my 15 year old 64bit Athlon cpu and bios
> have this large SPI flash

No. The reads will wrap, i.e. if your flash is 2MB then it would be 
repeated 8 times in the 16MB window.

> and the required hardware to
> convert bus cycles to serial spi reads?

Yes. The window is part of the DMI interface and the south bridge or PCH 
converts the bus cycles to SPI reads. It is because this region contains 
the reset vector address of your CPU and the very first instruction it 
executes after a reset when the internal setup is done will actually be 
loaded from the serial SPI bus. It is AFAIK part of AMD's original 
64-bit specification.

However, after reading your mail I understand that I should have looked 
up the exact explanations in the respective specs. So to definitively 
answer your question I need to know which south bridge there is in your 
15 year old system and have a look into its datasheet. Do you know which 
one it is by any chance?

Hans-Gert Dahmen

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[gregkh]

On Wed, Jun 23, 2021 at 02:17:54PM +0200, Hans-Gert Dahmen wrote:
> Hi,
> 
> these are some good points.
> 
> On 23.06.21 00:18, David Laight wrote:
> > Are you saying that my 15 year old 64bit Athlon cpu and bios
> > have this large SPI flash
> 
> No. The reads will wrap, i.e. if your flash is 2MB then it would be repeated
> 8 times in the 16MB window.
> 
> > and the required hardware to
> > convert bus cycles to serial spi reads?
> 
> Yes. The window is part of the DMI interface and the south bridge or PCH
> converts the bus cycles to SPI reads. It is because this region contains the
> reset vector address of your CPU and the very first instruction it executes
> after a reset when the internal setup is done will actually be loaded from
> the serial SPI bus. It is AFAIK part of AMD's original 64-bit specification.
> 
> However, after reading your mail I understand that I should have looked up
> the exact explanations in the respective specs. So to definitively answer
> your question I need to know which south bridge there is in your 15 year old
> system and have a look into its datasheet. Do you know which one it is by
> any chance?

The point is that you will never be able to do this for all devices.
You should ONLY be allowed to have this module bind to the hardware that
you KNOW it will work with.

So please work off of a DMI table, or some such hardware description,
instead of just blindly enabling it for all systems.

thanks,

greg k-h

RE: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[David Laight]

From: Hans-Gert Dahmen
> Sent: 23 June 2021 13:18
> 
> these are some good points.
> 
> On 23.06.21 00:18, David Laight wrote:
> > Are you saying that my 15 year old 64bit Athlon cpu and bios
> > have this large SPI flash
> 
> No. The reads will wrap, i.e. if your flash is 2MB then it would be
> repeated 8 times in the 16MB window.
> 
> > and the required hardware to
> > convert bus cycles to serial spi reads?
> 
> Yes. The window is part of the DMI interface and the south bridge or PCH
> converts the bus cycles to SPI reads. It is because this region contains
> the reset vector address of your CPU and the very first instruction it
> executes after a reset when the internal setup is done will actually be
> loaded from the serial SPI bus. It is AFAIK part of AMD's original
> 64-bit specification.
> 
> However, after reading your mail I understand that I should have looked
> up the exact explanations in the respective specs. So to definitively
> answer your question I need to know which south bridge there is in your
> 15 year old system and have a look into its datasheet. Do you know which
> one it is by any chance?

Absolutely no idea.
That particular system doesn't actually boot any more
with either cpu I have for it plugged it.
I suspect the psu voltages are out of range and have broken it.

But that isn't the point.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Hans-Gert Dahmen]

On 23.06.21 14:40, gregkh@linuxfoundation.org wrote:
>> On Wed, Jun 23, 2021 at 02:17:54PM +0200, Hans-Gert Dahmen wrote:
>> Hi,
>> Yes. The window is part of the DMI interface and the south bridge or PCH
>> converts the bus cycles to SPI reads. It is because this region contains the
>> reset vector address of your CPU and the very first instruction it executes
>> after a reset when the internal setup is done will actually be loaded from
>> the serial SPI bus. It is AFAIK part of AMD's original 64-bit specification.
> The point is that you will never be able to do this for all devices.
> You should ONLY be allowed to have this module bind to the hardware that
> you KNOW it will work with.
> 
> So please work off of a DMI table, or some such hardware description,
> instead of just blindly enabling it for all systems.

I was referring to the DMI/QPI/PCI interface that connects the 
ICH/PCH/south bridge to the CPU. I have gone through all datasheets of 
intel ICH and PCH and they state that the address range from 0xff000000 
through 0xffffffff is a fixed mapping that cannot be changed (no BAR) 
except for the original ICH (dating back to 1999) where the window is 
only 8MB. The original ICH is for 32-bit systems only so all 64-bit 
Intel systems that exist have this feature. I have talked to somebody 
who works with future Intel hardware and the person indicated that it is 
not likely to change.

This is why I made the module depend on X86_64. I still have to do the 
same complete research for AMD systems which is a little harder to do, 
so I am proposing to check if the root complex has Intel's vendor ID and 
only load the module on 64-bit Intel systems until I can confirm the 
same behavior for all 64-bit AMD systems. Then I could check if the root 
complex is Intel or AMD. Would that suffice as "some such hardware 
description"?

Here are my public sources:

ICH0 Datasheet Chapter 6.3 and Table 6-5
https://www.tautec-electronics.de/Datenblaetter/Schaltkreise/FW82801AA.pdf

ICH2 Datasheet Chapter 6.4 and Table 6-4
https://www.intel.com/content/dam/doc/datasheet/82801ba-i-o-controller-hub-2-82801bam-i-o-controller-hub-2-mobile-datasheet.pdf

ICH3 Datasheet Chapter 6.4 and Table 6-5
https://www.intel.com/content/dam/doc/datasheet/82801ca-io-controller-hub-3-datasheet.pdf

ICH4 Datasheet Chapter 6.4 and Table 6-5
https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/82801db-io-controller-hub-4-datasheet.pdf

ICH5 Datasheet Chapter 6.4 and Table 133
https://www.intel.com/content/dam/doc/datasheet/82801eb-82801er-io-controller-hub-datasheet.pdf

ICH6 Datasheet Chapter 6.4 and Table 6-4
https://www.intel.com/content/dam/doc/datasheet/io-controller-hub-6-datasheet.pdf

ICH7 Datasheet Chapter 6.4 and Table 6-4
https://www.intel.com/content/dam/doc/datasheet/i-o-controller-hub-7-datasheet.pdf

ICH8 Datasheet Chapter 6.4 and Table 102
https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/io-controller-hub-8-datasheet.pdf

ICH9 Datasheet Chapter 9.4 and Table 9-4
https://www.intel.com/content/dam/doc/datasheet/io-controller-hub-9-datasheet.pdf

ICH10 Datasheet Chapter 9.4 and Table 9-4
https://theswissbay.ch/pdf/Datasheets/Intel/io-controller-hub-10-family-datasheet.pdf

PCH Intel 5 Series Datasheet Chapter 9.4 and Table 9-4
https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/5-chipset-3400-chipset-datasheet.pdf

PCH Intel 6 Series Datasheet Chapter 9.4 and Table 9-4
https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/6-chipset-c200-chipset-datasheet.pdf

PCH Intel 7 Series Datasheet Chapter 9.4 and Table 9-4
https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/7-series-chipset-pch-datasheet.pdf

PCH Intel 8 Series Datasheet Chapter 9.4 and Table 9-4
https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/8-series-chipset-pch-datasheet.pdf

PCH Intel 9 Series Datasheet Chapter 9.4 and Table 9-4
https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/9-series-chipset-pch-datasheet.pdf

PCH Intel 100 Series Datasheet Vol 1 Chapter 4.3 and Table 4-4
https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/100-series-chipset-datasheet-vol-1.pdf

PCH Intel 200 Series Datasheet Vol 1 Chapter 4.3 and Table 4-4
https://www.mouser.com/datasheet/2/612/200-series-chipset-pch-datasheet-vol-1-1391746.pdf

PCH Intel 300 Series Datasheet Vol 1 Chapter 4.3 and Table 4-4
https://www.intel.com/content/dam/www/public/us/en/documents/technical-specifications/300-series-chipset-on-package-pch-datasheet-vol-1.pdf

PCH Intel 400 Series Datasheet Vol 1 Chapter 4.2 and Table 8
https://images-eu.ssl-images-amazon.com/images/I/B1TDsSyARKS.pdf

Hans-Gert Dahmen

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[gregkh]

On Thu, Jun 24, 2021 at 01:20:28PM +0200, Hans-Gert Dahmen wrote:
> 
> On 23.06.21 14:40, gregkh@linuxfoundation.org wrote:
> > > On Wed, Jun 23, 2021 at 02:17:54PM +0200, Hans-Gert Dahmen wrote:
> > > Hi,
> > > Yes. The window is part of the DMI interface and the south bridge or PCH
> > > converts the bus cycles to SPI reads. It is because this region contains the
> > > reset vector address of your CPU and the very first instruction it executes
> > > after a reset when the internal setup is done will actually be loaded from
> > > the serial SPI bus. It is AFAIK part of AMD's original 64-bit specification.
> > The point is that you will never be able to do this for all devices.
> > You should ONLY be allowed to have this module bind to the hardware that
> > you KNOW it will work with.
> > 
> > So please work off of a DMI table, or some such hardware description,
> > instead of just blindly enabling it for all systems.
> 
> I was referring to the DMI/QPI/PCI interface that connects the ICH/PCH/south
> bridge to the CPU. I have gone through all datasheets of intel ICH and PCH
> and they state that the address range from 0xff000000 through 0xffffffff is
> a fixed mapping that cannot be changed (no BAR) except for the original ICH
> (dating back to 1999) where the window is only 8MB. The original ICH is for
> 32-bit systems only so all 64-bit Intel systems that exist have this
> feature. I have talked to somebody who works with future Intel hardware and
> the person indicated that it is not likely to change.
> 
> This is why I made the module depend on X86_64. I still have to do the same
> complete research for AMD systems which is a little harder to do, so I am
> proposing to check if the root complex has Intel's vendor ID and only load
> the module on 64-bit Intel systems until I can confirm the same behavior for
> all 64-bit AMD systems. Then I could check if the root complex is Intel or
> AMD. Would that suffice as "some such hardware description"?

That would help, yes.  Especially given the other types of Intel-like
cpus we are seeing in the wild these days (not all the world is Intel
and AMD...)

But what is this really going to be used for?  What userspace tools need
this type of direct access to do something useful?

thanks,

greg k-h

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Hans-Gert Dahmen]

Hi,

this is the first time I am working on the Linux kernel, so please 
excuse that I overlooked some things.

On 22.06.21 22:02, Greg KH wrote:
> On Tue, Jun 22, 2021 at 04:23:34PM +0200, Hans-Gert Dahmen wrote:
>> Make the 16MiB long memory-mapped BIOS region of the platform SPI flash
>> on X86_64 system available via /sys/kernel/firmware/flash_mmap/bios_region
>> for pen-testing, security analysis and malware detection on kernels
>> which restrict module loading and/or access to /dev/mem.
>>
>> It will be used by the open source Converged Security Suite.
>> https://github.com/9elements/converged-security-suite
>>
>> Signed-off-by: Hans-Gert Dahmen <hans-gert.dahmen@immu.ne>
>> ---
>>   drivers/firmware/Kconfig             |  9 ++++
>>   drivers/firmware/Makefile            |  1 +
>>   drivers/firmware/x86_64_flash_mmap.c | 65 ++++++++++++++++++++++++++++
>>   3 files changed, 75 insertions(+)
>>   create mode 100644 drivers/firmware/x86_64_flash_mmap.c
>>
>> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
>> index db0ea2d2d75a..bd77ca2b4fa6 100644
>> --- a/drivers/firmware/Kconfig
>> +++ b/drivers/firmware/Kconfig
>> @@ -296,6 +296,15 @@ config TURRIS_MOX_RWTM
>>   	  other manufacturing data and also utilize the Entropy Bit Generator
>>   	  for hardware random number generation.
>>   
>> +config X86_64_FLASH_MMAP
>> +	tristate "Export platform flash memory-mapped BIOS region"
>> +	depends on X86_64
>> +	help
>> +	  Export the memory-mapped BIOS region of the platform SPI flash as
>> +	  a read-only sysfs binary attribute on X86_64 systems. The first 16MiB
>> +	  will be accessible via /sys/kernel/firmware/flash_mmap/bios_region
>> +	  for security and malware analysis for example.
> 
> Module name information here?
> 
> Can this be auto-loaded based on hardware-specific values somewhere?
> Otherwise it just looks like if this module loads, it will "always
> work"?
> 
> And why would you want to map the bios into userspace?
> 
> What bios, UEFI?
> 
> And you need a Documentation/ABI/ update for new sysfs files.

The core use-case is security analysis and detecting BIOS/UEFI malware. 
It is going to be used by the open-source Converged Security Suite 
developed by Facebook, Google and 9elements security. The CSS dissects 
UEFI binaries and checks it for common vulnerabilities.

The current state is that there are some drivers to access the SPI flash 
bit they are in a questionable state and often don't work. Using this 
memory mapped region works most of the time without requiring a real 
hardware driver and significantly lowers the barrier to asses UEFI 
security of systems deployed in the wild.

In another mail I have shown that this can safely be done on Intel 
systems so I will make this module load on Intel systems for now and 
also fix the documentation.

> 
> 
>> +
>>   source "drivers/firmware/broadcom/Kconfig"
>>   source "drivers/firmware/google/Kconfig"
>>   source "drivers/firmware/efi/Kconfig"
>> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
>> index 5e013b6a3692..eb7483c5a2ac 100644
>> --- a/drivers/firmware/Makefile
>> +++ b/drivers/firmware/Makefile
>> @@ -21,6 +21,7 @@ obj-$(CONFIG_QCOM_SCM)		+= qcom_scm.o qcom_scm-smc.o qcom_scm-legacy.o
>>   obj-$(CONFIG_TI_SCI_PROTOCOL)	+= ti_sci.o
>>   obj-$(CONFIG_TRUSTED_FOUNDATIONS) += trusted_foundations.o
>>   obj-$(CONFIG_TURRIS_MOX_RWTM)	+= turris-mox-rwtm.o
>> +obj-$(CONFIG_X86_64_FLASH_MMAP)	+= x86_64_flash_mmap.o
>>   
>>   obj-y				+= arm_scmi/
>>   obj-y				+= broadcom/
>> diff --git a/drivers/firmware/x86_64_flash_mmap.c b/drivers/firmware/x86_64_flash_mmap.c
>> new file mode 100644
>> index 000000000000..f9d871a8b516
>> --- /dev/null
>> +++ b/drivers/firmware/x86_64_flash_mmap.c
>> @@ -0,0 +1,65 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Export the memory-mapped BIOS region of the platform SPI flash as
>> + * a read-only sysfs binary attribute on X86_64 systems.
>> + *
>> + * Copyright © 2021 immune GmbH
>> + */
>> +
>> +#include <linux/version.h>
>> +#include <linux/init.h>
>> +#include <linux/module.h>
>> +#include <linux/io.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/kobject.h>
>> +
>> +#define FLASH_REGION_START 0xFF000000ULL
>> +#define FLASH_REGION_SIZE 0x1000000ULL
> 
> Where do these values come from?

I have listed the relevant Intel datasheets in another mail in this thread.

> 
>> +#define FLASH_REGION_MASK (FLASH_REGION_SIZE - 1)
>> +
>> +struct kobject *kobj_ref;
> 
> Only 1?  Not per-hardware-device?

Yes, there is only one BIOS/UEFI that is configured to actively boot the 
system. This method is not suitable to access shadow flash chips or 
other wild things that mainboard manufacturers did in the past.

> 
>> +
>> +static ssize_t bios_region_read(struct file *file, struct kobject *kobj,
>> +				struct bin_attribute *bin_attr, char *buffer,
>> +				loff_t offset, size_t count)
>> +{
>> +	resource_size_t pa = FLASH_REGION_START + (offset & FLASH_REGION_MASK);
>> +	void __iomem *va = ioremap(pa, PAGE_SIZE);
> 
> Why PAGE_SIZE?

Please correct me if I'm wrong: the documentation is sparse and from 
what I could see in the sources it appears that binary attributes pass a 
page sized buffer around. I was assuming that the offset parameter would 
be page aligned.

> 
>> +
>> +	memcpy_fromio(buffer, va, PAGE_SIZE);
>> +	iounmap(va);
>> +
>> +	return min(count, PAGE_SIZE);
>> +}
>> +
>> +BIN_ATTR_RO(bios_region, FLASH_REGION_SIZE);
>> +
>> +static int __init flash_mmap_init(void)
>> +{
>> +	int ret = 0;
>> +
>> +	kobj_ref = kobject_create_and_add("flash_mmap", firmware_kobj);
>> +	ret = sysfs_create_bin_file(kobj_ref, &bin_attr_bios_region);
> 
> You just raced with userspace and lost :(

I have taken inspiration from other modules. The documentation doesn't 
say a lot. Could somebody point me to a proper example somewhere in the 
source?

> 
> Please make a sysfs attribute part of a default "group" for a kobject.
> But as you are using a "raw" kobject here, that feels really wrong to
> me.  Isn't this some sort of platform device really?  Why not go that
> way, why tie this to the firmware subsystem?
> 

What this module provides read access to is the firmware. I am new to 
Linux kernel development and found it quite hard to decide where to put 
this. Suggestions are welcome.

>> +	if (ret) {
>> +		pr_err("sysfs_create_bin_file failed\n");
>> +		goto error;
>> +	}
>> +
>> +	return ret;
> 
> So this just "always works"?  That feels VERY dangerous.

Will change that.

> 
> As this is a x86 thing, you should also cc: the x86 maintainers.

Will do.

> 
> thanks,
> 
> greg k-h
> 

Hans-Gert Dahmen

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Hans-Gert Dahmen]

Am Fr., 12. Nov. 2021 um 11:43 Uhr schrieb Greg KH <gregkh@linuxfoundation.org>:
>
> On Fri, Nov 12, 2021 at 10:09:14AM +0000, Richard Hughes wrote:
> > On Fri, 12 Nov 2021 at 06:52, Greg KH <gregkh@linuxfoundation.org> wrote:
> > > Why don't we just export these areas to userspace and let the decoding
> > > of them happen there?
> >
> > Unless I'm missing something, the patch from Hans-Gert does just that:
> > exposing the IFD BIOS partition that encloses the various EFI file
> > volumes.
>
> But it is not tied into the EFI subsystem at all, binding only to those
> resources.  It does not do anything with any efi symbol or access
> control.
>
> Again, that's my primary complaint here, the driver HAS to be able to
> tell the kernel what resource it wants to bind to and control, right now
> it just "assumes" that it can have access to a chunk of memory without
> any notification or checks at all, which will cause problems on systems
> that do not follow that assumption.
>
> So while you all are arguing over oddities, the main complaint here of
> "this driver is not ok as-is" seems to keep being ignored for some odd
> reason.

No, no, at least I am not ignoring it. I am aware of that and I was
planning to fix the broken parts since the start of the discussion.
Sorry for the miscommunication here.

>
> I'm going to just ignore this thread now and wait for a new patch to
> review.

That's the plan and I'd be happy if we don't have to discuss it
further right now.

>
> thanks,
>
> greg k-h

Hans-Gert

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Greg KH]

On Fri, Nov 12, 2021 at 10:09:14AM +0000, Richard Hughes wrote:
> On Fri, 12 Nov 2021 at 06:52, Greg KH <gregkh@linuxfoundation.org> wrote:
> > Why don't we just export these areas to userspace and let the decoding
> > of them happen there?
> 
> Unless I'm missing something, the patch from Hans-Gert does just that:
> exposing the IFD BIOS partition that encloses the various EFI file
> volumes.

But it is not tied into the EFI subsystem at all, binding only to those
resources.  It does not do anything with any efi symbol or access
control.

Again, that's my primary complaint here, the driver HAS to be able to
tell the kernel what resource it wants to bind to and control, right now
it just "assumes" that it can have access to a chunk of memory without
any notification or checks at all, which will cause problems on systems
that do not follow that assumption.

So while you all are arguing over oddities, the main complaint here of
"this driver is not ok as-is" seems to keep being ignored for some odd
reason.

I'm going to just ignore this thread now and wait for a new patch to
review.

thanks,

greg k-h

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Richard Hughes]

On Fri, 12 Nov 2021 at 06:52, Greg KH <gregkh@linuxfoundation.org> wrote:
> Why don't we just export these areas to userspace and let the decoding
> of them happen there?

Unless I'm missing something, the patch from Hans-Gert does just that:
exposing the IFD BIOS partition that encloses the various EFI file
volumes.

Richard.

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Mika Westerberg]

Hi,

On Thu, Nov 11, 2021 at 04:16:14PM +0100, Hans-Gert Dahmen wrote:
> > In case someone is unfamiliar with this, the Intel SPI hardware
> > exposes two interfaces through the same controller. One that is called
> > software sequencer and there is a register of "allowed" opcodes that
> > software can use as it wishes. This register can be locked down but is
> > not always. The second interface is the hardware sequencer that only
> > exposes higher level commands like read, write and so on and internally
> > then executes whatever opcode the controller got from the chip
> > "supported opcodes table" (SFDP).  The recent Intel hardware, all
> > big-cores, only provide hardware sequencer and the software one is not
> > even available.
> 
> I am familiar with this and I do totally agree. I believe HW
> sequencing is available since sandy-bridge from 2011, so it will
> suffice for modern platforms. Honestly me and my developer friends
> never understood why this driver needs to still focus on SW sequencing
> altogether, it seems like a (possibly buggy) relic that just slows
> down the vital parts.

Just to clarify the software sequencer was used in "recent" Atoms
(Baytrail, Braswell and I think its successor too). After Broxton I
think it all is now hardware sequencer only. AFAIK those are still used
in embedded world so we should keep the support in the driver but that
support can be put under the "DANGEROUS" KConfig option.

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Greg KH]

On Thu, Nov 11, 2021 at 09:07:42PM +0000, Richard Hughes wrote:
> On Thu, 11 Nov 2021 at 15:50, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > I was thinking about SHA256 hashes or so (as they tell about
> > binaries). In any case the interface for this seems to be in the
> > kernel.
> 
> I'm quite sure you don't want to put those EFI format decoders in the
> kernel; there is all kinds of weird compression schemes, volumes are
> recursive, and vendors like to hide weird things in the undocumented
> (or reserved) areas.

Why don't we just export these areas to userspace and let the decoding
of them happen there?  That's a real "thing" that the kernel can easily
detect is present and know to export properly as they are defined by the
UEFI specification, right?

Yes, that will not help for the non-UEFI x86 systems, but it's a
start...

thanks,

greg k-h

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Richard Hughes]

On Thu, 11 Nov 2021 at 15:50, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> I was thinking about SHA256 hashes or so (as they tell about
> binaries). In any case the interface for this seems to be in the
> kernel.

I'm quite sure you don't want to put those EFI format decoders in the
kernel; there is all kinds of weird compression schemes, volumes are
recursive, and vendors like to hide weird things in the undocumented
(or reserved) areas.

Richard.

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Hans-Gert Dahmen]

Am Do., 11. Nov. 2021 um 20:14 Uhr schrieb Ard Biesheuvel <ardb@kernel.org>:
>
> On Thu, 11 Nov 2021 at 19:15, Hans-Gert Dahmen <hans-gert.dahmen@immu.ne> wrote:
> >
> > Am Do., 11. Nov. 2021 um 18:49 Uhr schrieb Andy Shevchenko
> > <andy.shevchenko@gmail.com>:
> > >
> > > On Thu, Nov 11, 2021 at 6:55 PM Hans-Gert Dahmen
> > > <hans-gert.dahmen@immu.ne> wrote:
> > > > Am Do., 11. Nov. 2021 um 17:45 Uhr schrieb Andy Shevchenko
> > > > <andy.shevchenko@gmail.com>:
> > > > > On Thu, Nov 11, 2021 at 6:07 PM Hans-Gert Dahmen
> > > > > <hans-gert.dahmen@immu.ne> wrote:
> > > > > > Am Do., 11. Nov. 2021 um 16:31 Uhr schrieb Andy Shevchenko
> > > > > > <andy.shevchenko@gmail.com>:
> > > > > > > On Thu, Nov 11, 2021 at 4:33 PM Hans-Gert Dahmen
> > > > > > > <hans-gert.dahmen@immu.ne> wrote:
> > > > > > > > Am Do., 11. Nov. 2021 um 14:55 Uhr schrieb Andy Shevchenko
> > > > > > > > <andy.shevchenko@gmail.com>:
> > > > > > > > > On Thu, Nov 11, 2021 at 2:56 PM Hans-Gert Dahmen
> > > > > > > > > <hans-gert.dahmen@immu.ne> wrote:
> > > > > > > > > > Am Do., 11. Nov. 2021 um 13:46 Uhr schrieb Andy Shevchenko
> > > > > > > > > > <andy.shevchenko@gmail.com>:
> > > > > > > > > > > On Thu, Nov 11, 2021 at 1:46 PM Richard Hughes <hughsient@gmail.com> wrote:
> > > > > > > > > > > > On Thu, 11 Nov 2021 at 10:33, Mika Westerberg
> > > > > > > > > > > > <mika.westerberg@linux.intel.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > > it's always going to work on x64 -- if the system firmware isn't available at that offset then the platform just isn't going to boot.
> > >
> > > (1)
> > >
> > > > > > > > > > > Well, it's _usual_ case, but in general the assumption is simply
> > > > > > > > > > > incorrect. Btw, have you checked it on Coreboot enabled platforms?
> > > > > > > > > > > What about bare metal configurations where the bootloader provides
> > > > > > > > > > > services to the OS?
> > > > > > > > > >
> > > > > > > > > > No it is always the case. I suggest you go read your own Intel specs
> > > > > > > > > > and datasheets
> > >
> > > (2)
> > >
> > > > > > > > > Point me out, please, chapters in SDM (I never really read it in full,
> > > > > > > > > it's kinda 10x Bible size). What x86 expects is 16 bytes at the end of
> > > > > > > > > 1Mb physical address space that the CPU runs at first.
> > > > > > > >
> > > > > > > > So you do not know what you are talking about, am I correct?
> > > > > > >
> > > > > > > Let me comment on this provocative question later, after some other
> > > > > > > comments first.
> > > > > > >
> > > > > > > > Starting
> > > > > > > > from 386 the first instruction is executed at 0xFFFFFFF0h. What you
> > > > > > > > are referring to is the 8086 reset vector and that was like 40 years
> > > > > > > > ago.
> > > > > > >
> > > > > > > True. The idea is the same, It has a reset vector standard for x86
> > > > > > > (which doesn't explicitly tell what is there). So, nothing new or
> > > > > > > different here.
> > > > > > >
> > > > > > > > Please refer to SDM volume 3A, chapter 9, section 9.1.4 "First
> > > > > > > > Instruction Executed", paragraph two. Just watch out for the hex
> > > > > > > > number train starting with FFFFF... then you will find it. This is
> > > > > > > > what requires the memory range to be mapped. Modern Intel CPUs require
> > > > > > > > larger portions, because of the ACM loading and XuCode and whatnot.
> > > > > > >
> > > > > > > Thanks. Have you read 9.7 and 9.8, btw?
> > > > > > > Where does it tell anything about memory to be mapped to a certain
> > > > > > > address, except the last up to 16 bytes?
> > > > > >
> > > > > > It doesn't, except that the FIT, ACM, BootGuard, XuCode stuff rely on
> > > > > > their binaries to be there, this just sets the upper address limit of
> > > > > > the window.
> > > > >
> > > > > Why is it needed? I mean the listed blobs are not mandatory to get
> > > > > system boot. Is this correct?
> > > >
> > > > That doesn't matter for this case.
> > >
> > > Then why did you mention them?
> > >
> > > > > > > > Please refer to the email [1] from me linked below where I reference
> > > > > > > > all PCH datasheets of the x64 era to prove that 16MB are mapped
> > > > > > > > hard-wired. Note that the range cannot be turned off and will read
> > > > > > > > back 0xFF's if the PCH registers are configured to not be backed by
> > > > > > > > the actual SPI flash contents.
> > > > > > >
> > > > > > > And as I said it does not cover _all_ x86 designs (usual != all) .
> > > > > > > Have you heard about Intel MID line of SoCs? Do you know that they
> > > > > >
> > > > > > No and a quick search didn't turn up anything. Can you point me to
> > > > > > resources about those SoCs? Also my module is targeting x86_64, that
> > > > > > is only a subset of x86 designs.
> > > > >
> > > > > They are x86_32 and x86_64, so in the category you listed.
> > > > >
> > > > > Unfortunately there is indeed not much publicly available information,
> > > > > but I can tell you that from a design perspective you may consider
> > > > > them PCH-less.
> > > >
> > > > Okay fine. Now you come around the corner with undocumented Intel
> > > > devices and make your first semi-valid point.
> > >
> > > Huh?!
> > > You simply have the wrong assumption (see (1) and (2) above) and
> > > _this_ is my point. And it seems you still can't admit that. Be brave!
> > >
> >
> > I thought my last email was admitting that, but, if you insist, I
> > hereby explicitly admit, that, now, after some 40 emails, you have
> > brought forward a valid point that proves my assumption wrong. If this
> > is what makes you happy, then I can also certify my defeat on paper
> > and send it to you so you can hang it as a trophy on your wall. The
> > notion of being brave or not has no value for me here, I am purely
> > interested in the technical details. I never said that my solution was
> > brilliant, and, all I wanted, was, as you already know: to retain
> > functionality used by userspace tools on locked-down systems. Please,
> > next time, be of good character and don't play games like this. Just
> > directly bring forward the evidence to challenge an assumption.
> >
> > > > Why did it take you so
> > > > long?
> > >
> > > Same question to you.
> > >
> > > >  Why did you seemingly just try to derail the discussion before?
> > >
> > > See just above. I don't like when people are blind with their
> > > brilliant solutions.
> >
> > Again, now, if you have the feeling that someone is blind, please
> > don't fool them around just to make them painfully aware of their
> > blind spot. IMO the more human solution is not to react with anger,
> > like you did, but with constructivism.
> >
>
> Can we cut the drama please?
>
> Greg has already pointed out that you cannot blindly expose this
> without tying it to a property exposed by the hardware. Andy has
> pointed out that your assumption that any x86_64 based platform
> exposes this region does not hold.
>
> So maybe it is time for some 'constructivism' on your part, and simply
> go and implement what the reviewers suggested, rather than keep this
> pointless discussion going?

It is in my best interest to not keep this discussion going and I
already asked a couple of times what hardware I could bind to on
tuesday. If there had been any constructive suggestion as to what I
could implement, I would already have done it. Andy only now pointed
out specifics about what could be the problem. Before that, the
reasons why my patch isn't welcome changed seemingly just to argue
against it and I was accused of trying to sneak something into the
kernel. Do you think I like to waste my and everyone's time like this?
Anyway, I am really tired of this and I nearly have wasted all my
holiday for this discussion, so I don't have any time and energy left
to finally implement the proper solution during this merge window.

Hans-Gert

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Ard Biesheuvel]

On Thu, 11 Nov 2021 at 19:15, Hans-Gert Dahmen <hans-gert.dahmen@immu.ne> wrote:
>
> Am Do., 11. Nov. 2021 um 18:49 Uhr schrieb Andy Shevchenko
> <andy.shevchenko@gmail.com>:
> >
> > On Thu, Nov 11, 2021 at 6:55 PM Hans-Gert Dahmen
> > <hans-gert.dahmen@immu.ne> wrote:
> > > Am Do., 11. Nov. 2021 um 17:45 Uhr schrieb Andy Shevchenko
> > > <andy.shevchenko@gmail.com>:
> > > > On Thu, Nov 11, 2021 at 6:07 PM Hans-Gert Dahmen
> > > > <hans-gert.dahmen@immu.ne> wrote:
> > > > > Am Do., 11. Nov. 2021 um 16:31 Uhr schrieb Andy Shevchenko
> > > > > <andy.shevchenko@gmail.com>:
> > > > > > On Thu, Nov 11, 2021 at 4:33 PM Hans-Gert Dahmen
> > > > > > <hans-gert.dahmen@immu.ne> wrote:
> > > > > > > Am Do., 11. Nov. 2021 um 14:55 Uhr schrieb Andy Shevchenko
> > > > > > > <andy.shevchenko@gmail.com>:
> > > > > > > > On Thu, Nov 11, 2021 at 2:56 PM Hans-Gert Dahmen
> > > > > > > > <hans-gert.dahmen@immu.ne> wrote:
> > > > > > > > > Am Do., 11. Nov. 2021 um 13:46 Uhr schrieb Andy Shevchenko
> > > > > > > > > <andy.shevchenko@gmail.com>:
> > > > > > > > > > On Thu, Nov 11, 2021 at 1:46 PM Richard Hughes <hughsient@gmail.com> wrote:
> > > > > > > > > > > On Thu, 11 Nov 2021 at 10:33, Mika Westerberg
> > > > > > > > > > > <mika.westerberg@linux.intel.com> wrote:
> > > > > > > > > >
> > > > > > > > > > > it's always going to work on x64 -- if the system firmware isn't available at that offset then the platform just isn't going to boot.
> >
> > (1)
> >
> > > > > > > > > > Well, it's _usual_ case, but in general the assumption is simply
> > > > > > > > > > incorrect. Btw, have you checked it on Coreboot enabled platforms?
> > > > > > > > > > What about bare metal configurations where the bootloader provides
> > > > > > > > > > services to the OS?
> > > > > > > > >
> > > > > > > > > No it is always the case. I suggest you go read your own Intel specs
> > > > > > > > > and datasheets
> >
> > (2)
> >
> > > > > > > > Point me out, please, chapters in SDM (I never really read it in full,
> > > > > > > > it's kinda 10x Bible size). What x86 expects is 16 bytes at the end of
> > > > > > > > 1Mb physical address space that the CPU runs at first.
> > > > > > >
> > > > > > > So you do not know what you are talking about, am I correct?
> > > > > >
> > > > > > Let me comment on this provocative question later, after some other
> > > > > > comments first.
> > > > > >
> > > > > > > Starting
> > > > > > > from 386 the first instruction is executed at 0xFFFFFFF0h. What you
> > > > > > > are referring to is the 8086 reset vector and that was like 40 years
> > > > > > > ago.
> > > > > >
> > > > > > True. The idea is the same, It has a reset vector standard for x86
> > > > > > (which doesn't explicitly tell what is there). So, nothing new or
> > > > > > different here.
> > > > > >
> > > > > > > Please refer to SDM volume 3A, chapter 9, section 9.1.4 "First
> > > > > > > Instruction Executed", paragraph two. Just watch out for the hex
> > > > > > > number train starting with FFFFF... then you will find it. This is
> > > > > > > what requires the memory range to be mapped. Modern Intel CPUs require
> > > > > > > larger portions, because of the ACM loading and XuCode and whatnot.
> > > > > >
> > > > > > Thanks. Have you read 9.7 and 9.8, btw?
> > > > > > Where does it tell anything about memory to be mapped to a certain
> > > > > > address, except the last up to 16 bytes?
> > > > >
> > > > > It doesn't, except that the FIT, ACM, BootGuard, XuCode stuff rely on
> > > > > their binaries to be there, this just sets the upper address limit of
> > > > > the window.
> > > >
> > > > Why is it needed? I mean the listed blobs are not mandatory to get
> > > > system boot. Is this correct?
> > >
> > > That doesn't matter for this case.
> >
> > Then why did you mention them?
> >
> > > > > > > Please refer to the email [1] from me linked below where I reference
> > > > > > > all PCH datasheets of the x64 era to prove that 16MB are mapped
> > > > > > > hard-wired. Note that the range cannot be turned off and will read
> > > > > > > back 0xFF's if the PCH registers are configured to not be backed by
> > > > > > > the actual SPI flash contents.
> > > > > >
> > > > > > And as I said it does not cover _all_ x86 designs (usual != all) .
> > > > > > Have you heard about Intel MID line of SoCs? Do you know that they
> > > > >
> > > > > No and a quick search didn't turn up anything. Can you point me to
> > > > > resources about those SoCs? Also my module is targeting x86_64, that
> > > > > is only a subset of x86 designs.
> > > >
> > > > They are x86_32 and x86_64, so in the category you listed.
> > > >
> > > > Unfortunately there is indeed not much publicly available information,
> > > > but I can tell you that from a design perspective you may consider
> > > > them PCH-less.
> > >
> > > Okay fine. Now you come around the corner with undocumented Intel
> > > devices and make your first semi-valid point.
> >
> > Huh?!
> > You simply have the wrong assumption (see (1) and (2) above) and
> > _this_ is my point. And it seems you still can't admit that. Be brave!
> >
>
> I thought my last email was admitting that, but, if you insist, I
> hereby explicitly admit, that, now, after some 40 emails, you have
> brought forward a valid point that proves my assumption wrong. If this
> is what makes you happy, then I can also certify my defeat on paper
> and send it to you so you can hang it as a trophy on your wall. The
> notion of being brave or not has no value for me here, I am purely
> interested in the technical details. I never said that my solution was
> brilliant, and, all I wanted, was, as you already know: to retain
> functionality used by userspace tools on locked-down systems. Please,
> next time, be of good character and don't play games like this. Just
> directly bring forward the evidence to challenge an assumption.
>
> > > Why did it take you so
> > > long?
> >
> > Same question to you.
> >
> > >  Why did you seemingly just try to derail the discussion before?
> >
> > See just above. I don't like when people are blind with their
> > brilliant solutions.
>
> Again, now, if you have the feeling that someone is blind, please
> don't fool them around just to make them painfully aware of their
> blind spot. IMO the more human solution is not to react with anger,
> like you did, but with constructivism.
>

Can we cut the drama please?

Greg has already pointed out that you cannot blindly expose this
without tying it to a property exposed by the hardware. Andy has
pointed out that your assumption that any x86_64 based platform
exposes this region does not hold.

So maybe it is time for some 'constructivism' on your part, and simply
go and implement what the reviewers suggested, rather than keep this
pointless discussion going?

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Hans-Gert Dahmen]

Am Do., 11. Nov. 2021 um 18:49 Uhr schrieb Andy Shevchenko
<andy.shevchenko@gmail.com>:
>
> On Thu, Nov 11, 2021 at 6:55 PM Hans-Gert Dahmen
> <hans-gert.dahmen@immu.ne> wrote:
> > Am Do., 11. Nov. 2021 um 17:45 Uhr schrieb Andy Shevchenko
> > <andy.shevchenko@gmail.com>:
> > > On Thu, Nov 11, 2021 at 6:07 PM Hans-Gert Dahmen
> > > <hans-gert.dahmen@immu.ne> wrote:
> > > > Am Do., 11. Nov. 2021 um 16:31 Uhr schrieb Andy Shevchenko
> > > > <andy.shevchenko@gmail.com>:
> > > > > On Thu, Nov 11, 2021 at 4:33 PM Hans-Gert Dahmen
> > > > > <hans-gert.dahmen@immu.ne> wrote:
> > > > > > Am Do., 11. Nov. 2021 um 14:55 Uhr schrieb Andy Shevchenko
> > > > > > <andy.shevchenko@gmail.com>:
> > > > > > > On Thu, Nov 11, 2021 at 2:56 PM Hans-Gert Dahmen
> > > > > > > <hans-gert.dahmen@immu.ne> wrote:
> > > > > > > > Am Do., 11. Nov. 2021 um 13:46 Uhr schrieb Andy Shevchenko
> > > > > > > > <andy.shevchenko@gmail.com>:
> > > > > > > > > On Thu, Nov 11, 2021 at 1:46 PM Richard Hughes <hughsient@gmail.com> wrote:
> > > > > > > > > > On Thu, 11 Nov 2021 at 10:33, Mika Westerberg
> > > > > > > > > > <mika.westerberg@linux.intel.com> wrote:
> > > > > > > > >
> > > > > > > > > > it's always going to work on x64 -- if the system firmware isn't available at that offset then the platform just isn't going to boot.
>
> (1)
>
> > > > > > > > > Well, it's _usual_ case, but in general the assumption is simply
> > > > > > > > > incorrect. Btw, have you checked it on Coreboot enabled platforms?
> > > > > > > > > What about bare metal configurations where the bootloader provides
> > > > > > > > > services to the OS?
> > > > > > > >
> > > > > > > > No it is always the case. I suggest you go read your own Intel specs
> > > > > > > > and datasheets
>
> (2)
>
> > > > > > > Point me out, please, chapters in SDM (I never really read it in full,
> > > > > > > it's kinda 10x Bible size). What x86 expects is 16 bytes at the end of
> > > > > > > 1Mb physical address space that the CPU runs at first.
> > > > > >
> > > > > > So you do not know what you are talking about, am I correct?
> > > > >
> > > > > Let me comment on this provocative question later, after some other
> > > > > comments first.
> > > > >
> > > > > > Starting
> > > > > > from 386 the first instruction is executed at 0xFFFFFFF0h. What you
> > > > > > are referring to is the 8086 reset vector and that was like 40 years
> > > > > > ago.
> > > > >
> > > > > True. The idea is the same, It has a reset vector standard for x86
> > > > > (which doesn't explicitly tell what is there). So, nothing new or
> > > > > different here.
> > > > >
> > > > > > Please refer to SDM volume 3A, chapter 9, section 9.1.4 "First
> > > > > > Instruction Executed", paragraph two. Just watch out for the hex
> > > > > > number train starting with FFFFF... then you will find it. This is
> > > > > > what requires the memory range to be mapped. Modern Intel CPUs require
> > > > > > larger portions, because of the ACM loading and XuCode and whatnot.
> > > > >
> > > > > Thanks. Have you read 9.7 and 9.8, btw?
> > > > > Where does it tell anything about memory to be mapped to a certain
> > > > > address, except the last up to 16 bytes?
> > > >
> > > > It doesn't, except that the FIT, ACM, BootGuard, XuCode stuff rely on
> > > > their binaries to be there, this just sets the upper address limit of
> > > > the window.
> > >
> > > Why is it needed? I mean the listed blobs are not mandatory to get
> > > system boot. Is this correct?
> >
> > That doesn't matter for this case.
>
> Then why did you mention them?
>
> > > > > > Please refer to the email [1] from me linked below where I reference
> > > > > > all PCH datasheets of the x64 era to prove that 16MB are mapped
> > > > > > hard-wired. Note that the range cannot be turned off and will read
> > > > > > back 0xFF's if the PCH registers are configured to not be backed by
> > > > > > the actual SPI flash contents.
> > > > >
> > > > > And as I said it does not cover _all_ x86 designs (usual != all) .
> > > > > Have you heard about Intel MID line of SoCs? Do you know that they
> > > >
> > > > No and a quick search didn't turn up anything. Can you point me to
> > > > resources about those SoCs? Also my module is targeting x86_64, that
> > > > is only a subset of x86 designs.
> > >
> > > They are x86_32 and x86_64, so in the category you listed.
> > >
> > > Unfortunately there is indeed not much publicly available information,
> > > but I can tell you that from a design perspective you may consider
> > > them PCH-less.
> >
> > Okay fine. Now you come around the corner with undocumented Intel
> > devices and make your first semi-valid point.
>
> Huh?!
> You simply have the wrong assumption (see (1) and (2) above) and
> _this_ is my point. And it seems you still can't admit that. Be brave!
>

I thought my last email was admitting that, but, if you insist, I
hereby explicitly admit, that, now, after some 40 emails, you have
brought forward a valid point that proves my assumption wrong. If this
is what makes you happy, then I can also certify my defeat on paper
and send it to you so you can hang it as a trophy on your wall. The
notion of being brave or not has no value for me here, I am purely
interested in the technical details. I never said that my solution was
brilliant, and, all I wanted, was, as you already know: to retain
functionality used by userspace tools on locked-down systems. Please,
next time, be of good character and don't play games like this. Just
directly bring forward the evidence to challenge an assumption.

> > Why did it take you so
> > long?
>
> Same question to you.
>
> >  Why did you seemingly just try to derail the discussion before?
>
> See just above. I don't like when people are blind with their
> brilliant solutions.

Again, now, if you have the feeling that someone is blind, please
don't fool them around just to make them painfully aware of their
blind spot. IMO the more human solution is not to react with anger,
like you did, but with constructivism.

>
> > Is the documentation non-existent or just NDA?
>
> Of course documentation exists. Otherwise it wouldn't be possible to
> program or do something about the chips.
>
> > May I remind you that from a CPU view it doesn't matter if the PCH or
> > some other thing attached to the CPU's bus provides the mapping.
>
> Exactly my (another) point!
>
> > So if that is your concern, then the solution would be to probe if we
> > see a PCH or FCH?
>
> If you can do that...
>
> Bottom line, the proposed solution can't be accepted because it
> appeals to the false assumptions besides other technical issues which
> have been pointed out by the others.
>
> > > > > have no SPI NOR and the firmware is located on eMMC? Do you know that
> > > > > they can run Linux?
> > > >
> > > > It doesn't matter where the firmware is coming from, as long as it is
> > > > _mapped_.
> > >
> > > It's not. That address space is full of devices, the same memory can't
> > > be used for ROM and devices at the same time (I won't go to the weird
> > > concept of different read and write paths, and it's not applicable
> > > here anyway).
> > >
> > > > And something has to be mapped there, even if it is just a
> > > > loader that gets eMMC going.
> > >
> > > (Semi-)wrong. Yes, _something_ is there (with the holes), but it's
> > > _not_ a firmware memory.
> > >
> > > You may google for iomem output on that kind of device.
> > >
> > > For your convenience (an excerpt):
> > >
> > >   fee00000-fee00fff : Local APIC
> > >    fee00000-fee00fff : Reserved
> > >  ff000000-ffffffff : Reserved
> > >    ff008000-ff008fff : 0000:00:0c.0
> > >      ff008000-ff008fff : 0000:00:0c.0
> > >    ff009000-ff009fff : 0000:00:13.0
> > >      ff009000-ff009fff : intel_scu_ipc
> > >
> > > > > So, maybe it's you who do not know what you are talking about, am I correct?
> > > > >
> > > > > > [1] https://lkml.org/lkml/2021/6/24/379
> > >
> > > --
> > > With Best Regards,
> > > Andy Shevchenko
>
>
>
> --
> With Best Regards,
> Andy Shevchenko

Hans-Gert

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Andy Shevchenko]

On Thu, Nov 11, 2021 at 6:55 PM Hans-Gert Dahmen
<hans-gert.dahmen@immu.ne> wrote:
> Am Do., 11. Nov. 2021 um 17:45 Uhr schrieb Andy Shevchenko
> <andy.shevchenko@gmail.com>:
> > On Thu, Nov 11, 2021 at 6:07 PM Hans-Gert Dahmen
> > <hans-gert.dahmen@immu.ne> wrote:
> > > Am Do., 11. Nov. 2021 um 16:31 Uhr schrieb Andy Shevchenko
> > > <andy.shevchenko@gmail.com>:
> > > > On Thu, Nov 11, 2021 at 4:33 PM Hans-Gert Dahmen
> > > > <hans-gert.dahmen@immu.ne> wrote:
> > > > > Am Do., 11. Nov. 2021 um 14:55 Uhr schrieb Andy Shevchenko
> > > > > <andy.shevchenko@gmail.com>:
> > > > > > On Thu, Nov 11, 2021 at 2:56 PM Hans-Gert Dahmen
> > > > > > <hans-gert.dahmen@immu.ne> wrote:
> > > > > > > Am Do., 11. Nov. 2021 um 13:46 Uhr schrieb Andy Shevchenko
> > > > > > > <andy.shevchenko@gmail.com>:
> > > > > > > > On Thu, Nov 11, 2021 at 1:46 PM Richard Hughes <hughsient@gmail.com> wrote:
> > > > > > > > > On Thu, 11 Nov 2021 at 10:33, Mika Westerberg
> > > > > > > > > <mika.westerberg@linux.intel.com> wrote:
> > > > > > > >
> > > > > > > > > it's always going to work on x64 -- if the system firmware isn't available at that offset then the platform just isn't going to boot.

(1)

> > > > > > > > Well, it's _usual_ case, but in general the assumption is simply
> > > > > > > > incorrect. Btw, have you checked it on Coreboot enabled platforms?
> > > > > > > > What about bare metal configurations where the bootloader provides
> > > > > > > > services to the OS?
> > > > > > >
> > > > > > > No it is always the case. I suggest you go read your own Intel specs
> > > > > > > and datasheets

(2)

> > > > > > Point me out, please, chapters in SDM (I never really read it in full,
> > > > > > it's kinda 10x Bible size). What x86 expects is 16 bytes at the end of
> > > > > > 1Mb physical address space that the CPU runs at first.
> > > > >
> > > > > So you do not know what you are talking about, am I correct?
> > > >
> > > > Let me comment on this provocative question later, after some other
> > > > comments first.
> > > >
> > > > > Starting
> > > > > from 386 the first instruction is executed at 0xFFFFFFF0h. What you
> > > > > are referring to is the 8086 reset vector and that was like 40 years
> > > > > ago.
> > > >
> > > > True. The idea is the same, It has a reset vector standard for x86
> > > > (which doesn't explicitly tell what is there). So, nothing new or
> > > > different here.
> > > >
> > > > > Please refer to SDM volume 3A, chapter 9, section 9.1.4 "First
> > > > > Instruction Executed", paragraph two. Just watch out for the hex
> > > > > number train starting with FFFFF... then you will find it. This is
> > > > > what requires the memory range to be mapped. Modern Intel CPUs require
> > > > > larger portions, because of the ACM loading and XuCode and whatnot.
> > > >
> > > > Thanks. Have you read 9.7 and 9.8, btw?
> > > > Where does it tell anything about memory to be mapped to a certain
> > > > address, except the last up to 16 bytes?
> > >
> > > It doesn't, except that the FIT, ACM, BootGuard, XuCode stuff rely on
> > > their binaries to be there, this just sets the upper address limit of
> > > the window.
> >
> > Why is it needed? I mean the listed blobs are not mandatory to get
> > system boot. Is this correct?
>
> That doesn't matter for this case.

Then why did you mention them?

> > > > > Please refer to the email [1] from me linked below where I reference
> > > > > all PCH datasheets of the x64 era to prove that 16MB are mapped
> > > > > hard-wired. Note that the range cannot be turned off and will read
> > > > > back 0xFF's if the PCH registers are configured to not be backed by
> > > > > the actual SPI flash contents.
> > > >
> > > > And as I said it does not cover _all_ x86 designs (usual != all) .
> > > > Have you heard about Intel MID line of SoCs? Do you know that they
> > >
> > > No and a quick search didn't turn up anything. Can you point me to
> > > resources about those SoCs? Also my module is targeting x86_64, that
> > > is only a subset of x86 designs.
> >
> > They are x86_32 and x86_64, so in the category you listed.
> >
> > Unfortunately there is indeed not much publicly available information,
> > but I can tell you that from a design perspective you may consider
> > them PCH-less.
>
> Okay fine. Now you come around the corner with undocumented Intel
> devices and make your first semi-valid point.

Huh?!
You simply have the wrong assumption (see (1) and (2) above) and
_this_ is my point. And it seems you still can't admit that. Be brave!

> Why did it take you so
> long?

Same question to you.

>  Why did you seemingly just try to derail the discussion before?

See just above. I don't like when people are blind with their
brilliant solutions.

> Is the documentation non-existent or just NDA?

Of course documentation exists. Otherwise it wouldn't be possible to
program or do something about the chips.

> May I remind you that from a CPU view it doesn't matter if the PCH or
> some other thing attached to the CPU's bus provides the mapping.

Exactly my (another) point!

> So if that is your concern, then the solution would be to probe if we
> see a PCH or FCH?

If you can do that...

Bottom line, the proposed solution can't be accepted because it
appeals to the false assumptions besides other technical issues which
have been pointed out by the others.

> > > > have no SPI NOR and the firmware is located on eMMC? Do you know that
> > > > they can run Linux?
> > >
> > > It doesn't matter where the firmware is coming from, as long as it is
> > > _mapped_.
> >
> > It's not. That address space is full of devices, the same memory can't
> > be used for ROM and devices at the same time (I won't go to the weird
> > concept of different read and write paths, and it's not applicable
> > here anyway).
> >
> > > And something has to be mapped there, even if it is just a
> > > loader that gets eMMC going.
> >
> > (Semi-)wrong. Yes, _something_ is there (with the holes), but it's
> > _not_ a firmware memory.
> >
> > You may google for iomem output on that kind of device.
> >
> > For your convenience (an excerpt):
> >
> >   fee00000-fee00fff : Local APIC
> >    fee00000-fee00fff : Reserved
> >  ff000000-ffffffff : Reserved
> >    ff008000-ff008fff : 0000:00:0c.0
> >      ff008000-ff008fff : 0000:00:0c.0
> >    ff009000-ff009fff : 0000:00:13.0
> >      ff009000-ff009fff : intel_scu_ipc
> >
> > > > So, maybe it's you who do not know what you are talking about, am I correct?
> > > >
> > > > > [1] https://lkml.org/lkml/2021/6/24/379
> >
> > --
> > With Best Regards,
> > Andy Shevchenko



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Hans-Gert Dahmen]

Am Do., 11. Nov. 2021 um 17:45 Uhr schrieb Andy Shevchenko
<andy.shevchenko@gmail.com>:
>
> On Thu, Nov 11, 2021 at 6:07 PM Hans-Gert Dahmen
> <hans-gert.dahmen@immu.ne> wrote:
> > Am Do., 11. Nov. 2021 um 16:31 Uhr schrieb Andy Shevchenko
> > <andy.shevchenko@gmail.com>:
> > > On Thu, Nov 11, 2021 at 4:33 PM Hans-Gert Dahmen
> > > <hans-gert.dahmen@immu.ne> wrote:
> > > > Am Do., 11. Nov. 2021 um 14:55 Uhr schrieb Andy Shevchenko
> > > > <andy.shevchenko@gmail.com>:
> > > > > On Thu, Nov 11, 2021 at 2:56 PM Hans-Gert Dahmen
> > > > > <hans-gert.dahmen@immu.ne> wrote:
> > > > > > Am Do., 11. Nov. 2021 um 13:46 Uhr schrieb Andy Shevchenko
> > > > > > <andy.shevchenko@gmail.com>:
> > > > > > > On Thu, Nov 11, 2021 at 1:46 PM Richard Hughes <hughsient@gmail.com> wrote:
> > > > > > > > On Thu, 11 Nov 2021 at 10:33, Mika Westerberg
> > > > > > > > <mika.westerberg@linux.intel.com> wrote:
> > > > > > >
> > > > > > > > it's always going to work on x64 -- if the system firmware isn't available at that offset then the platform just isn't going to boot.
> > > > > > >
> > > > > > > Well, it's _usual_ case, but in general the assumption is simply
> > > > > > > incorrect. Btw, have you checked it on Coreboot enabled platforms?
> > > > > > > What about bare metal configurations where the bootloader provides
> > > > > > > services to the OS?
> > > > > >
> > > > > > No it is always the case. I suggest you go read your own Intel specs
> > > > > > and datasheets
> > > > >
> > > > > Point me out, please, chapters in SDM (I never really read it in full,
> > > > > it's kinda 10x Bible size). What x86 expects is 16 bytes at the end of
> > > > > 1Mb physical address space that the CPU runs at first.
> > > >
> > > > So you do not know what you are talking about, am I correct?
> > >
> > > Let me comment on this provocative question later, after some other
> > > comments first.
> > >
> > > > Starting
> > > > from 386 the first instruction is executed at 0xFFFFFFF0h. What you
> > > > are referring to is the 8086 reset vector and that was like 40 years
> > > > ago.
> > >
> > > True. The idea is the same, It has a reset vector standard for x86
> > > (which doesn't explicitly tell what is there). So, nothing new or
> > > different here.
> > >
> > > > Please refer to SDM volume 3A, chapter 9, section 9.1.4 "First
> > > > Instruction Executed", paragraph two. Just watch out for the hex
> > > > number train starting with FFFFF... then you will find it. This is
> > > > what requires the memory range to be mapped. Modern Intel CPUs require
> > > > larger portions, because of the ACM loading and XuCode and whatnot.
> > >
> > > Thanks. Have you read 9.7 and 9.8, btw?
> > > Where does it tell anything about memory to be mapped to a certain
> > > address, except the last up to 16 bytes?
> > >
> >
> > It doesn't, except that the FIT, ACM, BootGuard, XuCode stuff rely on
> > their binaries to be there, this just sets the upper address limit of
> > the window.
>
> Why is it needed? I mean the listed blobs are not mandatory to get
> system boot. Is this correct?

That doesn't matter for this case.

>
> > > > Please refer to the email [1] from me linked below where I reference
> > > > all PCH datasheets of the x64 era to prove that 16MB are mapped
> > > > hard-wired. Note that the range cannot be turned off and will read
> > > > back 0xFF's if the PCH registers are configured to not be backed by
> > > > the actual SPI flash contents.
> > >
> > > And as I said it does not cover _all_ x86 designs (usual != all) .
> > > Have you heard about Intel MID line of SoCs? Do you know that they
> >
> > No and a quick search didn't turn up anything. Can you point me to
> > resources about those SoCs? Also my module is targeting x86_64, that
> > is only a subset of x86 designs.
>
> They are x86_32 and x86_64, so in the category you listed.
>
> Unfortunately there is indeed not much publicly available information,
> but I can tell you that from a design perspective you may consider
> them PCH-less.

Okay fine. Now you come around the corner with undocumented Intel
devices and make your first semi-valid point. Why did it take you so
long? Why did you seemingly just try to derail the discussion before?
Is the documentation non-existent or just NDA?
May I remind you that from a CPU view it doesn't matter if the PCH or
some other thing attached to the CPU's bus provides the mapping.

So if that is your concern, then the solution would be to probe if we
see a PCH or FCH?


>
> > > have no SPI NOR and the firmware is located on eMMC? Do you know that
> > > they can run Linux?
> >
> > It doesn't matter where the firmware is coming from, as long as it is
> > _mapped_.
>
> It's not. That address space is full of devices, the same memory can't
> be used for ROM and devices at the same time (I won't go to the weird
> concept of different read and write paths, and it's not applicable
> here anyway).
>
> > And something has to be mapped there, even if it is just a
> > loader that gets eMMC going.
>
> (Semi-)wrong. Yes, _something_ is there (with the holes), but it's
> _not_ a firmware memory.
>
> You may google for iomem output on that kind of device.
>
> For your convenience (an excerpt):
>
>   fee00000-fee00fff : Local APIC
>    fee00000-fee00fff : Reserved
>  ff000000-ffffffff : Reserved
>    ff008000-ff008fff : 0000:00:0c.0
>      ff008000-ff008fff : 0000:00:0c.0
>    ff009000-ff009fff : 0000:00:13.0
>      ff009000-ff009fff : intel_scu_ipc
>
> > > So, maybe it's you who do not know what you are talking about, am I correct?
> > >
> > > > [1] https://lkml.org/lkml/2021/6/24/379
>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Andy Shevchenko]

On Thu, Nov 11, 2021 at 6:07 PM Hans-Gert Dahmen
<hans-gert.dahmen@immu.ne> wrote:
> Am Do., 11. Nov. 2021 um 16:31 Uhr schrieb Andy Shevchenko
> <andy.shevchenko@gmail.com>:
> > On Thu, Nov 11, 2021 at 4:33 PM Hans-Gert Dahmen
> > <hans-gert.dahmen@immu.ne> wrote:
> > > Am Do., 11. Nov. 2021 um 14:55 Uhr schrieb Andy Shevchenko
> > > <andy.shevchenko@gmail.com>:
> > > > On Thu, Nov 11, 2021 at 2:56 PM Hans-Gert Dahmen
> > > > <hans-gert.dahmen@immu.ne> wrote:
> > > > > Am Do., 11. Nov. 2021 um 13:46 Uhr schrieb Andy Shevchenko
> > > > > <andy.shevchenko@gmail.com>:
> > > > > > On Thu, Nov 11, 2021 at 1:46 PM Richard Hughes <hughsient@gmail.com> wrote:
> > > > > > > On Thu, 11 Nov 2021 at 10:33, Mika Westerberg
> > > > > > > <mika.westerberg@linux.intel.com> wrote:
> > > > > >
> > > > > > > it's always going to work on x64 -- if the system firmware isn't available at that offset then the platform just isn't going to boot.
> > > > > >
> > > > > > Well, it's _usual_ case, but in general the assumption is simply
> > > > > > incorrect. Btw, have you checked it on Coreboot enabled platforms?
> > > > > > What about bare metal configurations where the bootloader provides
> > > > > > services to the OS?
> > > > >
> > > > > No it is always the case. I suggest you go read your own Intel specs
> > > > > and datasheets
> > > >
> > > > Point me out, please, chapters in SDM (I never really read it in full,
> > > > it's kinda 10x Bible size). What x86 expects is 16 bytes at the end of
> > > > 1Mb physical address space that the CPU runs at first.
> > >
> > > So you do not know what you are talking about, am I correct?
> >
> > Let me comment on this provocative question later, after some other
> > comments first.
> >
> > > Starting
> > > from 386 the first instruction is executed at 0xFFFFFFF0h. What you
> > > are referring to is the 8086 reset vector and that was like 40 years
> > > ago.
> >
> > True. The idea is the same, It has a reset vector standard for x86
> > (which doesn't explicitly tell what is there). So, nothing new or
> > different here.
> >
> > > Please refer to SDM volume 3A, chapter 9, section 9.1.4 "First
> > > Instruction Executed", paragraph two. Just watch out for the hex
> > > number train starting with FFFFF... then you will find it. This is
> > > what requires the memory range to be mapped. Modern Intel CPUs require
> > > larger portions, because of the ACM loading and XuCode and whatnot.
> >
> > Thanks. Have you read 9.7 and 9.8, btw?
> > Where does it tell anything about memory to be mapped to a certain
> > address, except the last up to 16 bytes?
> >
>
> It doesn't, except that the FIT, ACM, BootGuard, XuCode stuff rely on
> their binaries to be there, this just sets the upper address limit of
> the window.

Why is it needed? I mean the listed blobs are not mandatory to get
system boot. Is this correct?

> > > Please refer to the email [1] from me linked below where I reference
> > > all PCH datasheets of the x64 era to prove that 16MB are mapped
> > > hard-wired. Note that the range cannot be turned off and will read
> > > back 0xFF's if the PCH registers are configured to not be backed by
> > > the actual SPI flash contents.
> >
> > And as I said it does not cover _all_ x86 designs (usual != all) .
> > Have you heard about Intel MID line of SoCs? Do you know that they
>
> No and a quick search didn't turn up anything. Can you point me to
> resources about those SoCs? Also my module is targeting x86_64, that
> is only a subset of x86 designs.

They are x86_32 and x86_64, so in the category you listed.

Unfortunately there is indeed not much publicly available information,
but I can tell you that from a design perspective you may consider
them PCH-less.

> > have no SPI NOR and the firmware is located on eMMC? Do you know that
> > they can run Linux?
>
> It doesn't matter where the firmware is coming from, as long as it is
> _mapped_.

It's not. That address space is full of devices, the same memory can't
be used for ROM and devices at the same time (I won't go to the weird
concept of different read and write paths, and it's not applicable
here anyway).

> And something has to be mapped there, even if it is just a
> loader that gets eMMC going.

(Semi-)wrong. Yes, _something_ is there (with the holes), but it's
_not_ a firmware memory.

You may google for iomem output on that kind of device.

For your convenience (an excerpt):

  fee00000-fee00fff : Local APIC
   fee00000-fee00fff : Reserved
 ff000000-ffffffff : Reserved
   ff008000-ff008fff : 0000:00:0c.0
     ff008000-ff008fff : 0000:00:0c.0
   ff009000-ff009fff : 0000:00:13.0
     ff009000-ff009fff : intel_scu_ipc

> > So, maybe it's you who do not know what you are talking about, am I correct?
> >
> > > [1] https://lkml.org/lkml/2021/6/24/379

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Hans-Gert Dahmen]

Am Do., 11. Nov. 2021 um 16:31 Uhr schrieb Andy Shevchenko
<andy.shevchenko@gmail.com>:
>
> On Thu, Nov 11, 2021 at 4:33 PM Hans-Gert Dahmen
> <hans-gert.dahmen@immu.ne> wrote:
> > Am Do., 11. Nov. 2021 um 14:55 Uhr schrieb Andy Shevchenko
> > <andy.shevchenko@gmail.com>:
> > > On Thu, Nov 11, 2021 at 2:56 PM Hans-Gert Dahmen
> > > <hans-gert.dahmen@immu.ne> wrote:
> > > > Am Do., 11. Nov. 2021 um 13:46 Uhr schrieb Andy Shevchenko
> > > > <andy.shevchenko@gmail.com>:
> > > > > On Thu, Nov 11, 2021 at 1:46 PM Richard Hughes <hughsient@gmail.com> wrote:
> > > > > > On Thu, 11 Nov 2021 at 10:33, Mika Westerberg
> > > > > > <mika.westerberg@linux.intel.com> wrote:
> > > > >
> > > > > > it's always going to work on x64 -- if the system firmware isn't available at that offset then the platform just isn't going to boot.
> > > > >
> > > > > Well, it's _usual_ case, but in general the assumption is simply
> > > > > incorrect. Btw, have you checked it on Coreboot enabled platforms?
> > > > > What about bare metal configurations where the bootloader provides
> > > > > services to the OS?
> > > >
> > > > No it is always the case. I suggest you go read your own Intel specs
> > > > and datasheets
> > >
> > > Point me out, please, chapters in SDM (I never really read it in full,
> > > it's kinda 10x Bible size). What x86 expects is 16 bytes at the end of
> > > 1Mb physical address space that the CPU runs at first.
> >
> > So you do not know what you are talking about, am I correct?
>
> Let me comment on this provocative question later, after some other
> comments first.
>
> > Starting
> > from 386 the first instruction is executed at 0xFFFFFFF0h. What you
> > are referring to is the 8086 reset vector and that was like 40 years
> > ago.
>
> True. The idea is the same, It has a reset vector standard for x86
> (which doesn't explicitly tell what is there). So, nothing new or
> different here.
>
> > Please refer to SDM volume 3A, chapter 9, section 9.1.4 "First
> > Instruction Executed", paragraph two. Just watch out for the hex
> > number train starting with FFFFF... then you will find it. This is
> > what requires the memory range to be mapped. Modern Intel CPUs require
> > larger portions, because of the ACM loading and XuCode and whatnot.
>
> Thanks. Have you read 9.7 and 9.8, btw?
> Where does it tell anything about memory to be mapped to a certain
> address, except the last up to 16 bytes?
>

It doesn't, except that the FIT, ACM, BootGuard, XuCode stuff rely on
their binaries to be there, this just sets the upper address limit of
the window.

> > Please refer to the email [1] from me linked below where I reference
> > all PCH datasheets of the x64 era to prove that 16MB are mapped
> > hard-wired. Note that the range cannot be turned off and will read
> > back 0xFF's if the PCH registers are configured to not be backed by
> > the actual SPI flash contents.
>
> And as I said it does not cover _all_ x86 designs (usual != all) .
> Have you heard about Intel MID line of SoCs? Do you know that they

No and a quick search didn't turn up anything. Can you point me to
resources about those SoCs? Also my module is targeting x86_64, that
is only a subset of x86 designs.

> have no SPI NOR and the firmware is located on eMMC? Do you know that
> they can run Linux?

It doesn't matter where the firmware is coming from, as long as it is
_mapped_. And something has to be mapped there, even if it is just a
loader that gets eMMC going.

>
> So, maybe it's you who do not know what you are talking about, am I correct?
>
> > [1] https://lkml.org/lkml/2021/6/24/379
>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Hans-Gert Dahmen]

Am Do., 11. Nov. 2021 um 16:50 Uhr schrieb Andy Shevchenko

<andy.shevchenko@gmail.com>:
>
> On Thu, Nov 11, 2021 at 5:43 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > On Thu, 11 Nov 2021 at 16:31, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > On Thu, Nov 11, 2021 at 4:33 PM Hans-Gert Dahmen
> > > <hans-gert.dahmen@immu.ne> wrote:
> > > > Am Do., 11. Nov. 2021 um 14:55 Uhr schrieb Andy Shevchenko
> > > > <andy.shevchenko@gmail.com>:
> > > > > On Thu, Nov 11, 2021 at 2:56 PM Hans-Gert Dahmen
> > > > > <hans-gert.dahmen@immu.ne> wrote:
> > > > > > Am Do., 11. Nov. 2021 um 13:46 Uhr schrieb Andy Shevchenko
> > > > > > <andy.shevchenko@gmail.com>:
> > > > > > > On Thu, Nov 11, 2021 at 1:46 PM Richard Hughes <hughsient@gmail.com> wrote:
> > > > > > > > On Thu, 11 Nov 2021 at 10:33, Mika Westerberg
> > > > > > > > <mika.westerberg@linux.intel.com> wrote:
> > > > > > >
> > > > > > > > it's always going to work on x64 -- if the system firmware isn't available at that offset then the platform just isn't going to boot.
> > > > > > >
> > > > > > > Well, it's _usual_ case, but in general the assumption is simply
> > > > > > > incorrect. Btw, have you checked it on Coreboot enabled platforms?
> > > > > > > What about bare metal configurations where the bootloader provides
> > > > > > > services to the OS?
> > > > > >
> > > > > > No it is always the case. I suggest you go read your own Intel specs
> > > > > > and datasheets
> > > > >
> > > > > Point me out, please, chapters in SDM (I never really read it in full,
> > > > > it's kinda 10x Bible size). What x86 expects is 16 bytes at the end of
> > > > > 1Mb physical address space that the CPU runs at first.
> > > >
> > > > So you do not know what you are talking about, am I correct?
> > >
> > > Let me comment on this provocative question later, after some other
> > > comments first.
> > >
> > > > Starting
> > > > from 386 the first instruction is executed at 0xFFFFFFF0h. What you
> > > > are referring to is the 8086 reset vector and that was like 40 years
> > > > ago.
> > >
> > > True. The idea is the same, It has a reset vector standard for x86
> > > (which doesn't explicitly tell what is there). So, nothing new or
> > > different here.
> > >
> > > > Please refer to SDM volume 3A, chapter 9, section 9.1.4 "First
> > > > Instruction Executed", paragraph two. Just watch out for the hex
> > > > number train starting with FFFFF... then you will find it. This is
> > > > what requires the memory range to be mapped. Modern Intel CPUs require
> > > > larger portions, because of the ACM loading and XuCode and whatnot.
> > >
> > > Thanks. Have you read 9.7 and 9.8, btw?
> > > Where does it tell anything about memory to be mapped to a certain
> > > address, except the last up to 16 bytes?
> > >
> > > > Please refer to the email [1] from me linked below where I reference
> > > > all PCH datasheets of the x64 era to prove that 16MB are mapped
> > > > hard-wired. Note that the range cannot be turned off and will read
> > > > back 0xFF's if the PCH registers are configured to not be backed by
> > > > the actual SPI flash contents.
> > >
> > > And as I said it does not cover _all_ x86 designs (usual != all) .
> > > Have you heard about Intel MID line of SoCs? Do you know that they
> > > have no SPI NOR and the firmware is located on eMMC? Do you know that
> > > they can run Linux?
> > >
> > > So, maybe it's you who do not know what you are talking about, am I correct?
> > >
> > > > [1] https://lkml.org/lkml/2021/6/24/379
> >
> > Thanks for looping me in (I think ...)
>
> Thank you for chiming in!
>
> > The thing I don't like about exposing the entire SPI NOR region to
> > user space is that we can never take it back, given the 'never break
> > user space' rule. So once we ship this, the cat is out of the bag, and
> > somebody (which != the contributors of this code) will have to
> > maintain this forever.
> >
> > Also, you quoted several different use cases, all of which are
> > currently served by exposing a chunk of PA space, and letting the user
> > do the interpretation. This is not how it usually works: we tend to
> > prefer targeted and maintainable interfaces. That woudl mean that,
> > e.g., fwupd can invoke some kind of syscall to get at the version
> > numbers it is after, and the logic that finds those numbers is in the
> > kernel and not in user space.
>
> I was thinking about SHA256 hashes or so (as they tell about
> binaries). In any case the interface for this seems to be in the
> kernel.
>
> It is also possible to do the other way around, i.e. piping binary to
> the kernel and wait for the answer if it is the same or not or...
>
> > For the pen testing use case, things are likely a bit different, so I
> > realize this is not universally applicable, but just exposing the PA
> > space directly is not the solution IMO.
>

All of this doesn't really sound like it is more maintainable. It
requires the base use case, being able to read the BIOS/UEFI binary
plus more code and a more complicated interface. So if you do this,
there's more cats out of the bag.
Also please be aware that not everything is UEFI and you would
probably need to support things like CoreBoot as well.

>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Andy Shevchenko]

On Thu, Nov 11, 2021 at 5:43 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> On Thu, 11 Nov 2021 at 16:31, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Thu, Nov 11, 2021 at 4:33 PM Hans-Gert Dahmen
> > <hans-gert.dahmen@immu.ne> wrote:
> > > Am Do., 11. Nov. 2021 um 14:55 Uhr schrieb Andy Shevchenko
> > > <andy.shevchenko@gmail.com>:
> > > > On Thu, Nov 11, 2021 at 2:56 PM Hans-Gert Dahmen
> > > > <hans-gert.dahmen@immu.ne> wrote:
> > > > > Am Do., 11. Nov. 2021 um 13:46 Uhr schrieb Andy Shevchenko
> > > > > <andy.shevchenko@gmail.com>:
> > > > > > On Thu, Nov 11, 2021 at 1:46 PM Richard Hughes <hughsient@gmail.com> wrote:
> > > > > > > On Thu, 11 Nov 2021 at 10:33, Mika Westerberg
> > > > > > > <mika.westerberg@linux.intel.com> wrote:
> > > > > >
> > > > > > > it's always going to work on x64 -- if the system firmware isn't available at that offset then the platform just isn't going to boot.
> > > > > >
> > > > > > Well, it's _usual_ case, but in general the assumption is simply
> > > > > > incorrect. Btw, have you checked it on Coreboot enabled platforms?
> > > > > > What about bare metal configurations where the bootloader provides
> > > > > > services to the OS?
> > > > >
> > > > > No it is always the case. I suggest you go read your own Intel specs
> > > > > and datasheets
> > > >
> > > > Point me out, please, chapters in SDM (I never really read it in full,
> > > > it's kinda 10x Bible size). What x86 expects is 16 bytes at the end of
> > > > 1Mb physical address space that the CPU runs at first.
> > >
> > > So you do not know what you are talking about, am I correct?
> >
> > Let me comment on this provocative question later, after some other
> > comments first.
> >
> > > Starting
> > > from 386 the first instruction is executed at 0xFFFFFFF0h. What you
> > > are referring to is the 8086 reset vector and that was like 40 years
> > > ago.
> >
> > True. The idea is the same, It has a reset vector standard for x86
> > (which doesn't explicitly tell what is there). So, nothing new or
> > different here.
> >
> > > Please refer to SDM volume 3A, chapter 9, section 9.1.4 "First
> > > Instruction Executed", paragraph two. Just watch out for the hex
> > > number train starting with FFFFF... then you will find it. This is
> > > what requires the memory range to be mapped. Modern Intel CPUs require
> > > larger portions, because of the ACM loading and XuCode and whatnot.
> >
> > Thanks. Have you read 9.7 and 9.8, btw?
> > Where does it tell anything about memory to be mapped to a certain
> > address, except the last up to 16 bytes?
> >
> > > Please refer to the email [1] from me linked below where I reference
> > > all PCH datasheets of the x64 era to prove that 16MB are mapped
> > > hard-wired. Note that the range cannot be turned off and will read
> > > back 0xFF's if the PCH registers are configured to not be backed by
> > > the actual SPI flash contents.
> >
> > And as I said it does not cover _all_ x86 designs (usual != all) .
> > Have you heard about Intel MID line of SoCs? Do you know that they
> > have no SPI NOR and the firmware is located on eMMC? Do you know that
> > they can run Linux?
> >
> > So, maybe it's you who do not know what you are talking about, am I correct?
> >
> > > [1] https://lkml.org/lkml/2021/6/24/379
>
> Thanks for looping me in (I think ...)

Thank you for chiming in!

> The thing I don't like about exposing the entire SPI NOR region to
> user space is that we can never take it back, given the 'never break
> user space' rule. So once we ship this, the cat is out of the bag, and
> somebody (which != the contributors of this code) will have to
> maintain this forever.
>
> Also, you quoted several different use cases, all of which are
> currently served by exposing a chunk of PA space, and letting the user
> do the interpretation. This is not how it usually works: we tend to
> prefer targeted and maintainable interfaces. That woudl mean that,
> e.g., fwupd can invoke some kind of syscall to get at the version
> numbers it is after, and the logic that finds those numbers is in the
> kernel and not in user space.

I was thinking about SHA256 hashes or so (as they tell about
binaries). In any case the interface for this seems to be in the
kernel.

It is also possible to do the other way around, i.e. piping binary to
the kernel and wait for the answer if it is the same or not or...

> For the pen testing use case, things are likely a bit different, so I
> realize this is not universally applicable, but just exposing the PA
> space directly is not the solution IMO.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Ard Biesheuvel]

On Thu, 11 Nov 2021 at 16:31, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> On Thu, Nov 11, 2021 at 4:33 PM Hans-Gert Dahmen
> <hans-gert.dahmen@immu.ne> wrote:
> > Am Do., 11. Nov. 2021 um 14:55 Uhr schrieb Andy Shevchenko
> > <andy.shevchenko@gmail.com>:
> > > On Thu, Nov 11, 2021 at 2:56 PM Hans-Gert Dahmen
> > > <hans-gert.dahmen@immu.ne> wrote:
> > > > Am Do., 11. Nov. 2021 um 13:46 Uhr schrieb Andy Shevchenko
> > > > <andy.shevchenko@gmail.com>:
> > > > > On Thu, Nov 11, 2021 at 1:46 PM Richard Hughes <hughsient@gmail.com> wrote:
> > > > > > On Thu, 11 Nov 2021 at 10:33, Mika Westerberg
> > > > > > <mika.westerberg@linux.intel.com> wrote:
> > > > >
> > > > > > it's always going to work on x64 -- if the system firmware isn't available at that offset then the platform just isn't going to boot.
> > > > >
> > > > > Well, it's _usual_ case, but in general the assumption is simply
> > > > > incorrect. Btw, have you checked it on Coreboot enabled platforms?
> > > > > What about bare metal configurations where the bootloader provides
> > > > > services to the OS?
> > > >
> > > > No it is always the case. I suggest you go read your own Intel specs
> > > > and datasheets
> > >
> > > Point me out, please, chapters in SDM (I never really read it in full,
> > > it's kinda 10x Bible size). What x86 expects is 16 bytes at the end of
> > > 1Mb physical address space that the CPU runs at first.
> >
> > So you do not know what you are talking about, am I correct?
>
> Let me comment on this provocative question later, after some other
> comments first.
>
> > Starting
> > from 386 the first instruction is executed at 0xFFFFFFF0h. What you
> > are referring to is the 8086 reset vector and that was like 40 years
> > ago.
>
> True. The idea is the same, It has a reset vector standard for x86
> (which doesn't explicitly tell what is there). So, nothing new or
> different here.
>
> > Please refer to SDM volume 3A, chapter 9, section 9.1.4 "First
> > Instruction Executed", paragraph two. Just watch out for the hex
> > number train starting with FFFFF... then you will find it. This is
> > what requires the memory range to be mapped. Modern Intel CPUs require
> > larger portions, because of the ACM loading and XuCode and whatnot.
>
> Thanks. Have you read 9.7 and 9.8, btw?
> Where does it tell anything about memory to be mapped to a certain
> address, except the last up to 16 bytes?
>
> > Please refer to the email [1] from me linked below where I reference
> > all PCH datasheets of the x64 era to prove that 16MB are mapped
> > hard-wired. Note that the range cannot be turned off and will read
> > back 0xFF's if the PCH registers are configured to not be backed by
> > the actual SPI flash contents.
>
> And as I said it does not cover _all_ x86 designs (usual != all) .
> Have you heard about Intel MID line of SoCs? Do you know that they
> have no SPI NOR and the firmware is located on eMMC? Do you know that
> they can run Linux?
>
> So, maybe it's you who do not know what you are talking about, am I correct?
>
> > [1] https://lkml.org/lkml/2021/6/24/379
>

Thanks for looping me in (I think ...)

The thing I don't like about exposing the entire SPI NOR region to
user space is that we can never take it back, given the 'never break
user space' rule. So once we ship this, the cat is out of the bag, and
somebody (which != the contributors of this code) will have to
maintain this forever.

Also, you quoted several different use cases, all of which are
currently served by exposing a chunk of PA space, and letting the user
do the interpretation. This is not how it usually works: we tend to
prefer targeted and maintainable interfaces. That woudl mean that,
e.g., fwupd can invoke some kind of syscall to get at the version
numbers it is after, and the logic that finds those numbers is in the
kernel and not in user space.

For the pen testing use case, things are likely a bit different, so I
realize this is not universally applicable, but just exposing the PA
space directly is not the solution IMO.

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Mauro Lima]

On Thu, Nov 11, 2021 at 12:06 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> Hi,
>
> On Thu, Nov 11, 2021 at 11:42:52AM -0300, Mauro Lima wrote:
> > > > > Having said that the hardware sequencer used in the recent CPUs should
> > > > > be much safer in that sense.
> > > >
> > > > FWIW, I'd be fine if we had RO access for HWSEQ flash access only. If
> > > > I understood correctly that's what Mauro proposed (with a patch) and
> > > > instead was told that it was being rewritten as a mtd driver
> > > > completion time unknown.
> > >
> > > I think Mauro proposed something different, basically exposing RO parts
> > > of the driver only.
> >
> > My patch was intended to move the read functionality of the spi chip
> > to be able to compile the driver with just that and then remove the
> > dangerous tag. So we can use that functionality to read the flash, I'm
> > missing what is different from the things being discussed here sorry.
>
> I'm hinting that we could make this "non-DANGEROUS" for hardware
> sequencer parts of the driver. Basically moving only the software
> sequencer bits as DANGEROUS or something like that. The hardware
> sequencer is much more safer because it does not allow to run random
> opcodes.

I'm aware about hw and sw sequencer diffs, my patch aimed to split
reading functionality because we thought that the issue was related
with write/erase ops plus the writable module param (this mixed with
the sw sequencer could be the bug?). I totally agree with the hw
sequencer path and I'm willing to help to make this happen.

> In case someone is unfamiliar with this, the Intel SPI hardware
> exposes two interfaces through the same controller. One that is called
> software sequencer and there is a register of "allowed" opcodes that
> software can use as it wishes. This register can be locked down but is
> not always. The second interface is the hardware sequencer that only
> exposes higher level commands like read, write and so on and internally
> then executes whatever opcode the controller got from the chip
> "supported opcodes table" (SFDP).  The recent Intel hardware, all
> big-cores, only provide hardware sequencer and the software one is not
> even available.
>
> Regardless of all this the driver needs to be converted from MTD to SPI
> (SPI MEM) before we can add any features. I'm planning to send v4 of
> that series next week.

Good luck with the patch.

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Andy Shevchenko]

On Thu, Nov 11, 2021 at 4:33 PM Hans-Gert Dahmen
<hans-gert.dahmen@immu.ne> wrote:
> Am Do., 11. Nov. 2021 um 14:55 Uhr schrieb Andy Shevchenko
> <andy.shevchenko@gmail.com>:
> > On Thu, Nov 11, 2021 at 2:56 PM Hans-Gert Dahmen
> > <hans-gert.dahmen@immu.ne> wrote:
> > > Am Do., 11. Nov. 2021 um 13:46 Uhr schrieb Andy Shevchenko
> > > <andy.shevchenko@gmail.com>:
> > > > On Thu, Nov 11, 2021 at 1:46 PM Richard Hughes <hughsient@gmail.com> wrote:
> > > > > On Thu, 11 Nov 2021 at 10:33, Mika Westerberg
> > > > > <mika.westerberg@linux.intel.com> wrote:
> > > >
> > > > > it's always going to work on x64 -- if the system firmware isn't available at that offset then the platform just isn't going to boot.
> > > >
> > > > Well, it's _usual_ case, but in general the assumption is simply
> > > > incorrect. Btw, have you checked it on Coreboot enabled platforms?
> > > > What about bare metal configurations where the bootloader provides
> > > > services to the OS?
> > >
> > > No it is always the case. I suggest you go read your own Intel specs
> > > and datasheets
> >
> > Point me out, please, chapters in SDM (I never really read it in full,
> > it's kinda 10x Bible size). What x86 expects is 16 bytes at the end of
> > 1Mb physical address space that the CPU runs at first.
>
> So you do not know what you are talking about, am I correct?

Let me comment on this provocative question later, after some other
comments first.

> Starting
> from 386 the first instruction is executed at 0xFFFFFFF0h. What you
> are referring to is the 8086 reset vector and that was like 40 years
> ago.

True. The idea is the same, It has a reset vector standard for x86
(which doesn't explicitly tell what is there). So, nothing new or
different here.

> Please refer to SDM volume 3A, chapter 9, section 9.1.4 "First
> Instruction Executed", paragraph two. Just watch out for the hex
> number train starting with FFFFF... then you will find it. This is
> what requires the memory range to be mapped. Modern Intel CPUs require
> larger portions, because of the ACM loading and XuCode and whatnot.

Thanks. Have you read 9.7 and 9.8, btw?
Where does it tell anything about memory to be mapped to a certain
address, except the last up to 16 bytes?

> Please refer to the email [1] from me linked below where I reference
> all PCH datasheets of the x64 era to prove that 16MB are mapped
> hard-wired. Note that the range cannot be turned off and will read
> back 0xFF's if the PCH registers are configured to not be backed by
> the actual SPI flash contents.

And as I said it does not cover _all_ x86 designs (usual != all) .
Have you heard about Intel MID line of SoCs? Do you know that they
have no SPI NOR and the firmware is located on eMMC? Do you know that
they can run Linux?

So, maybe it's you who do not know what you are talking about, am I correct?

> [1] https://lkml.org/lkml/2021/6/24/379

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Hans-Gert Dahmen]

Am Do., 11. Nov. 2021 um 16:06 Uhr schrieb Mika Westerberg
<mika.westerberg@linux.intel.com>:
>
> Hi,
>
> On Thu, Nov 11, 2021 at 11:42:52AM -0300, Mauro Lima wrote:
> > > > > Having said that the hardware sequencer used in the recent CPUs should
> > > > > be much safer in that sense.
> > > >
> > > > FWIW, I'd be fine if we had RO access for HWSEQ flash access only. If
> > > > I understood correctly that's what Mauro proposed (with a patch) and
> > > > instead was told that it was being rewritten as a mtd driver
> > > > completion time unknown.
> > >
> > > I think Mauro proposed something different, basically exposing RO parts
> > > of the driver only.
> >
> > My patch was intended to move the read functionality of the spi chip
> > to be able to compile the driver with just that and then remove the
> > dangerous tag. So we can use that functionality to read the flash, I'm
> > missing what is different from the things being discussed here sorry.
>
> I'm hinting that we could make this "non-DANGEROUS" for hardware
> sequencer parts of the driver. Basically moving only the software
> sequencer bits as DANGEROUS or something like that. The hardware
> sequencer is much more safer because it does not allow to run random
> opcodes.
>
> In case someone is unfamiliar with this, the Intel SPI hardware
> exposes two interfaces through the same controller. One that is called
> software sequencer and there is a register of "allowed" opcodes that
> software can use as it wishes. This register can be locked down but is
> not always. The second interface is the hardware sequencer that only
> exposes higher level commands like read, write and so on and internally
> then executes whatever opcode the controller got from the chip
> "supported opcodes table" (SFDP).  The recent Intel hardware, all
> big-cores, only provide hardware sequencer and the software one is not
> even available.
>

I am familiar with this and I do totally agree. I believe HW
sequencing is available since sandy-bridge from 2011, so it will
suffice for modern platforms. Honestly me and my developer friends
never understood why this driver needs to still focus on SW sequencing
altogether, it seems like a (possibly buggy) relic that just slows
down the vital parts. So I'd say it is a good idea to move the HW
sequencing parts, basically splitting it, but still add a RO/RW flag
to the module to be extra safe.

> Regardless of all this the driver needs to be converted from MTD to SPI
> (SPI MEM) before we can add any features. I'm planning to send v4 of
> that series next week.

This is an understandable reason and thank you for working on it.

Hans-Gert

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Mika Westerberg]

Hi,

On Thu, Nov 11, 2021 at 11:42:52AM -0300, Mauro Lima wrote:
> > > > Having said that the hardware sequencer used in the recent CPUs should
> > > > be much safer in that sense.
> > >
> > > FWIW, I'd be fine if we had RO access for HWSEQ flash access only. If
> > > I understood correctly that's what Mauro proposed (with a patch) and
> > > instead was told that it was being rewritten as a mtd driver
> > > completion time unknown.
> >
> > I think Mauro proposed something different, basically exposing RO parts
> > of the driver only.
> 
> My patch was intended to move the read functionality of the spi chip
> to be able to compile the driver with just that and then remove the
> dangerous tag. So we can use that functionality to read the flash, I'm
> missing what is different from the things being discussed here sorry.

I'm hinting that we could make this "non-DANGEROUS" for hardware
sequencer parts of the driver. Basically moving only the software
sequencer bits as DANGEROUS or something like that. The hardware
sequencer is much more safer because it does not allow to run random
opcodes.

In case someone is unfamiliar with this, the Intel SPI hardware
exposes two interfaces through the same controller. One that is called
software sequencer and there is a register of "allowed" opcodes that
software can use as it wishes. This register can be locked down but is
not always. The second interface is the hardware sequencer that only
exposes higher level commands like read, write and so on and internally
then executes whatever opcode the controller got from the chip
"supported opcodes table" (SFDP).  The recent Intel hardware, all
big-cores, only provide hardware sequencer and the software one is not
even available.

Regardless of all this the driver needs to be converted from MTD to SPI
(SPI MEM) before we can add any features. I'm planning to send v4 of
that series next week.

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Mauro Lima]

On Thu, Nov 11, 2021 at 10:34 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> On Thu, Nov 11, 2021 at 01:22:25PM +0000, Richard Hughes wrote:
> > On Thu, 11 Nov 2021 at 13:01, Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > > I'm not sure I understand why the platform security needs to be turned off?
> >
> > Sorry to be unclear, I meant we had to turn off Secure Boot (and thus
> > also kernel lockdown) so that we could use /dev/mem to verify that
> > OEMs have set up the IFD, BLE, BIOSWP etc correctly. You'd be
> > surprised just how many vendors don't read the specifications
> > correctly and get this wrong. We also need port IO to use the
> > intel-spi driver so we can parse the BIOS contents from userspace,
> > which you can't obviously do when SB is turned off. The eSPI
> > controller is hidden on some hardware now, and we need to play games
> > to make it visible again.
>
> Okay, thanks for explaining.
>
> > > I think exposing /dev/mem opens another can of worms that we want to
> > > avoid.
> >
> > Ohh it's not all of /dev/mem, it's just the 16MB BIOS region that has
> > to be mapped at that address for the hardware to boot.
>
> I see.
>
> > > Don't we already expose some of the EFI stuff under /sys/firmware?
> >
> > Yes, some information, but not the file volumes themselves. I don't
> > think the kernel wants to be in the game of supporting every nested
> > container and compression format that EFI supports. It's also the
> > wrong layer to expose platform attributes like BLE, but that's an
> > orthogonal thing to the patch Hans-Gert is proposing.
> >
> > > I just don't want to
> > > spend yet another Christmas holiday trying to fix angry peoples laptops :(
> >
> > Completely understood, I don't think any of us want that.
> >
> > > Having said that the hardware sequencer used in the recent CPUs should
> > > be much safer in that sense.
> >
> > FWIW, I'd be fine if we had RO access for HWSEQ flash access only. If
> > I understood correctly that's what Mauro proposed (with a patch) and
> > instead was told that it was being rewritten as a mtd driver
> > completion time unknown.
>
> I think Mauro proposed something different, basically exposing RO parts
> of the driver only.

My patch was intended to move the read functionality of the spi chip
to be able to compile the driver with just that and then remove the
dangerous tag. So we can use that functionality to read the flash, I'm
missing what is different from the things being discussed here sorry.

> The intel-spi driver is being moved from MTD to SPI because the MTD
> SPI-NOR maintainers (not me) said that it needs to be done before we can
> add any new feature to the driver. That includes also Mauro's patch.
>
> I have v4 of the conversion patch series done already but since it is a
> middle of the merge window I'm holding it until v5.16-rc1 is released
> (next sunday). I can CC you too and I suppose Hans and Mauro (who else,
> let me know). Once the MTD maintainers are happy we can progress adding
> features what fwupd needs there too (and the features we, Intel, want to
> add there).

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Hans-Gert Dahmen]

Am Do., 11. Nov. 2021 um 14:55 Uhr schrieb Andy Shevchenko
<andy.shevchenko@gmail.com>:
>
> On Thu, Nov 11, 2021 at 2:56 PM Hans-Gert Dahmen
> <hans-gert.dahmen@immu.ne> wrote:
> >
> > Am Do., 11. Nov. 2021 um 13:46 Uhr schrieb Andy Shevchenko
> > <andy.shevchenko@gmail.com>:
> > >
> > > On Thu, Nov 11, 2021 at 1:46 PM Richard Hughes <hughsient@gmail.com> wrote:
> > > > On Thu, 11 Nov 2021 at 10:33, Mika Westerberg
> > > > <mika.westerberg@linux.intel.com> wrote:
> > >
> > > > it's always going to work on x64 -- if the system firmware isn't available at that offset then the platform just isn't going to boot.
> > >
> > > Well, it's _usual_ case, but in general the assumption is simply
> > > incorrect. Btw, have you checked it on Coreboot enabled platforms?
> > > What about bare metal configurations where the bootloader provides
> > > services to the OS?
> >
> > No it is always the case. I suggest you go read your own Intel specs
> > and datasheets
>
> Point me out, please, chapters in SDM (I never really read it in full,
> it's kinda 10x Bible size). What x86 expects is 16 bytes at the end of
> 1Mb physical address space that the CPU runs at first.

So you do not know what you are talking about, am I correct? Starting
from 386 the first instruction is executed at 0xFFFFFFF0h. What you
are referring to is the 8086 reset vector and that was like 40 years
ago.

Please refer to SDM volume 3A, chapter 9, section 9.1.4 "First
Instruction Executed", paragraph two. Just watch out for the hex
number train starting with FFFFF... then you will find it. This is
what requires the memory range to be mapped. Modern Intel CPUs require
larger portions, because of the ACM loading and XuCode and whatnot.
Please refer to the email [1] from me linked below where I reference
all PCH datasheets of the x64 era to prove that 16MB are mapped
hard-wired. Note that the range cannot be turned off and will read
back 0xFF's if the PCH registers are configured to not be backed by
the actual SPI flash contents.

[1] https://lkml.org/lkml/2021/6/24/379

>
> > before spreading further FUD. I have experienced u-root
> > and coreboot developers sitting right next to me in my office and they
> > were among the ones suggesting my patch. This is just laughable,
> > please stop it Andy.
>
> Yeah, zillion people can't ever make a mistake... I see.
>
> --
> With Best Regards,
> Andy Shevchenko

Hans-Gert

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Andy Shevchenko]

On Thu, Nov 11, 2021 at 2:56 PM Hans-Gert Dahmen
<hans-gert.dahmen@immu.ne> wrote:
>
> Am Do., 11. Nov. 2021 um 13:46 Uhr schrieb Andy Shevchenko
> <andy.shevchenko@gmail.com>:
> >
> > On Thu, Nov 11, 2021 at 1:46 PM Richard Hughes <hughsient@gmail.com> wrote:
> > > On Thu, 11 Nov 2021 at 10:33, Mika Westerberg
> > > <mika.westerberg@linux.intel.com> wrote:
> >
> > > it's always going to work on x64 -- if the system firmware isn't available at that offset then the platform just isn't going to boot.
> >
> > Well, it's _usual_ case, but in general the assumption is simply
> > incorrect. Btw, have you checked it on Coreboot enabled platforms?
> > What about bare metal configurations where the bootloader provides
> > services to the OS?
>
> No it is always the case. I suggest you go read your own Intel specs
> and datasheets

Point me out, please, chapters in SDM (I never really read it in full,
it's kinda 10x Bible size). What x86 expects is 16 bytes at the end of
1Mb physical address space that the CPU runs at first.

> before spreading further FUD. I have experienced u-root
> and coreboot developers sitting right next to me in my office and they
> were among the ones suggesting my patch. This is just laughable,
> please stop it Andy.

Yeah, zillion people can't ever make a mistake... I see.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Hans-Gert Dahmen]

Am Do., 11. Nov. 2021 um 14:34 Uhr schrieb Mika Westerberg
<mika.westerberg@linux.intel.com>:
>
> On Thu, Nov 11, 2021 at 01:22:25PM +0000, Richard Hughes wrote:
> > On Thu, 11 Nov 2021 at 13:01, Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > > I'm not sure I understand why the platform security needs to be turned off?
> >
> > Sorry to be unclear, I meant we had to turn off Secure Boot (and thus
> > also kernel lockdown) so that we could use /dev/mem to verify that
> > OEMs have set up the IFD, BLE, BIOSWP etc correctly. You'd be
> > surprised just how many vendors don't read the specifications
> > correctly and get this wrong. We also need port IO to use the
> > intel-spi driver so we can parse the BIOS contents from userspace,
> > which you can't obviously do when SB is turned off. The eSPI
> > controller is hidden on some hardware now, and we need to play games
> > to make it visible again.
>
> Okay, thanks for explaining.
>
> > > I think exposing /dev/mem opens another can of worms that we want to
> > > avoid.
> >
> > Ohh it's not all of /dev/mem, it's just the 16MB BIOS region that has
> > to be mapped at that address for the hardware to boot.
>
> I see.
>
> > > Don't we already expose some of the EFI stuff under /sys/firmware?
> >
> > Yes, some information, but not the file volumes themselves. I don't
> > think the kernel wants to be in the game of supporting every nested
> > container and compression format that EFI supports. It's also the
> > wrong layer to expose platform attributes like BLE, but that's an
> > orthogonal thing to the patch Hans-Gert is proposing.
> >
> > > I just don't want to
> > > spend yet another Christmas holiday trying to fix angry peoples laptops :(
> >
> > Completely understood, I don't think any of us want that.
> >
> > > Having said that the hardware sequencer used in the recent CPUs should
> > > be much safer in that sense.
> >
> > FWIW, I'd be fine if we had RO access for HWSEQ flash access only. If
> > I understood correctly that's what Mauro proposed (with a patch) and
> > instead was told that it was being rewritten as a mtd driver
> > completion time unknown.
>
> I think Mauro proposed something different, basically exposing RO parts
> of the driver only.
>
> The intel-spi driver is being moved from MTD to SPI because the MTD
> SPI-NOR maintainers (not me) said that it needs to be done before we can
> add any new feature to the driver. That includes also Mauro's patch.
>
> I have v4 of the conversion patch series done already but since it is a
> middle of the merge window I'm holding it until v5.16-rc1 is released
> (next sunday). I can CC you too and I suppose Hans and Mauro (who else,

I'd be delighted.

> let me know). Once the MTD maintainers are happy we can progress adding
> features what fwupd needs there too (and the features we, Intel, want to
> add there).

Hans-Gert

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Mika Westerberg]

On Thu, Nov 11, 2021 at 01:22:25PM +0000, Richard Hughes wrote:
> On Thu, 11 Nov 2021 at 13:01, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > I'm not sure I understand why the platform security needs to be turned off?
> 
> Sorry to be unclear, I meant we had to turn off Secure Boot (and thus
> also kernel lockdown) so that we could use /dev/mem to verify that
> OEMs have set up the IFD, BLE, BIOSWP etc correctly. You'd be
> surprised just how many vendors don't read the specifications
> correctly and get this wrong. We also need port IO to use the
> intel-spi driver so we can parse the BIOS contents from userspace,
> which you can't obviously do when SB is turned off. The eSPI
> controller is hidden on some hardware now, and we need to play games
> to make it visible again.

Okay, thanks for explaining.

> > I think exposing /dev/mem opens another can of worms that we want to
> > avoid.
> 
> Ohh it's not all of /dev/mem, it's just the 16MB BIOS region that has
> to be mapped at that address for the hardware to boot.

I see.

> > Don't we already expose some of the EFI stuff under /sys/firmware?
> 
> Yes, some information, but not the file volumes themselves. I don't
> think the kernel wants to be in the game of supporting every nested
> container and compression format that EFI supports. It's also the
> wrong layer to expose platform attributes like BLE, but that's an
> orthogonal thing to the patch Hans-Gert is proposing.
> 
> > I just don't want to
> > spend yet another Christmas holiday trying to fix angry peoples laptops :(
> 
> Completely understood, I don't think any of us want that.
> 
> > Having said that the hardware sequencer used in the recent CPUs should
> > be much safer in that sense.
> 
> FWIW, I'd be fine if we had RO access for HWSEQ flash access only. If
> I understood correctly that's what Mauro proposed (with a patch) and
> instead was told that it was being rewritten as a mtd driver
> completion time unknown.

I think Mauro proposed something different, basically exposing RO parts
of the driver only.

The intel-spi driver is being moved from MTD to SPI because the MTD
SPI-NOR maintainers (not me) said that it needs to be done before we can
add any new feature to the driver. That includes also Mauro's patch.

I have v4 of the conversion patch series done already but since it is a
middle of the merge window I'm holding it until v5.16-rc1 is released
(next sunday). I can CC you too and I suppose Hans and Mauro (who else,
let me know). Once the MTD maintainers are happy we can progress adding
features what fwupd needs there too (and the features we, Intel, want to
add there).

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Richard Hughes]

On Thu, 11 Nov 2021 at 13:01, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> I'm not sure I understand why the platform security needs to be turned off?

Sorry to be unclear, I meant we had to turn off Secure Boot (and thus
also kernel lockdown) so that we could use /dev/mem to verify that
OEMs have set up the IFD, BLE, BIOSWP etc correctly. You'd be
surprised just how many vendors don't read the specifications
correctly and get this wrong. We also need port IO to use the
intel-spi driver so we can parse the BIOS contents from userspace,
which you can't obviously do when SB is turned off. The eSPI
controller is hidden on some hardware now, and we need to play games
to make it visible again.

> I think exposing /dev/mem opens another can of worms that we want to
> avoid.

Ohh it's not all of /dev/mem, it's just the 16MB BIOS region that has
to be mapped at that address for the hardware to boot.

> Don't we already expose some of the EFI stuff under /sys/firmware?

Yes, some information, but not the file volumes themselves. I don't
think the kernel wants to be in the game of supporting every nested
container and compression format that EFI supports. It's also the
wrong layer to expose platform attributes like BLE, but that's an
orthogonal thing to the patch Hans-Gert is proposing.

> I just don't want to
> spend yet another Christmas holiday trying to fix angry peoples laptops :(

Completely understood, I don't think any of us want that.

> Having said that the hardware sequencer used in the recent CPUs should
> be much safer in that sense.

FWIW, I'd be fine if we had RO access for HWSEQ flash access only. If
I understood correctly that's what Mauro proposed (with a patch) and
instead was told that it was being rewritten as a mtd driver
completion time unknown.

Richard.

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Mika Westerberg]

On Thu, Nov 11, 2021 at 11:46:39AM +0000, Richard Hughes wrote:
> On Thu, 11 Nov 2021 at 10:33, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > OK, I see from your patch that it uses the direct mapped read-only
> > region to read this data.
> 
> Sorry for the delay in replying here. What I like about Hans-Gert's
> patch is that it's always going to work on x64 -- if the system
> firmware isn't available at that offset then the platform just isn't
> going to boot. It's super simple, and means we can read out the hugely
> complex UEFI blob without asking the user to turn off kernel lockdown
> and secure boot so we can run the security verification tools. At the
> moment we're in this strange situation where we ask the user to
> cripple their platform security to run the platform security tools.

I'm not sure I understand why the platform security needs to be turned
off? Sorry if that was already mentioned somewhere in the thread, I did
not read all of them throughly.

> > Do we know what information exactly fwupd needs? I mean exposing all of
> > this might not be good idea from security perspective (but I'm not an
> > expert).
> 
> Ideally, fwupd needs the entire IFD partition which contains all the
> EFI File Volumes. We already parse these when the user is booting
> without secure boot using the Intel SPI controller and doing *evil*
> hacks to make the PCI device visible. The reason we want to parse the
> BIOS can be explained pretty easily; at startup we look at the TPM
> PCRs and we know very quickly and easily if the system firmware event
> has changed. If the checksum changed, then the firmware was modified
> in some way. However, saying to the user that "checksum changed" isn't
> useful at all. What we want to do is say something like "an EFI binary
> called RootKitz.efi was added" or "the AmiTcgPlatformPeiAfterMem.efi
> binary changed" and actually report what was done. At the moment we
> can do this, but not if /dev/mem cannot be used.

I think exposing /dev/mem opens another can of worms that we want to
avoid.

Is there anything preventing to add a sane interface from say the
intel-spi driver (can be something else too) that exposes the minimal
set you need? That can be in the mainline kernel and is secure (and
safe) enough that distros (and users) can enable it?

Don't we already expose some of the EFI stuff under /sys/firmware?

> > However, we can perhaps expose some of it through intel-spi,
> > and make that work so that distros can enable it safely.
> 
> I think, if we're being honest, that Intel has no plans to make
> intel-spi available as a RO interface of any sort. There's some sort
> of hardware errata or misdesign that nobody can mention that makes the
> RO access unsafe. I think it's probably more than missing arbitration.

No that's not the reason. The real reason I'm concerned (as the author
and maintainer of that thing) is that last time the driver was
accidentally enabled in Ubuntu (around 2019) there was a driver bug
(which was fixed already but it was not yet in the Ubuntu kernel) that
turned some Lenovo based systems' BIOSes to read-only. It has nothing to
do with my employer or hardware errata/misdesign. I just don't want to
spend yet another Christmas holiday trying to fix angry peoples laptops :(

Having said that the hardware sequencer used in the recent CPUs should
be much safer in that sense. It should not allow things like this to
happen (the affected systems used software based sequencer).

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Hans-Gert Dahmen]

Am Do., 11. Nov. 2021 um 13:46 Uhr schrieb Andy Shevchenko
<andy.shevchenko@gmail.com>:
>
> On Thu, Nov 11, 2021 at 1:46 PM Richard Hughes <hughsient@gmail.com> wrote:
> > On Thu, 11 Nov 2021 at 10:33, Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
>
> > it's always going to work on x64 -- if the system firmware isn't available at that offset then the platform just isn't going to boot.
>
> Well, it's _usual_ case, but in general the assumption is simply
> incorrect. Btw, have you checked it on Coreboot enabled platforms?
> What about bare metal configurations where the bootloader provides
> services to the OS?

No it is always the case. I suggest you go read your own Intel specs
and datasheets before spreading further FUD. I have experienced u-root
and coreboot developers sitting right next to me in my office and they
were among the ones suggesting my patch. This is just laughable,
please stop it Andy.

>
> > Ideally, fwupd needs the entire IFD partition which contains all the
> > EFI File Volumes.
>
> Well, can't it be part of the EFI subsystem in the kernel then? (+Ard)
>
> > We already parse these when the user is booting
> > without secure boot using the Intel SPI controller and doing *evil*
> > hacks to make the PCI device visible. The reason we want to parse the
> > BIOS can be explained pretty easily; at startup we look at the TPM
> > PCRs and we know very quickly and easily if the system firmware event
> > has changed. If the checksum changed, then the firmware was modified
> > in some way. However, saying to the user that "checksum changed" isn't
> > useful at all. What we want to do is say something like "an EFI binary
> > called RootKitz.efi was added" or "the AmiTcgPlatformPeiAfterMem.efi
> > binary changed" and actually report what was done. At the moment we
> > can do this, but not if /dev/mem cannot be used.
> >
> > > However, we can perhaps expose some of it through intel-spi,
> > > and make that work so that distros can enable it safely.
> >
> > I think, if we're being honest, that Intel has no plans to make
> > intel-spi available as a RO interface of any sort. There's some sort
> > of hardware errata or misdesign that nobody can mention that makes the
> > RO access unsafe. I think it's probably more than missing arbitration.
>
>
>
> --
> With Best Regards,
> Andy Shevchenko

Hans-Gert

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Andy Shevchenko]

On Thu, Nov 11, 2021 at 1:46 PM Richard Hughes <hughsient@gmail.com> wrote:
> On Thu, 11 Nov 2021 at 10:33, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:

> it's always going to work on x64 -- if the system firmware isn't available at that offset then the platform just isn't going to boot.

Well, it's _usual_ case, but in general the assumption is simply
incorrect. Btw, have you checked it on Coreboot enabled platforms?
What about bare metal configurations where the bootloader provides
services to the OS?

> Ideally, fwupd needs the entire IFD partition which contains all the
> EFI File Volumes.

Well, can't it be part of the EFI subsystem in the kernel then? (+Ard)

> We already parse these when the user is booting
> without secure boot using the Intel SPI controller and doing *evil*
> hacks to make the PCI device visible. The reason we want to parse the
> BIOS can be explained pretty easily; at startup we look at the TPM
> PCRs and we know very quickly and easily if the system firmware event
> has changed. If the checksum changed, then the firmware was modified
> in some way. However, saying to the user that "checksum changed" isn't
> useful at all. What we want to do is say something like "an EFI binary
> called RootKitz.efi was added" or "the AmiTcgPlatformPeiAfterMem.efi
> binary changed" and actually report what was done. At the moment we
> can do this, but not if /dev/mem cannot be used.
>
> > However, we can perhaps expose some of it through intel-spi,
> > and make that work so that distros can enable it safely.
>
> I think, if we're being honest, that Intel has no plans to make
> intel-spi available as a RO interface of any sort. There's some sort
> of hardware errata or misdesign that nobody can mention that makes the
> RO access unsafe. I think it's probably more than missing arbitration.



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Mauro Lima]

Hi Mika

On Thu, Nov 11, 2021 at 3:42 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> Hi,
>
> On Wed, Nov 10, 2021 at 02:37:56PM -0300, Mauro Lima wrote:
> > > Try again to collaborate with Intel SPI driver author(s) /
> > > maintainer(s) by sending the proposal that may work.
> >
> > The proposal I sent makes the driver work only with read ops, but that
> > was not the issue with this driver. The issue was something related to
> > a status register and this was fixed. So if there is no issue with
> > write/erase ops, the bug was fixed and there is no intention to remove
> > the tag. What kind of proposal are we talking about?
>
> I think we discussed this previously already but in any case, instead of
> removing the tag from the "main" driver, we can make certain "safe"
> parts of the driver available without that tag. That would allow you to
> read the things the controller exposes and allow distros safely include
> the driver too. By "safe" parts, I mean the information available
> through the SPI flash controller without actually sending commands to
> the flash chip. I think this is the information everybody (on this
> thread at least) is interested in. Unless I'm mistaken - I did not check
> the original patch. If that's not enough we can possibly expand it to
> cover the controllers that only use hardware sequencer since they
> operate on a certain limited set of ops that should not break anything
> accidentally.

We discussed this previously indeed, and you are right that we are
interested in the spi configuration, but we are also interested in
reading the bios. For that we needed to use the driver, if that
expansion you are talking about, gives me the possibility to use the
driver for reading the flash and having this functionality without any
dangerous tag, it would be awesome.

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Richard Hughes]

On Thu, 11 Nov 2021 at 10:33, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> OK, I see from your patch that it uses the direct mapped read-only
> region to read this data.

Sorry for the delay in replying here. What I like about Hans-Gert's
patch is that it's always going to work on x64 -- if the system
firmware isn't available at that offset then the platform just isn't
going to boot. It's super simple, and means we can read out the hugely
complex UEFI blob without asking the user to turn off kernel lockdown
and secure boot so we can run the security verification tools. At the
moment we're in this strange situation where we ask the user to
cripple their platform security to run the platform security tools.

> Do we know what information exactly fwupd needs? I mean exposing all of
> this might not be good idea from security perspective (but I'm not an
> expert).

Ideally, fwupd needs the entire IFD partition which contains all the
EFI File Volumes. We already parse these when the user is booting
without secure boot using the Intel SPI controller and doing *evil*
hacks to make the PCI device visible. The reason we want to parse the
BIOS can be explained pretty easily; at startup we look at the TPM
PCRs and we know very quickly and easily if the system firmware event
has changed. If the checksum changed, then the firmware was modified
in some way. However, saying to the user that "checksum changed" isn't
useful at all. What we want to do is say something like "an EFI binary
called RootKitz.efi was added" or "the AmiTcgPlatformPeiAfterMem.efi
binary changed" and actually report what was done. At the moment we
can do this, but not if /dev/mem cannot be used.

> However, we can perhaps expose some of it through intel-spi,
> and make that work so that distros can enable it safely.

I think, if we're being honest, that Intel has no plans to make
intel-spi available as a RO interface of any sort. There's some sort
of hardware errata or misdesign that nobody can mention that makes the
RO access unsafe. I think it's probably more than missing arbitration.

Richard

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Greg KH]

On Thu, Nov 11, 2021 at 11:55:25AM +0100, Hans-Gert Dahmen wrote:
> > My concern of
> > removing the DANGEROUS tag is that we end up bricking yet another Lenovo
> > laptop by accident. Avoiding that would give me more peace of mind :)
> 
> Yes of course, I think to not endanger users with malfunctions caused
> by code we produce is of our mutual interest here and we should make
> sure to be absolutely serious about it.

This is why I was saying that the driver HAS to specify the hardware
that it is allowed to bind to, and only bind to that hardware.  Your
driver today does not take that into account at all, which is my primary
objection to the code as-is.

thanks,

greg k-h

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Hans-Gert Dahmen]

Am Do., 11. Nov. 2021 um 11:33 Uhr schrieb Mika Westerberg
<mika.westerberg@linux.intel.com>:
>
> Hi,
>
> On Thu, Nov 11, 2021 at 09:59:32AM +0100, Hans-Gert Dahmen wrote:
> > > I think we discussed this previously already but in any case, instead of
> > > removing the tag from the "main" driver, we can make certain "safe"
> > > parts of the driver available without that tag. That would allow you to
> > > read the things the controller exposes and allow distros safely include
> > > the driver too. By "safe" parts, I mean the information available
> > > through the SPI flash controller without actually sending commands to
> > > the flash chip. I think this is the information everybody (on this
> > > thread at least) is interested in. Unless I'm mistaken - I did not check
> >
> > Yes you are mistaken. My patch is about safely reading the BIOS/UEFI
> > binary on every past and future x86_64 system. There are tools out
> > there that use the interface my patch uses and they can not work any
> > longer when /dev/mem is locked down with SecureBoot enabled. The
> > tools, like fwupd, should work out-of-the-box on the typical
> > distribution. During this discussion we were told that my patch is not
> > welcome and that we have to work with you to achieve the same. So I'm
> > curious to hear how that can be done.
>
> OK, I see from your patch that it uses the direct mapped read-only
> region to read this data.
>
> Do we know what information exactly fwupd needs?

I think Richard can tell us best and he should receive this mail.

> I mean exposing all of
> this might not be good idea from security perspective (but I'm not an
> expert)

There is no secret hidden in there and the binary is typically openly
shared by the vendor through update sites or mechanisms. Actually it
would be better from a security perspective to access all of it to be
able to compare it to the official versions and check it for malicious
modifications, which is becoming ever more relevant in the wake of
recent CVEs about supply-chain attacks. Doing so would be a possible
use beyond fwupd and I am specifically speaking about the open source
converged security suite by 9elements.

> However, we can perhaps expose some of it through intel-spi,
> and make that work so that distros can enable it safely.

That would really be great and I'd already like to thank you for being
open about going down this path. I think Mauro is willing to
collaborate on this front, correct me if I am wrong.

> My concern of
> removing the DANGEROUS tag is that we end up bricking yet another Lenovo
> laptop by accident. Avoiding that would give me more peace of mind :)

Yes of course, I think to not endanger users with malfunctions caused
by code we produce is of our mutual interest here and we should make
sure to be absolutely serious about it.

Hans-Gert

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Mika Westerberg]

Hi,

On Thu, Nov 11, 2021 at 09:59:32AM +0100, Hans-Gert Dahmen wrote:
> > I think we discussed this previously already but in any case, instead of
> > removing the tag from the "main" driver, we can make certain "safe"
> > parts of the driver available without that tag. That would allow you to
> > read the things the controller exposes and allow distros safely include
> > the driver too. By "safe" parts, I mean the information available
> > through the SPI flash controller without actually sending commands to
> > the flash chip. I think this is the information everybody (on this
> > thread at least) is interested in. Unless I'm mistaken - I did not check
> 
> Yes you are mistaken. My patch is about safely reading the BIOS/UEFI
> binary on every past and future x86_64 system. There are tools out
> there that use the interface my patch uses and they can not work any
> longer when /dev/mem is locked down with SecureBoot enabled. The
> tools, like fwupd, should work out-of-the-box on the typical
> distribution. During this discussion we were told that my patch is not
> welcome and that we have to work with you to achieve the same. So I'm
> curious to hear how that can be done.

OK, I see from your patch that it uses the direct mapped read-only
region to read this data.

Do we know what information exactly fwupd needs? I mean exposing all of
this might not be good idea from security perspective (but I'm not an
expert). However, we can perhaps expose some of it through intel-spi,
and make that work so that distros can enable it safely. My concern of
removing the DANGEROUS tag is that we end up bricking yet another Lenovo
laptop by accident. Avoiding that would give me more peace of mind :)

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Hans-Gert Dahmen]

Am Do., 11. Nov. 2021 um 07:42 Uhr schrieb Mika Westerberg
<mika.westerberg@linux.intel.com>:
>
> Hi,
>
> On Wed, Nov 10, 2021 at 02:37:56PM -0300, Mauro Lima wrote:
> > > Try again to collaborate with Intel SPI driver author(s) /
> > > maintainer(s) by sending the proposal that may work.
> >
> > The proposal I sent makes the driver work only with read ops, but that
> > was not the issue with this driver. The issue was something related to
> > a status register and this was fixed. So if there is no issue with
> > write/erase ops, the bug was fixed and there is no intention to remove
> > the tag. What kind of proposal are we talking about?
>
> I think we discussed this previously already but in any case, instead of
> removing the tag from the "main" driver, we can make certain "safe"
> parts of the driver available without that tag. That would allow you to
> read the things the controller exposes and allow distros safely include
> the driver too. By "safe" parts, I mean the information available
> through the SPI flash controller without actually sending commands to
> the flash chip. I think this is the information everybody (on this
> thread at least) is interested in. Unless I'm mistaken - I did not check

Yes you are mistaken. My patch is about safely reading the BIOS/UEFI
binary on every past and future x86_64 system. There are tools out
there that use the interface my patch uses and they can not work any
longer when /dev/mem is locked down with SecureBoot enabled. The
tools, like fwupd, should work out-of-the-box on the typical
distribution. During this discussion we were told that my patch is not
welcome and that we have to work with you to achieve the same. So I'm
curious to hear how that can be done.

> the original patch. If that's not enough we can possibly expand it to
> cover the controllers that only use hardware sequencer since they
> operate on a certain limited set of ops that should not break anything
> accidentally.

Hans-Gert

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Mika Westerberg]

Hi,

On Wed, Nov 10, 2021 at 02:37:56PM -0300, Mauro Lima wrote:
> > Try again to collaborate with Intel SPI driver author(s) /
> > maintainer(s) by sending the proposal that may work.
> 
> The proposal I sent makes the driver work only with read ops, but that
> was not the issue with this driver. The issue was something related to
> a status register and this was fixed. So if there is no issue with
> write/erase ops, the bug was fixed and there is no intention to remove
> the tag. What kind of proposal are we talking about?

I think we discussed this previously already but in any case, instead of
removing the tag from the "main" driver, we can make certain "safe"
parts of the driver available without that tag. That would allow you to
read the things the controller exposes and allow distros safely include
the driver too. By "safe" parts, I mean the information available
through the SPI flash controller without actually sending commands to
the flash chip. I think this is the information everybody (on this
thread at least) is interested in. Unless I'm mistaken - I did not check
the original patch. If that's not enough we can possibly expand it to
cover the controllers that only use hardware sequencer since they
operate on a certain limited set of ops that should not break anything
accidentally.

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Hans-Gert Dahmen]

Am Mi., 10. Nov. 2021 um 17:32 Uhr schrieb Andy Shevchenko
<andy.shevchenko@gmail.com>:
> Why do we see this instead?

I personally do not need this patch to be in the kernel. This is my
first ever submission to the LKML and all I was trying to do is do
other open source tools a favor in my spare time during my HOLIDAY. At
least for me that is it. After this horrible experience you can be
sure to never ever see anything again from me in the Linux community.

Hans-Gert

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Mauro Lima]

On Wed, Nov 10, 2021 at 1:32 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Nov 10, 2021 at 3:13 PM Mauro Lima <mauro.lima@eclypsium.com> wrote:
> >
> > On Wed, Nov 10, 2021 at 7:00 AM Hans-Gert Dahmen
> > <hans-gert.dahmen@immu.ne> wrote:
> > >
> > > Am Mi., 10. Nov. 2021 um 10:25 Uhr schrieb Andy Shevchenko
> > > <andy.shevchenko@gmail.com>:
> > > >
> > > > On Wed, Nov 10, 2021 at 11:17 AM Hans-Gert Dahmen
> > > > <hans-gert.dahmen@immu.ne> wrote:
> > > > > Am Mi., 10. Nov. 2021 um 10:05 Uhr schrieb Andy Shevchenko
> > > > > <andy.shevchenko@gmail.com>:
> > > > > > On Wed, Nov 10, 2021 at 10:37 AM Hans-Gert Dahmen
> > > > > > <hans-gert.dahmen@immu.ne> wrote:
> > > > > > > Am Mi., 10. Nov. 2021 um 07:35 Uhr schrieb Andy Shevchenko
> > > > > > > <andy.shevchenko@gmail.com>:
> > > > > > > > On Tuesday, November 9, 2021, Hans-Gert Dahmen <hans-gert.dahmen@immu.ne> wrote:
> > > > > > > >> Am Di., 9. Nov. 2021 um 13:42 Uhr schrieb Greg KH <gregkh@linuxfoundation.org>:
> > > > > >
> > > > > > > >> > Do you have a pointer to these complex and buggy drivers anywhere?
> > > > > > > >>
> > > > > > > >> I am talking about the linux-mtd intel-spi driver for example, but I
> > > > > > > >> feel that this gets the discussion in the wrong direction.
> > > > > > > >
> > > > > > > > This is the driver that covers all BIOSes on modern x86\64. What’s wrong with it? Why do you need this?!
> > > > > > > >
> > > > > > > > If it’s buggy, where is the bug reports from you or somebody else?
> > > > > > >
> > > > > > > Please see Mauro's mail in this thread from yesterday below:
> > > > > >
> > > > > > I didn't get this. What's wrong with the response from the author of
> > > > > > intel-spi (since we are speaking about it, let me add him to the
> > > > > > thread)?
> > > > > > What you are trying to do is to sneak around with ugly hack behind the
> > > > > > proper subsystem's back.
> > > > > >
> > > > > > NAK as I already said.
> > > > >
> > > > > There is nothing wrong with the response. Also we are not trying to
> > > > > sneak anything into the kernel. This just is no mtd or spi driver, it
> > > > > is not even a driver. What this does is it opens back up a portion of
> > > > > memory that can not be read anymore through /dev/mem on locked down
> > > > > systems with SecureBoot enabled. This portion of memory is actively
> > > > > being used by userspace programs. We, 9elements, Eclypsium, fwudp and
> > > > > immune are among those who rely upon this to improve the security of
> > > > > x86_64 linux devices.
> > > >
> > > > I appreciate this.
> > > >
> > > > x86_64 starting from long time ago has more or less standard hardware
> > > > interface for BIOS chip, i.e. SPI NOR. PCH usually has a separate host
> > > > controller and we have the driver in the kernel for that (coverage of
> > > > the systems may not be 100%, but close enough). Now you are trying to
> > > > repeat something that is _already_ provided by the kernel. Correct?
> > >
> > > Yes and no. We are not repeating the functionality of the SPI driver
> > > because we don't read nor write the flash specifically. What we do, is
> > > we make the boot vector readable by userspace when it is not
> > > accessible by /dev/mem. It happens to be a portion of the SPI flash
> > > but that doesn't have to be the case because the required view upon io
> > > memory here is CPU-centric and the rest of the system could be built
> > > in a way that backs that memory window differently. However, this is
> > > more or less hypothetical if you ignore some probably never-used bits
> > > that can be set in PCH registers. As I said, effectively we are trying
> > > to partially revert the lockdown on /dev/mem to be able to do a
> > > harmless read on an important memory range. From that point of view we
> > > are trying to keep functionality intact. The interface being used here
> > > has not changed a bit since 15 years and it probably will stay that
> > > way. In contrast to the intel-spi driver this will likely cover more
> > > future and past systems in different use-cases with fewer lines of
> > > code and next to no maintenance.
> > >
> > > >
> > > > > Now what happens is that more distros start to
> > > > > lock down their kernels and require signed modules. With the mtd
> > > > > driver not working on those machine to read the bios binary, you are
> > > > > effectively forcing users into less secure system configurations (i.e.
> > > > > opening up the whole /dev/mem and/or disabling SecureBoot) just to be
> > > > > able to run fwupd or other tools to assess firmware information. The
> > > > > issue of being able to assess and compare the bios binary to the one
> > > > > publicly available from the vendors is increasingly getting important
> > > > > in the wake of recent CVEs about supply-chain attacks where UEFI
> > > > > malware was pre-installed. So we are not even doing anything new here,
> > > > > you are just making life harder for everybody.
> > > >
> > > > Why me? As far as I got it from above the bottleneck is the distros
> > > > which do not enable the driver. So why not go and work with them?
> > > >
> > >
> > > Oh come on,  the distros have no choice here. Either not provide a
> > > more secure locked down system or allow a dangerous driver where
> > > patches to make it less dangerous are not welcome. And even if the
> > > latter could be solved, the history clearly is that it is in no way
> > > not even remotely as reliable as the memory region my patch is
> > > relaying to userspace.
> >
> > As Hans already said, it is a no go to put drivers tagged as
> > _(DANGEROUS)_ within the kernel distro. In this thread [1] from the
> > latest changes to the intel-spi driver I asked if there is any reason
> > to keep the dangerous tag or if there is any work we can do to achieve
> > this. As long as this tag is in the driver, we can't use the driver.
> >
> > [1] https://lore.kernel.org/all/CAArk9MPWh4f1E=ecKBHy8PFzvU_y_ROgDyUU_O3ZQ0FuMhkj5Q@mail.gmail.com/
>
> From [1]:
>   "> IMHO we can make certain parts of the driver, that are known not to
>   > cause any issues available without the DANGEROUS. I mean the controller
>   > exposes some information that I think you are intersted in and that does
>   > not cause anything to be sent to the flash chip itself.
>   This will work for me."
>
> And?.. Where is your proposal to modify the driver the way maintainer
> and you will be happy? Why do we see this instead? Try again to
> collaborate with Intel SPI driver author(s) / maintainer(s) by sending
> the proposal that may work.

I talked with Mika and now I'm working on a way to read BIOSWE, BIOSLE
and FLOCKDN values from the chip through pci (not using intel-spi
driver), you will see it in near future. Unfortunately this is only to
see if the vendor did the homework and set the flags correctly before
the spi leaves the factory. This is still useful to see if the write
protection mechanism it's in place.
We are still interested in a way to get the BIOS, for this the first
way you think of is reading it using the spi but after this (also from
[1] )
> I don't think we want to remove that. This driver is not for regular
> users, at least in its current form.
It seems that is not an option to use it.

> Why do we see this instead?

This is not my patch.

> Try again to collaborate with Intel SPI driver author(s) / maintainer(s) by sending
> the proposal that may work.

The proposal I sent makes the driver work only with read ops, but that
was not the issue with this driver. The issue was something related to
a status register and this was fixed. So if there is no issue with
write/erase ops, the bug was fixed and there is no intention to remove
the tag. What kind of proposal are we talking about?

> --
> With Best Regards,
> Andy Shevchenko

Thanks, Mauro.

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Andy Shevchenko]

On Wed, Nov 10, 2021 at 3:13 PM Mauro Lima <mauro.lima@eclypsium.com> wrote:
>
> On Wed, Nov 10, 2021 at 7:00 AM Hans-Gert Dahmen
> <hans-gert.dahmen@immu.ne> wrote:
> >
> > Am Mi., 10. Nov. 2021 um 10:25 Uhr schrieb Andy Shevchenko
> > <andy.shevchenko@gmail.com>:
> > >
> > > On Wed, Nov 10, 2021 at 11:17 AM Hans-Gert Dahmen
> > > <hans-gert.dahmen@immu.ne> wrote:
> > > > Am Mi., 10. Nov. 2021 um 10:05 Uhr schrieb Andy Shevchenko
> > > > <andy.shevchenko@gmail.com>:
> > > > > On Wed, Nov 10, 2021 at 10:37 AM Hans-Gert Dahmen
> > > > > <hans-gert.dahmen@immu.ne> wrote:
> > > > > > Am Mi., 10. Nov. 2021 um 07:35 Uhr schrieb Andy Shevchenko
> > > > > > <andy.shevchenko@gmail.com>:
> > > > > > > On Tuesday, November 9, 2021, Hans-Gert Dahmen <hans-gert.dahmen@immu.ne> wrote:
> > > > > > >> Am Di., 9. Nov. 2021 um 13:42 Uhr schrieb Greg KH <gregkh@linuxfoundation.org>:
> > > > >
> > > > > > >> > Do you have a pointer to these complex and buggy drivers anywhere?
> > > > > > >>
> > > > > > >> I am talking about the linux-mtd intel-spi driver for example, but I
> > > > > > >> feel that this gets the discussion in the wrong direction.
> > > > > > >
> > > > > > > This is the driver that covers all BIOSes on modern x86\64. What’s wrong with it? Why do you need this?!
> > > > > > >
> > > > > > > If it’s buggy, where is the bug reports from you or somebody else?
> > > > > >
> > > > > > Please see Mauro's mail in this thread from yesterday below:
> > > > >
> > > > > I didn't get this. What's wrong with the response from the author of
> > > > > intel-spi (since we are speaking about it, let me add him to the
> > > > > thread)?
> > > > > What you are trying to do is to sneak around with ugly hack behind the
> > > > > proper subsystem's back.
> > > > >
> > > > > NAK as I already said.
> > > >
> > > > There is nothing wrong with the response. Also we are not trying to
> > > > sneak anything into the kernel. This just is no mtd or spi driver, it
> > > > is not even a driver. What this does is it opens back up a portion of
> > > > memory that can not be read anymore through /dev/mem on locked down
> > > > systems with SecureBoot enabled. This portion of memory is actively
> > > > being used by userspace programs. We, 9elements, Eclypsium, fwudp and
> > > > immune are among those who rely upon this to improve the security of
> > > > x86_64 linux devices.
> > >
> > > I appreciate this.
> > >
> > > x86_64 starting from long time ago has more or less standard hardware
> > > interface for BIOS chip, i.e. SPI NOR. PCH usually has a separate host
> > > controller and we have the driver in the kernel for that (coverage of
> > > the systems may not be 100%, but close enough). Now you are trying to
> > > repeat something that is _already_ provided by the kernel. Correct?
> >
> > Yes and no. We are not repeating the functionality of the SPI driver
> > because we don't read nor write the flash specifically. What we do, is
> > we make the boot vector readable by userspace when it is not
> > accessible by /dev/mem. It happens to be a portion of the SPI flash
> > but that doesn't have to be the case because the required view upon io
> > memory here is CPU-centric and the rest of the system could be built
> > in a way that backs that memory window differently. However, this is
> > more or less hypothetical if you ignore some probably never-used bits
> > that can be set in PCH registers. As I said, effectively we are trying
> > to partially revert the lockdown on /dev/mem to be able to do a
> > harmless read on an important memory range. From that point of view we
> > are trying to keep functionality intact. The interface being used here
> > has not changed a bit since 15 years and it probably will stay that
> > way. In contrast to the intel-spi driver this will likely cover more
> > future and past systems in different use-cases with fewer lines of
> > code and next to no maintenance.
> >
> > >
> > > > Now what happens is that more distros start to
> > > > lock down their kernels and require signed modules. With the mtd
> > > > driver not working on those machine to read the bios binary, you are
> > > > effectively forcing users into less secure system configurations (i.e.
> > > > opening up the whole /dev/mem and/or disabling SecureBoot) just to be
> > > > able to run fwupd or other tools to assess firmware information. The
> > > > issue of being able to assess and compare the bios binary to the one
> > > > publicly available from the vendors is increasingly getting important
> > > > in the wake of recent CVEs about supply-chain attacks where UEFI
> > > > malware was pre-installed. So we are not even doing anything new here,
> > > > you are just making life harder for everybody.
> > >
> > > Why me? As far as I got it from above the bottleneck is the distros
> > > which do not enable the driver. So why not go and work with them?
> > >
> >
> > Oh come on,  the distros have no choice here. Either not provide a
> > more secure locked down system or allow a dangerous driver where
> > patches to make it less dangerous are not welcome. And even if the
> > latter could be solved, the history clearly is that it is in no way
> > not even remotely as reliable as the memory region my patch is
> > relaying to userspace.
>
> As Hans already said, it is a no go to put drivers tagged as
> _(DANGEROUS)_ within the kernel distro. In this thread [1] from the
> latest changes to the intel-spi driver I asked if there is any reason
> to keep the dangerous tag or if there is any work we can do to achieve
> this. As long as this tag is in the driver, we can't use the driver.
>
> [1] https://lore.kernel.org/all/CAArk9MPWh4f1E=ecKBHy8PFzvU_y_ROgDyUU_O3ZQ0FuMhkj5Q@mail.gmail.com/

From [1]:
  "> IMHO we can make certain parts of the driver, that are known not to
  > cause any issues available without the DANGEROUS. I mean the controller
  > exposes some information that I think you are intersted in and that does
  > not cause anything to be sent to the flash chip itself.
  This will work for me."

And?.. Where is your proposal to modify the driver the way maintainer
and you will be happy? Why do we see this instead? Try again to
collaborate with Intel SPI driver author(s) / maintainer(s) by sending
the proposal that may work.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Mauro Lima]

On Wed, Nov 10, 2021 at 7:00 AM Hans-Gert Dahmen
<hans-gert.dahmen@immu.ne> wrote:
>
> Am Mi., 10. Nov. 2021 um 10:25 Uhr schrieb Andy Shevchenko
> <andy.shevchenko@gmail.com>:
> >
> > On Wed, Nov 10, 2021 at 11:17 AM Hans-Gert Dahmen
> > <hans-gert.dahmen@immu.ne> wrote:
> > > Am Mi., 10. Nov. 2021 um 10:05 Uhr schrieb Andy Shevchenko
> > > <andy.shevchenko@gmail.com>:
> > > > On Wed, Nov 10, 2021 at 10:37 AM Hans-Gert Dahmen
> > > > <hans-gert.dahmen@immu.ne> wrote:
> > > > > Am Mi., 10. Nov. 2021 um 07:35 Uhr schrieb Andy Shevchenko
> > > > > <andy.shevchenko@gmail.com>:
> > > > > > On Tuesday, November 9, 2021, Hans-Gert Dahmen <hans-gert.dahmen@immu.ne> wrote:
> > > > > >> Am Di., 9. Nov. 2021 um 13:42 Uhr schrieb Greg KH <gregkh@linuxfoundation.org>:
> > > >
> > > > > >> > Do you have a pointer to these complex and buggy drivers anywhere?
> > > > > >>
> > > > > >> I am talking about the linux-mtd intel-spi driver for example, but I
> > > > > >> feel that this gets the discussion in the wrong direction.
> > > > > >
> > > > > > This is the driver that covers all BIOSes on modern x86\64. What’s wrong with it? Why do you need this?!
> > > > > >
> > > > > > If it’s buggy, where is the bug reports from you or somebody else?
> > > > >
> > > > > Please see Mauro's mail in this thread from yesterday below:
> > > >
> > > > I didn't get this. What's wrong with the response from the author of
> > > > intel-spi (since we are speaking about it, let me add him to the
> > > > thread)?
> > > > What you are trying to do is to sneak around with ugly hack behind the
> > > > proper subsystem's back.
> > > >
> > > > NAK as I already said.
> > >
> > > There is nothing wrong with the response. Also we are not trying to
> > > sneak anything into the kernel. This just is no mtd or spi driver, it
> > > is not even a driver. What this does is it opens back up a portion of
> > > memory that can not be read anymore through /dev/mem on locked down
> > > systems with SecureBoot enabled. This portion of memory is actively
> > > being used by userspace programs. We, 9elements, Eclypsium, fwudp and
> > > immune are among those who rely upon this to improve the security of
> > > x86_64 linux devices.
> >
> > I appreciate this.
> >
> > x86_64 starting from long time ago has more or less standard hardware
> > interface for BIOS chip, i.e. SPI NOR. PCH usually has a separate host
> > controller and we have the driver in the kernel for that (coverage of
> > the systems may not be 100%, but close enough). Now you are trying to
> > repeat something that is _already_ provided by the kernel. Correct?
>
> Yes and no. We are not repeating the functionality of the SPI driver
> because we don't read nor write the flash specifically. What we do, is
> we make the boot vector readable by userspace when it is not
> accessible by /dev/mem. It happens to be a portion of the SPI flash
> but that doesn't have to be the case because the required view upon io
> memory here is CPU-centric and the rest of the system could be built
> in a way that backs that memory window differently. However, this is
> more or less hypothetical if you ignore some probably never-used bits
> that can be set in PCH registers. As I said, effectively we are trying
> to partially revert the lockdown on /dev/mem to be able to do a
> harmless read on an important memory range. From that point of view we
> are trying to keep functionality intact. The interface being used here
> has not changed a bit since 15 years and it probably will stay that
> way. In contrast to the intel-spi driver this will likely cover more
> future and past systems in different use-cases with fewer lines of
> code and next to no maintenance.
>
> >
> > > Now what happens is that more distros start to
> > > lock down their kernels and require signed modules. With the mtd
> > > driver not working on those machine to read the bios binary, you are
> > > effectively forcing users into less secure system configurations (i.e.
> > > opening up the whole /dev/mem and/or disabling SecureBoot) just to be
> > > able to run fwupd or other tools to assess firmware information. The
> > > issue of being able to assess and compare the bios binary to the one
> > > publicly available from the vendors is increasingly getting important
> > > in the wake of recent CVEs about supply-chain attacks where UEFI
> > > malware was pre-installed. So we are not even doing anything new here,
> > > you are just making life harder for everybody.
> >
> > Why me? As far as I got it from above the bottleneck is the distros
> > which do not enable the driver. So why not go and work with them?
> >
>
> Oh come on,  the distros have no choice here. Either not provide a
> more secure locked down system or allow a dangerous driver where
> patches to make it less dangerous are not welcome. And even if the
> latter could be solved, the history clearly is that it is in no way
> not even remotely as reliable as the memory region my patch is
> relaying to userspace.

As Hans already said, it is a no go to put drivers tagged as
_(DANGEROUS)_ within the kernel distro. In this thread [1] from the
latest changes to the intel-spi driver I asked if there is any reason
to keep the dangerous tag or if there is any work we can do to achieve
this. As long as this tag is in the driver, we can't use the driver.

[1] https://lore.kernel.org/all/CAArk9MPWh4f1E=ecKBHy8PFzvU_y_ROgDyUU_O3ZQ0FuMhkj5Q@mail.gmail.com/

> Hans-Gert

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Hans-Gert Dahmen]

Am Mi., 10. Nov. 2021 um 10:25 Uhr schrieb Andy Shevchenko
<andy.shevchenko@gmail.com>:
>
> On Wed, Nov 10, 2021 at 11:17 AM Hans-Gert Dahmen
> <hans-gert.dahmen@immu.ne> wrote:
> > Am Mi., 10. Nov. 2021 um 10:05 Uhr schrieb Andy Shevchenko
> > <andy.shevchenko@gmail.com>:
> > > On Wed, Nov 10, 2021 at 10:37 AM Hans-Gert Dahmen
> > > <hans-gert.dahmen@immu.ne> wrote:
> > > > Am Mi., 10. Nov. 2021 um 07:35 Uhr schrieb Andy Shevchenko
> > > > <andy.shevchenko@gmail.com>:
> > > > > On Tuesday, November 9, 2021, Hans-Gert Dahmen <hans-gert.dahmen@immu.ne> wrote:
> > > > >> Am Di., 9. Nov. 2021 um 13:42 Uhr schrieb Greg KH <gregkh@linuxfoundation.org>:
> > >
> > > > >> > Do you have a pointer to these complex and buggy drivers anywhere?
> > > > >>
> > > > >> I am talking about the linux-mtd intel-spi driver for example, but I
> > > > >> feel that this gets the discussion in the wrong direction.
> > > > >
> > > > > This is the driver that covers all BIOSes on modern x86\64. What’s wrong with it? Why do you need this?!
> > > > >
> > > > > If it’s buggy, where is the bug reports from you or somebody else?
> > > >
> > > > Please see Mauro's mail in this thread from yesterday below:
> > >
> > > I didn't get this. What's wrong with the response from the author of
> > > intel-spi (since we are speaking about it, let me add him to the
> > > thread)?
> > > What you are trying to do is to sneak around with ugly hack behind the
> > > proper subsystem's back.
> > >
> > > NAK as I already said.
> >
> > There is nothing wrong with the response. Also we are not trying to
> > sneak anything into the kernel. This just is no mtd or spi driver, it
> > is not even a driver. What this does is it opens back up a portion of
> > memory that can not be read anymore through /dev/mem on locked down
> > systems with SecureBoot enabled. This portion of memory is actively
> > being used by userspace programs. We, 9elements, Eclypsium, fwudp and
> > immune are among those who rely upon this to improve the security of
> > x86_64 linux devices.
>
> I appreciate this.
>
> x86_64 starting from long time ago has more or less standard hardware
> interface for BIOS chip, i.e. SPI NOR. PCH usually has a separate host
> controller and we have the driver in the kernel for that (coverage of
> the systems may not be 100%, but close enough). Now you are trying to
> repeat something that is _already_ provided by the kernel. Correct?

Yes and no. We are not repeating the functionality of the SPI driver
because we don't read nor write the flash specifically. What we do, is
we make the boot vector readable by userspace when it is not
accessible by /dev/mem. It happens to be a portion of the SPI flash
but that doesn't have to be the case because the required view upon io
memory here is CPU-centric and the rest of the system could be built
in a way that backs that memory window differently. However, this is
more or less hypothetical if you ignore some probably never-used bits
that can be set in PCH registers. As I said, effectively we are trying
to partially revert the lockdown on /dev/mem to be able to do a
harmless read on an important memory range. From that point of view we
are trying to keep functionality intact. The interface being used here
has not changed a bit since 15 years and it probably will stay that
way. In contrast to the intel-spi driver this will likely cover more
future and past systems in different use-cases with fewer lines of
code and next to no maintenance.

>
> > Now what happens is that more distros start to
> > lock down their kernels and require signed modules. With the mtd
> > driver not working on those machine to read the bios binary, you are
> > effectively forcing users into less secure system configurations (i.e.
> > opening up the whole /dev/mem and/or disabling SecureBoot) just to be
> > able to run fwupd or other tools to assess firmware information. The
> > issue of being able to assess and compare the bios binary to the one
> > publicly available from the vendors is increasingly getting important
> > in the wake of recent CVEs about supply-chain attacks where UEFI
> > malware was pre-installed. So we are not even doing anything new here,
> > you are just making life harder for everybody.
>
> Why me? As far as I got it from above the bottleneck is the distros
> which do not enable the driver. So why not go and work with them?
>

Oh come on,  the distros have no choice here. Either not provide a
more secure locked down system or allow a dangerous driver where
patches to make it less dangerous are not welcome. And even if the
latter could be solved, the history clearly is that it is in no way
not even remotely as reliable as the memory region my patch is
relaying to userspace.

Hans-Gert

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Andy Shevchenko]

On Wed, Nov 10, 2021 at 11:17 AM Hans-Gert Dahmen
<hans-gert.dahmen@immu.ne> wrote:
> Am Mi., 10. Nov. 2021 um 10:05 Uhr schrieb Andy Shevchenko
> <andy.shevchenko@gmail.com>:
> > On Wed, Nov 10, 2021 at 10:37 AM Hans-Gert Dahmen
> > <hans-gert.dahmen@immu.ne> wrote:
> > > Am Mi., 10. Nov. 2021 um 07:35 Uhr schrieb Andy Shevchenko
> > > <andy.shevchenko@gmail.com>:
> > > > On Tuesday, November 9, 2021, Hans-Gert Dahmen <hans-gert.dahmen@immu.ne> wrote:
> > > >> Am Di., 9. Nov. 2021 um 13:42 Uhr schrieb Greg KH <gregkh@linuxfoundation.org>:
> >
> > > >> > Do you have a pointer to these complex and buggy drivers anywhere?
> > > >>
> > > >> I am talking about the linux-mtd intel-spi driver for example, but I
> > > >> feel that this gets the discussion in the wrong direction.
> > > >
> > > > This is the driver that covers all BIOSes on modern x86\64. What’s wrong with it? Why do you need this?!
> > > >
> > > > If it’s buggy, where is the bug reports from you or somebody else?
> > >
> > > Please see Mauro's mail in this thread from yesterday below:
> >
> > I didn't get this. What's wrong with the response from the author of
> > intel-spi (since we are speaking about it, let me add him to the
> > thread)?
> > What you are trying to do is to sneak around with ugly hack behind the
> > proper subsystem's back.
> >
> > NAK as I already said.
>
> There is nothing wrong with the response. Also we are not trying to
> sneak anything into the kernel. This just is no mtd or spi driver, it
> is not even a driver. What this does is it opens back up a portion of
> memory that can not be read anymore through /dev/mem on locked down
> systems with SecureBoot enabled. This portion of memory is actively
> being used by userspace programs. We, 9elements, Eclypsium, fwudp and
> immune are among those who rely upon this to improve the security of
> x86_64 linux devices.

I appreciate this.

x86_64 starting from long time ago has more or less standard hardware
interface for BIOS chip, i.e. SPI NOR. PCH usually has a separate host
controller and we have the driver in the kernel for that (coverage of
the systems may not be 100%, but close enough). Now you are trying to
repeat something that is _already_ provided by the kernel. Correct?

> Now what happens is that more distros start to
> lock down their kernels and require signed modules. With the mtd
> driver not working on those machine to read the bios binary, you are
> effectively forcing users into less secure system configurations (i.e.
> opening up the whole /dev/mem and/or disabling SecureBoot) just to be
> able to run fwupd or other tools to assess firmware information. The
> issue of being able to assess and compare the bios binary to the one
> publicly available from the vendors is increasingly getting important
> in the wake of recent CVEs about supply-chain attacks where UEFI
> malware was pre-installed. So we are not even doing anything new here,
> you are just making life harder for everybody.

Why me? As far as I got it from above the bottleneck is the distros
which do not enable the driver. So why not go and work with them?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Hans-Gert Dahmen]

Am Mi., 10. Nov. 2021 um 10:05 Uhr schrieb Andy Shevchenko
<andy.shevchenko@gmail.com>:
>
> On Wed, Nov 10, 2021 at 10:37 AM Hans-Gert Dahmen
> <hans-gert.dahmen@immu.ne> wrote:
> > Am Mi., 10. Nov. 2021 um 07:35 Uhr schrieb Andy Shevchenko
> > <andy.shevchenko@gmail.com>:
> > > On Tuesday, November 9, 2021, Hans-Gert Dahmen <hans-gert.dahmen@immu.ne> wrote:
> > >> Am Di., 9. Nov. 2021 um 13:42 Uhr schrieb Greg KH <gregkh@linuxfoundation.org>:
>
> > >> > Do you have a pointer to these complex and buggy drivers anywhere?
> > >>
> > >> I am talking about the linux-mtd intel-spi driver for example, but I
> > >> feel that this gets the discussion in the wrong direction.
> > >
> > > This is the driver that covers all BIOSes on modern x86\64. What’s wrong with it? Why do you need this?!
> > >
> > > If it’s buggy, where is the bug reports from you or somebody else?
> >
> > Please see Mauro's mail in this thread from yesterday below:
>
> I didn't get this. What's wrong with the response from the author of
> intel-spi (since we are speaking about it, let me add him to the
> thread)?
> What you are trying to do is to sneak around with ugly hack behind the
> proper subsystem's back.
>
> NAK as I already said.

There is nothing wrong with the response. Also we are not trying to
sneak anything into the kernel. This just is no mtd or spi driver, it
is not even a driver. What this does is it opens back up a portion of
memory that can not be read anymore through /dev/mem on locked down
systems with SecureBoot enabled. This portion of memory is actively
being used by userspace programs. We, 9elements, Eclypsium, fwudp and
immune are among those who rely upon this to improve the security of
x86_64 linux devices. Now what happens is that more distros start to
lock down their kernels and require signed modules. With the mtd
driver not working on those machine to read the bios binary, you are
effectively forcing users into less secure system configurations (i.e.
opening up the whole /dev/mem and/or disabling SecureBoot) just to be
able to run fwupd or other tools to assess firmware information. The
issue of being able to assess and compare the bios binary to the one
publicly available from the vendors is increasingly getting important
in the wake of recent CVEs about supply-chain attacks where UEFI
malware was pre-installed. So we are not even doing anything new here,
you are just making life harder for everybody.

>
> --
> With Best Regards,
> Andy Shevchenko

Hans-Gert

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Andy Shevchenko]

On Wed, Nov 10, 2021 at 10:37 AM Hans-Gert Dahmen
<hans-gert.dahmen@immu.ne> wrote:
> Am Mi., 10. Nov. 2021 um 07:35 Uhr schrieb Andy Shevchenko
> <andy.shevchenko@gmail.com>:
> > On Tuesday, November 9, 2021, Hans-Gert Dahmen <hans-gert.dahmen@immu.ne> wrote:
> >> Am Di., 9. Nov. 2021 um 13:42 Uhr schrieb Greg KH <gregkh@linuxfoundation.org>:

> >> > Do you have a pointer to these complex and buggy drivers anywhere?
> >>
> >> I am talking about the linux-mtd intel-spi driver for example, but I
> >> feel that this gets the discussion in the wrong direction.
> >
> > This is the driver that covers all BIOSes on modern x86\64. What’s wrong with it? Why do you need this?!
> >
> > If it’s buggy, where is the bug reports from you or somebody else?
>
> Please see Mauro's mail in this thread from yesterday below:

I didn't get this. What's wrong with the response from the author of
intel-spi (since we are speaking about it, let me add him to the
thread)?
What you are trying to do is to sneak around with ugly hack behind the
proper subsystem's back.

NAK as I already said.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Hans-Gert Dahmen]

Am Mi., 10. Nov. 2021 um 07:35 Uhr schrieb Andy Shevchenko
<andy.shevchenko@gmail.com>:
>
>
>
> On Tuesday, November 9, 2021, Hans-Gert Dahmen <hans-gert.dahmen@immu.ne> wrote:
>>
>> Am Di., 9. Nov. 2021 um 13:42 Uhr schrieb Greg KH <gregkh@linuxfoundation.org>:
>> > ...
>> >
>> > Do you have a pointer to these complex and buggy drivers anywhere?
>>
>> I am talking about the linux-mtd intel-spi driver for example, but I
>> feel that this gets the discussion in the wrong direction.
>
>
>
> This is the driver that covers all BIOSes on modern x86\64. What’s wrong with it? Why do you need this?!
>
> If it’s buggy, where is the bug reports from you or somebody else?
>

Please see Mauro's mail in this thread from yesterday below:

On Tuesday, November 9, 2021 Mauro Lima <mauro.lima@eclypsium.com> wrote:
> Sorry, I forgot the links
>
> [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1734147/+index?comments=all
> [2] https://lore.kernel.org/linux-mtd/20210910211348.642103-1-mauro.lima@eclypsium.com/
>
> Thanks

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Mauro Lima]

On Tue, Nov 9, 2021 at 1:12 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Nov 09, 2021 at 10:55:54AM -0300, Mauro Lima wrote:
> > Hi all,
> >
> > On Tue, Nov 9, 2021 at 3:16 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Tue, Nov 09, 2021 at 01:01:30AM +0100, Hans-Gert Dahmen wrote:
> > > > Make the 16MiB long memory-mapped BIOS region of the platform SPI flash
> > > > on X86_64 system available via /sys/kernel/firmware/flash_mmap/bios_region
> > > > for pen-testing, security analysis and malware detection on kernels
> > > > which restrict module loading and/or access to /dev/mem.
> > >
> > > That feels like a big security hole we would be opening up for no good
> > > reason.
> > Please, can you explain why this could be a security hole?
>
> We restricted /dev/mem and now you want to open a portion of it back up,
> hence my worry that now you can read information that previously you
> could not read.

Thanks for the explanation, I understand the worry about changing this
again but still I don't understand what advantage it can give an
attacker to be able to read the binary :(.

> > IMO if the host is compromised the attacker already has information
> > about the BIOS version, and after a quick lookup they know the BIOS
> > vulnerabilities or the lack of them.
>
> So you are saying that you do NOT need this access to get the BIOS
> information if you have root access?  If not, then why is this needed?

Sorry if I did not express myself clearly, but the information I was
talking about, could be the BIOS version number for example. If the
host is compromised you can get the BIOS version and check what
vulnerabilities are not fixed in that version and exploit them.
You don't need the binary for this, just with the number you can go
and check on vendor sites for unpatched vulnerabilities for that
version.
But, if you could take this binary, somehow compare it to a certified
blob given by your vendor and see if they match or not (corrupted),
could be a good reason for this.

> confused,

Sorry again and thanks for your time.

> greg k-h

Thanks, Mauro.

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Greg KH]

On Tue, Nov 09, 2021 at 10:55:54AM -0300, Mauro Lima wrote:
> Hi all,
> 
> On Tue, Nov 9, 2021 at 3:16 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Nov 09, 2021 at 01:01:30AM +0100, Hans-Gert Dahmen wrote:
> > > Make the 16MiB long memory-mapped BIOS region of the platform SPI flash
> > > on X86_64 system available via /sys/kernel/firmware/flash_mmap/bios_region
> > > for pen-testing, security analysis and malware detection on kernels
> > > which restrict module loading and/or access to /dev/mem.
> >
> > That feels like a big security hole we would be opening up for no good
> > reason.
> Please, can you explain why this could be a security hole?

We restricted /dev/mem and now you want to open a portion of it back up,
hence my worry that now you can read information that previously you
could not read.

> IMO if the host is compromised the attacker already has information
> about the BIOS version, and after a quick lookup they know the BIOS
> vulnerabilities or the lack of them.

So you are saying that you do NOT need this access to get the BIOS
information if you have root access?  If not, then why is this needed?

confused,

greg k-h

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Mauro Lima]

On Tue, Nov 9, 2021 at 11:09 AM Mauro Lima <mauro.lima@eclypsium.com> wrote:
>
> On Tue, Nov 9, 2021 at 9:42 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Nov 09, 2021 at 01:32:48PM +0100, Hans-Gert Dahmen wrote:
> > > On Tue, 9 Nov 2021, 11:28 Greg KH, <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Tue, Nov 09, 2021 at 09:52:53AM +0100, Hans-Gert Dahmen wrote:
> > > > > On 09.11.21 07:16, Greg KH wrote:
> > > > > > On Tue, Nov 09, 2021 at 01:01:30AM +0100, Hans-Gert Dahmen wrote:
> > > > > > > Make the 16MiB long memory-mapped BIOS region of the platform SPI flash
> > > > > > > on X86_64 system available via /sys/kernel/firmware/flash_mmap/bios_region
> > > > > > > for pen-testing, security analysis and malware detection on kernels
> > > > > > > which restrict module loading and/or access to /dev/mem.
> > > > > >
> > > > > > That feels like a big security hole we would be opening up for no good
> > > > > > reason.
> > > > > >
> > > > > > > It will be used by the open source Converged Security Suite.
> > > > > > > https://github.com/9elements/converged-security-suite
> > > > > >
> > > > > > What is the reason for this, and what use is this new interface?
> > > > > Because it is very hard to access the SPI flash to read the BIOS contents
> > > > > for (security) analysis and this works without the more complex and
> > > > > unfinished SPI drivers and it does so on a system where we may not access
> > > > > the full /dev/mem.
> > > >
> > > > Why not fix the spi drivers to do this properly?  What is preventing you
> > > > from doing that instead of adding a new user/kernel api that you will
> > > > have to support for the next 20+ years?
> > > >
> > >
> > > Because this interface we want to use is inherently tied to the
> > > workings of x86_64 CPUs. It is very stable and easy to use. The SPI
> > > drivers in contrast have a history of being complex, buggy and in
> > > general not reliable.
> >
> > Do you have a pointer to these complex and buggy drivers anywhere?
>
> Right now the intel spi driver has a _(DANGEROUS)_ tag [1], this is
> preventing the kernel engineers from compiling the driver into
> different distros.
> A couple of months ago I tried to modify the driver and make it RO [2]
>  but the drivers author told me that they don't want to remove the
> tag, even if the driver is compiled without any write/erase
> functionality as I proposed.
> The driver is fixed as the author claims but, as I said, they want to
> preserve the tag.
> This patch is close to what we need if we can't use the intel spi mechanism.
>
> >
> > > This module will require very little maintenance
> > > and will probably work in the future without needing modification for
> > > newer platforms. It generally is meant as a fallback to the real SPI
> > > flash drivers so that userspace programs are able to provide essential
> > > functionality.
> >
> > If it is a "fallback", how is it going to interact on a system where the
> > SPI driver is loaded?
> >
> > > > > > > +static int __init flash_mmap_init(void)
> > > > > > > +{
> > > > > > > + int ret;
> > > > > > > +
> > > > > > > + pdev = platform_device_register_simple("flash_mmap", -1, NULL, 0);
> > > > > > > + if (IS_ERR(pdev))
> > > > > > > +         return PTR_ERR(pdev);
> > > > > > > +
> > > > > > > + ret = sysfs_create_group(&pdev->dev.kobj, &flash_mmap_group);
> > > > > >
> > > > > > You just raced with userspace and lost  >
> > > > > > Please set the attribute to the platform driver before you create the
> > > > > > device.
> > > > > >
> > > > >
> > > > > Sorry, but I went through tons of code and could not find a single instance
> > > > > where I can use a default group for creation without using a probe function
> > > > > that does the magic for me. Please help me find the correct way of doing
> > > > > this without manually creating and adding kobjects.
> > > >
> > > > The problem here is that you are abusing the platform driver code and
> > > > making a "fake" device that is not attached to anything real, and
> > > > without a driver.  That should not be done, as you do not know what
> > > > hardware this driver is going to be run on.
> > > >
> > > > Bind to the real hardware resource please.
> > >
> > > In a previous mail in June you suggested this is some sort of platform
> > > device, that is why I rewrote it as such.
> >
> > That's fine, but bind to the real platform device that describes this
> > hardware!  You are not doing anything like this at all here, you are
> > just creating a random device in the sysfs tree and instantly allowing
> > userspace access to raw memory.  You are not actually controlling the
> > platform device at all.
> >
> > > The hardware resource here
> > > is the south bridge for x86_64 CPUs and the module dependencies will
> > > compile it only for these platforms.
> >
> > What "module dependencies"?  You do realize that distro kernels enable
> > all modules to be built. We have an "autoload only the modules that this
> > hardware needs" infrastructure that you need to tie into here, you are
> > totally ignoring it (despite it being present for almost 20 years
> > now...)
> >
> > > The situation is like, if you
> > > have that CPU, you have the hardware, so it is implicitly bound or it
> > > just will not execute on a machine code level.
> >
> > What do you mean "implicitly bound"?  How does this tie into the driver
> > model such that it will only be autoloaded, and have the driver bind to
> > the hardware, that it knows it can control?
> >
> > That is what needs to be fixed here, you are just claiming that it can
> > talk to the hardware without having any check at all for this.
> >
> > > I feel like your
> > > requirement is somewhat impossible to satisfy and I really don't know
> > > what to do. Please, can anyone help me by pointing out a proper
> > > example elsewhere in the kernel?
> >
> > What resource in the system describes the hardware you wish to bind to?
> > Write a driver for _that_ resource, and have it be properly autoloaded
> > when that resource is present in the system.
> >
> > You have hundreds of examples running on your system today, but as I do
> > not know where this hardware is described specifically, it's hard to be
> > able to point you to an example that works like that.
> >
> > So again, what hardware signature describes the resource you wish to
> > bind to?
> >
> > thanks,
> >
> > greg k-h
>
> Thanks, Mauro.

Sorry, I forgot the links

[1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1734147/+index?comments=all
[2] https://lore.kernel.org/linux-mtd/20210910211348.642103-1-mauro.lima@eclypsium.com/

Thanks

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Hans-Gert Dahmen]

Am Di., 9. Nov. 2021 um 13:42 Uhr schrieb Greg KH <gregkh@linuxfoundation.org>:
> ...
>
> Do you have a pointer to these complex and buggy drivers anywhere?

I am talking about the linux-mtd intel-spi driver for example, but I
feel that this gets the discussion in the wrong direction.

>
> > The situation is like, if you
> > have that CPU, you have the hardware, so it is implicitly bound or it
> > just will not execute on a machine code level.
>
> What do you mean "implicitly bound"?  How does this tie into the driver
> model such that it will only be autoloaded, and have the driver bind to
> the hardware, that it knows it can control?
>
> That is what needs to be fixed here, you are just claiming that it can
> talk to the hardware without having any check at all for this.

This memory mapping is magically provided by the thing on the CPU
front side bus, the south bridge f.e. It is there from the instant the
system powers on and it cannot be turned off. It is the CPU reset
vector that is used to boot any system. The memory range is required
by the x86_64 architectural specification. Every x86_64 system out
there has got it by definition. I do understand your concerns, but
this thing is a very special feature of the system architecture and so
the usual rules do not apply here.

>
> > I feel like your
> > requirement is somewhat impossible to satisfy and I really don't know
> > what to do. Please, can anyone help me by pointing out a proper
> > example elsewhere in the kernel?
>
> What resource in the system describes the hardware you wish to bind to?
> Write a driver for _that_ resource, and have it be properly autoloaded
> when that resource is present in the system.
>
> You have hundreds of examples running on your system today, but as I do
> not know where this hardware is described specifically, it's hard to be
> able to point you to an example that works like that.
>
> So again, what hardware signature describes the resource you wish to
> bind to?

As I said above the mapping is just required to be there but it is not
bound to any specific hardware. In practice every south-bridge on
every x86_64 mainboard in existence provides it. It does not have any
signature in a DMI table or a PCI ID or any of the like. It does not
need any descriptor because it is a given thing or the system could
not have booted. It is inherently tied to how the x86_64 architecture
works.
The only hardware dependency I can imagine there is, is that we are
running on x86_64. I have checked back with other developers and they
do agree. So is there a way to write a driver that is probed when we
are on a certain CPU architecture and that provides me a means to hook
into a probing flow where I can set up my sysfs default group to
safely add the attributes to userspace? Otherwise I'd say I am sure
that I understand your points and insist that the code is not broken
because of the very special circumstances.

Hans-Gert

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Mauro Lima]

On Tue, Nov 9, 2021 at 9:42 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Nov 09, 2021 at 01:32:48PM +0100, Hans-Gert Dahmen wrote:
> > On Tue, 9 Nov 2021, 11:28 Greg KH, <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Tue, Nov 09, 2021 at 09:52:53AM +0100, Hans-Gert Dahmen wrote:
> > > > On 09.11.21 07:16, Greg KH wrote:
> > > > > On Tue, Nov 09, 2021 at 01:01:30AM +0100, Hans-Gert Dahmen wrote:
> > > > > > Make the 16MiB long memory-mapped BIOS region of the platform SPI flash
> > > > > > on X86_64 system available via /sys/kernel/firmware/flash_mmap/bios_region
> > > > > > for pen-testing, security analysis and malware detection on kernels
> > > > > > which restrict module loading and/or access to /dev/mem.
> > > > >
> > > > > That feels like a big security hole we would be opening up for no good
> > > > > reason.
> > > > >
> > > > > > It will be used by the open source Converged Security Suite.
> > > > > > https://github.com/9elements/converged-security-suite
> > > > >
> > > > > What is the reason for this, and what use is this new interface?
> > > > Because it is very hard to access the SPI flash to read the BIOS contents
> > > > for (security) analysis and this works without the more complex and
> > > > unfinished SPI drivers and it does so on a system where we may not access
> > > > the full /dev/mem.
> > >
> > > Why not fix the spi drivers to do this properly?  What is preventing you
> > > from doing that instead of adding a new user/kernel api that you will
> > > have to support for the next 20+ years?
> > >
> >
> > Because this interface we want to use is inherently tied to the
> > workings of x86_64 CPUs. It is very stable and easy to use. The SPI
> > drivers in contrast have a history of being complex, buggy and in
> > general not reliable.
>
> Do you have a pointer to these complex and buggy drivers anywhere?

Right now the intel spi driver has a _(DANGEROUS)_ tag [1], this is
preventing the kernel engineers from compiling the driver into
different distros.
A couple of months ago I tried to modify the driver and make it RO [2]
 but the drivers author told me that they don't want to remove the
tag, even if the driver is compiled without any write/erase
functionality as I proposed.
The driver is fixed as the author claims but, as I said, they want to
preserve the tag.
This patch is close to what we need if we can't use the intel spi mechanism.

>
> > This module will require very little maintenance
> > and will probably work in the future without needing modification for
> > newer platforms. It generally is meant as a fallback to the real SPI
> > flash drivers so that userspace programs are able to provide essential
> > functionality.
>
> If it is a "fallback", how is it going to interact on a system where the
> SPI driver is loaded?
>
> > > > > > +static int __init flash_mmap_init(void)
> > > > > > +{
> > > > > > + int ret;
> > > > > > +
> > > > > > + pdev = platform_device_register_simple("flash_mmap", -1, NULL, 0);
> > > > > > + if (IS_ERR(pdev))
> > > > > > +         return PTR_ERR(pdev);
> > > > > > +
> > > > > > + ret = sysfs_create_group(&pdev->dev.kobj, &flash_mmap_group);
> > > > >
> > > > > You just raced with userspace and lost  >
> > > > > Please set the attribute to the platform driver before you create the
> > > > > device.
> > > > >
> > > >
> > > > Sorry, but I went through tons of code and could not find a single instance
> > > > where I can use a default group for creation without using a probe function
> > > > that does the magic for me. Please help me find the correct way of doing
> > > > this without manually creating and adding kobjects.
> > >
> > > The problem here is that you are abusing the platform driver code and
> > > making a "fake" device that is not attached to anything real, and
> > > without a driver.  That should not be done, as you do not know what
> > > hardware this driver is going to be run on.
> > >
> > > Bind to the real hardware resource please.
> >
> > In a previous mail in June you suggested this is some sort of platform
> > device, that is why I rewrote it as such.
>
> That's fine, but bind to the real platform device that describes this
> hardware!  You are not doing anything like this at all here, you are
> just creating a random device in the sysfs tree and instantly allowing
> userspace access to raw memory.  You are not actually controlling the
> platform device at all.
>
> > The hardware resource here
> > is the south bridge for x86_64 CPUs and the module dependencies will
> > compile it only for these platforms.
>
> What "module dependencies"?  You do realize that distro kernels enable
> all modules to be built. We have an "autoload only the modules that this
> hardware needs" infrastructure that you need to tie into here, you are
> totally ignoring it (despite it being present for almost 20 years
> now...)
>
> > The situation is like, if you
> > have that CPU, you have the hardware, so it is implicitly bound or it
> > just will not execute on a machine code level.
>
> What do you mean "implicitly bound"?  How does this tie into the driver
> model such that it will only be autoloaded, and have the driver bind to
> the hardware, that it knows it can control?
>
> That is what needs to be fixed here, you are just claiming that it can
> talk to the hardware without having any check at all for this.
>
> > I feel like your
> > requirement is somewhat impossible to satisfy and I really don't know
> > what to do. Please, can anyone help me by pointing out a proper
> > example elsewhere in the kernel?
>
> What resource in the system describes the hardware you wish to bind to?
> Write a driver for _that_ resource, and have it be properly autoloaded
> when that resource is present in the system.
>
> You have hundreds of examples running on your system today, but as I do
> not know where this hardware is described specifically, it's hard to be
> able to point you to an example that works like that.
>
> So again, what hardware signature describes the resource you wish to
> bind to?
>
> thanks,
>
> greg k-h

Thanks, Mauro.

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Mauro Lima]

Hi all,

On Tue, Nov 9, 2021 at 3:16 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Nov 09, 2021 at 01:01:30AM +0100, Hans-Gert Dahmen wrote:
> > Make the 16MiB long memory-mapped BIOS region of the platform SPI flash
> > on X86_64 system available via /sys/kernel/firmware/flash_mmap/bios_region
> > for pen-testing, security analysis and malware detection on kernels
> > which restrict module loading and/or access to /dev/mem.
>
> That feels like a big security hole we would be opening up for no good
> reason.
Please, can you explain why this could be a security hole?
IMO if the host is compromised the attacker already has information
about the BIOS version, and after a quick lookup they know the BIOS
vulnerabilities or the lack of them.
>
> > It will be used by the open source Converged Security Suite.
> > https://github.com/9elements/converged-security-suite
>
> What is the reason for this, and what use is this new interface?
In Eclypsium we are also interested in being able to dump the actual
binary from hosts and compare them to see if they are corrupted
somehow.
>
> >
> > Signed-off-by: Hans-Gert Dahmen <hans-gert.dahmen@immu.ne>
> > ---
> >  drivers/firmware/Kconfig             |  9 +++
> >  drivers/firmware/Makefile            |  1 +
> >  drivers/firmware/x86_64_flash_mmap.c | 86 ++++++++++++++++++++++++++++
>
> You forgot to document the new sysfs files in Documentation/ABI/ :(
>
>
> >  3 files changed, 96 insertions(+)
> >  create mode 100644 drivers/firmware/x86_64_flash_mmap.c
> >
> > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> > index 75cb91055c17..27c2d0b074e0 100644
> > --- a/drivers/firmware/Kconfig
> > +++ b/drivers/firmware/Kconfig
> > @@ -293,6 +293,15 @@ config TURRIS_MOX_RWTM
> >         other manufacturing data and also utilize the Entropy Bit Generator
> >         for hardware random number generation.
> >
> > +config X86_64_FLASH_MMAP
> > +     tristate "Export platform flash memory-mapped BIOS region"
> > +     depends on X86_64
> > +     help
> > +       Export the memory-mapped BIOS region of the platform SPI flash as
> > +       a read-only sysfs binary attribute on X86_64 systems. The first 16MiB
> > +       will be accessible via /sys/devices/platform/flash_mmap/bios_region
> > +       for security and malware analysis for example.
> > +
> >  source "drivers/firmware/arm_ffa/Kconfig"
> >  source "drivers/firmware/broadcom/Kconfig"
> >  source "drivers/firmware/cirrus/Kconfig"
> > diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
> > index 4e58cb474a68..60dc4ea08705 100644
> > --- a/drivers/firmware/Makefile
> > +++ b/drivers/firmware/Makefile
> > @@ -24,6 +24,7 @@ obj-$(CONFIG_SYSFB_SIMPLEFB)        += sysfb_simplefb.o
> >  obj-$(CONFIG_TI_SCI_PROTOCOL)        += ti_sci.o
> >  obj-$(CONFIG_TRUSTED_FOUNDATIONS) += trusted_foundations.o
> >  obj-$(CONFIG_TURRIS_MOX_RWTM)        += turris-mox-rwtm.o
> > +obj-$(CONFIG_X86_64_FLASH_MMAP)      += x86_64_flash_mmap.o
> >
> >  obj-y                                += arm_ffa/
> >  obj-y                                += arm_scmi/
> > diff --git a/drivers/firmware/x86_64_flash_mmap.c b/drivers/firmware/x86_64_flash_mmap.c
> > new file mode 100644
> > index 000000000000..23d8655d17bb
> > --- /dev/null
> > +++ b/drivers/firmware/x86_64_flash_mmap.c
> > @@ -0,0 +1,86 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Export the memory-mapped BIOS region of the platform SPI flash as
> > + * a read-only sysfs binary attribute on X86_64 systems.
> > + *
> > + * Copyright © 2021 immune GmbH
> > + */
> > +
> > +#include <linux/version.h>
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/io.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/platform_device.h>
> > +
> > +#define FLASH_REGION_START 0xFF000000ULL
> > +#define FLASH_REGION_SIZE 0x1000000ULL
> > +#define FLASH_REGION_MASK (FLASH_REGION_SIZE - 1)
> > +
> > +struct platform_device *pdev;
> > +
> > +static ssize_t bios_region_read(struct file *file, struct kobject *kobj,
> > +                             struct bin_attribute *bin_attr, char *buffer,
> > +                             loff_t offset, size_t count)
> > +{
> > +     resource_size_t pa;
> > +     size_t copysize, remapsize;
> > +     void __iomem *va;
> > +
> > +     offset = offset & FLASH_REGION_MASK;
> > +     pa = (FLASH_REGION_START + offset) & PAGE_MASK;
> > +
> > +     if ((offset + count) > FLASH_REGION_SIZE)
> > +             copysize = FLASH_REGION_SIZE - offset;
> > +     else
> > +             copysize = min(count, PAGE_SIZE);
> > +
> > +     if (((offset & ~PAGE_MASK) + copysize) > PAGE_SIZE)
> > +             remapsize = 2 * PAGE_SIZE;
> > +     else
> > +             remapsize = PAGE_SIZE;
> > +
> > +     va = ioremap(pa, remapsize);
> > +     memcpy_fromio(buffer, va, copysize);
> > +     iounmap(va);
> > +
> > +     return copysize;
> > +}
> > +
> > +static BIN_ATTR_RO(bios_region, FLASH_REGION_SIZE);
> > +
> > +static struct bin_attribute *flash_mmap_attrs[] = { &bin_attr_bios_region,
> > +                                                 NULL };
> > +
> > +static const struct attribute_group flash_mmap_group = {
> > +     .bin_attrs = flash_mmap_attrs,
> > +};
> > +
> > +static int __init flash_mmap_init(void)
> > +{
> > +     int ret;
> > +
> > +     pdev = platform_device_register_simple("flash_mmap", -1, NULL, 0);
> > +     if (IS_ERR(pdev))
> > +             return PTR_ERR(pdev);
> > +
> > +     ret = sysfs_create_group(&pdev->dev.kobj, &flash_mmap_group);
>
> You just raced with userspace and lost :(
>
> Please set the attribute to the platform driver before you create the
> device.
>
> Also, you just bound this driver to ANY platform that it was loaded on,
> with no actual detection of the hardware present, which feels like it
> could cause big problems on all platforms.  Please, if you really want
> to do this, restrict it to hardware that actually has the hardware you
> are wanting to access, not all machines in the world.
>
> thanks,
>
> greg k-h

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Greg KH]

On Tue, Nov 09, 2021 at 01:32:48PM +0100, Hans-Gert Dahmen wrote:
> On Tue, 9 Nov 2021, 11:28 Greg KH, <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Nov 09, 2021 at 09:52:53AM +0100, Hans-Gert Dahmen wrote:
> > > On 09.11.21 07:16, Greg KH wrote:
> > > > On Tue, Nov 09, 2021 at 01:01:30AM +0100, Hans-Gert Dahmen wrote:
> > > > > Make the 16MiB long memory-mapped BIOS region of the platform SPI flash
> > > > > on X86_64 system available via /sys/kernel/firmware/flash_mmap/bios_region
> > > > > for pen-testing, security analysis and malware detection on kernels
> > > > > which restrict module loading and/or access to /dev/mem.
> > > >
> > > > That feels like a big security hole we would be opening up for no good
> > > > reason.
> > > >
> > > > > It will be used by the open source Converged Security Suite.
> > > > > https://github.com/9elements/converged-security-suite
> > > >
> > > > What is the reason for this, and what use is this new interface?
> > > Because it is very hard to access the SPI flash to read the BIOS contents
> > > for (security) analysis and this works without the more complex and
> > > unfinished SPI drivers and it does so on a system where we may not access
> > > the full /dev/mem.
> >
> > Why not fix the spi drivers to do this properly?  What is preventing you
> > from doing that instead of adding a new user/kernel api that you will
> > have to support for the next 20+ years?
> >
> 
> Because this interface we want to use is inherently tied to the
> workings of x86_64 CPUs. It is very stable and easy to use. The SPI
> drivers in contrast have a history of being complex, buggy and in
> general not reliable.

Do you have a pointer to these complex and buggy drivers anywhere?

> This module will require very little maintenance
> and will probably work in the future without needing modification for
> newer platforms. It generally is meant as a fallback to the real SPI
> flash drivers so that userspace programs are able to provide essential
> functionality.

If it is a "fallback", how is it going to interact on a system where the
SPI driver is loaded?

> > > > > +static int __init flash_mmap_init(void)
> > > > > +{
> > > > > + int ret;
> > > > > +
> > > > > + pdev = platform_device_register_simple("flash_mmap", -1, NULL, 0);
> > > > > + if (IS_ERR(pdev))
> > > > > +         return PTR_ERR(pdev);
> > > > > +
> > > > > + ret = sysfs_create_group(&pdev->dev.kobj, &flash_mmap_group);
> > > >
> > > > You just raced with userspace and lost  >
> > > > Please set the attribute to the platform driver before you create the
> > > > device.
> > > >
> > >
> > > Sorry, but I went through tons of code and could not find a single instance
> > > where I can use a default group for creation without using a probe function
> > > that does the magic for me. Please help me find the correct way of doing
> > > this without manually creating and adding kobjects.
> >
> > The problem here is that you are abusing the platform driver code and
> > making a "fake" device that is not attached to anything real, and
> > without a driver.  That should not be done, as you do not know what
> > hardware this driver is going to be run on.
> >
> > Bind to the real hardware resource please.
> 
> In a previous mail in June you suggested this is some sort of platform
> device, that is why I rewrote it as such.

That's fine, but bind to the real platform device that describes this
hardware!  You are not doing anything like this at all here, you are
just creating a random device in the sysfs tree and instantly allowing
userspace access to raw memory.  You are not actually controlling the
platform device at all.

> The hardware resource here
> is the south bridge for x86_64 CPUs and the module dependencies will
> compile it only for these platforms.

What "module dependencies"?  You do realize that distro kernels enable
all modules to be built. We have an "autoload only the modules that this
hardware needs" infrastructure that you need to tie into here, you are
totally ignoring it (despite it being present for almost 20 years
now...)

> The situation is like, if you
> have that CPU, you have the hardware, so it is implicitly bound or it
> just will not execute on a machine code level.

What do you mean "implicitly bound"?  How does this tie into the driver
model such that it will only be autoloaded, and have the driver bind to
the hardware, that it knows it can control?

That is what needs to be fixed here, you are just claiming that it can
talk to the hardware without having any check at all for this.

> I feel like your
> requirement is somewhat impossible to satisfy and I really don't know
> what to do. Please, can anyone help me by pointing out a proper
> example elsewhere in the kernel?

What resource in the system describes the hardware you wish to bind to?
Write a driver for _that_ resource, and have it be properly autoloaded
when that resource is present in the system.

You have hundreds of examples running on your system today, but as I do
not know where this hardware is described specifically, it's hard to be
able to point you to an example that works like that.

So again, what hardware signature describes the resource you wish to
bind to?

thanks,

greg k-h

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Hans-Gert Dahmen]

On Tue, 9 Nov 2021, 11:28 Greg KH, <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Nov 09, 2021 at 09:52:53AM +0100, Hans-Gert Dahmen wrote:
> > On 09.11.21 07:16, Greg KH wrote:
> > > On Tue, Nov 09, 2021 at 01:01:30AM +0100, Hans-Gert Dahmen wrote:
> > > > Make the 16MiB long memory-mapped BIOS region of the platform SPI flash
> > > > on X86_64 system available via /sys/kernel/firmware/flash_mmap/bios_region
> > > > for pen-testing, security analysis and malware detection on kernels
> > > > which restrict module loading and/or access to /dev/mem.
> > >
> > > That feels like a big security hole we would be opening up for no good
> > > reason.
> > >
> > > > It will be used by the open source Converged Security Suite.
> > > > https://github.com/9elements/converged-security-suite
> > >
> > > What is the reason for this, and what use is this new interface?
> > Because it is very hard to access the SPI flash to read the BIOS contents
> > for (security) analysis and this works without the more complex and
> > unfinished SPI drivers and it does so on a system where we may not access
> > the full /dev/mem.
>
> Why not fix the spi drivers to do this properly?  What is preventing you
> from doing that instead of adding a new user/kernel api that you will
> have to support for the next 20+ years?
>

Because this interface we want to use is inherently tied to the
workings of x86_64 CPUs. It is very stable and easy to use. The SPI
drivers in contrast have a history of being complex, buggy and in
general not reliable. This module will require very little maintenance
and will probably work in the future without needing modification for
newer platforms. It generally is meant as a fallback to the real SPI
flash drivers so that userspace programs are able to provide essential
functionality.

> > > > +static int __init flash_mmap_init(void)
> > > > +{
> > > > + int ret;
> > > > +
> > > > + pdev = platform_device_register_simple("flash_mmap", -1, NULL, 0);
> > > > + if (IS_ERR(pdev))
> > > > +         return PTR_ERR(pdev);
> > > > +
> > > > + ret = sysfs_create_group(&pdev->dev.kobj, &flash_mmap_group);
> > >
> > > You just raced with userspace and lost  >
> > > Please set the attribute to the platform driver before you create the
> > > device.
> > >
> >
> > Sorry, but I went through tons of code and could not find a single instance
> > where I can use a default group for creation without using a probe function
> > that does the magic for me. Please help me find the correct way of doing
> > this without manually creating and adding kobjects.
>
> The problem here is that you are abusing the platform driver code and
> making a "fake" device that is not attached to anything real, and
> without a driver.  That should not be done, as you do not know what
> hardware this driver is going to be run on.
>
> Bind to the real hardware resource please.

In a previous mail in June you suggested this is some sort of platform
device, that is why I rewrote it as such. The hardware resource here
is the south bridge for x86_64 CPUs and the module dependencies will
compile it only for these platforms. The situation is like, if you
have that CPU, you have the hardware, so it is implicitly bound or it
just will not execute on a machine code level. I feel like your
requirement is somewhat impossible to satisfy and I really don't know
what to do. Please, can anyone help me by pointing out a proper
example elsewhere in the kernel?

Hans-Gert

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Greg KH]

A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top


On Tue, Nov 09, 2021 at 11:30:06AM +0100, Philipp Deppenwiese wrote:
> Hi Greg,
> 
> sorry for the previous html email, totally forgot kernel ml was plain
> text only.

It's also interleaved responses :)

> Just some feedback regarding the use case for the interface. As you may
> know the firmware (BIOS/UEFI/coreboot) is growing massively in the last
> few years. So we have now 32MB of firmware on a normal x86 system. The
> interface shall be used as safe and secure method to get the first 16MB
> read-only copy of the firmware. The interface from Intel is read-only so
> we shouldn't introduce any security issues here.

The problem is this driver will "bind" to any device it is loaded on,
which is not ok.  It must only work on hardware that it is known to work
on, as remember, Linux runs on hundreds of thousands of different
platforms and types of hardware.

> Aside from us there is fwupd.org and another company which are
> interested in the interface as well. We have added Richard from Redhat
> to the CC. We use the interface mainly for firmware analysis and TPM PCR
> pre-calculation features in our SaaS product.
> 
> I hope that resolves your questions :). Let me know if you want to
> discuss more.

Given a lack of documentation as to what this interface would be used
for, that needs to be resolved, along with links to userspace code that
uses this new api.

thanks,

greg k-h

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Philipp Deppenwiese]

Hi Greg,

sorry for the previous html email, totally forgot kernel ml was plain
text only.

Just some feedback regarding the use case for the interface. As you may
know the firmware (BIOS/UEFI/coreboot) is growing massively in the last
few years. So we have now 32MB of firmware on a normal x86 system. The
interface shall be used as safe and secure method to get the first 16MB
read-only copy of the firmware. The interface from Intel is read-only so
we shouldn't introduce any security issues here.

Aside from us there is fwupd.org and another company which are
interested in the interface as well. We have added Richard from Redhat
to the CC. We use the interface mainly for firmware analysis and TPM PCR
pre-calculation features in our SaaS product.

I hope that resolves your questions :). Let me know if you want to
discuss more.

@Hans-Gert can you deliver the documentation, otherwise I can help out.

Greetings Philipp

On Nov. 9 2021, at 11:24 am, Greg KH <gregkh@linuxfoundation.org> wrote:

> On Tue, Nov 09, 2021 at 11:06:07AM +0100, Philipp Deppenwiese wrote:
>> Hi Greg,
>  
> <snip>
>  
> Sorry, html email is rejected by the mailing lists.  Please fix your
> email client to send text-only email and I will be glad to respond.
>  
> thanks,
>  
> greg k-h
>

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Greg KH]

On Tue, Nov 09, 2021 at 09:52:53AM +0100, Hans-Gert Dahmen wrote:
> On 09.11.21 07:16, Greg KH wrote:
> > On Tue, Nov 09, 2021 at 01:01:30AM +0100, Hans-Gert Dahmen wrote:
> > > Make the 16MiB long memory-mapped BIOS region of the platform SPI flash
> > > on X86_64 system available via /sys/kernel/firmware/flash_mmap/bios_region
> > > for pen-testing, security analysis and malware detection on kernels
> > > which restrict module loading and/or access to /dev/mem.
> > 
> > That feels like a big security hole we would be opening up for no good
> > reason.
> > 
> > > It will be used by the open source Converged Security Suite.
> > > https://github.com/9elements/converged-security-suite
> > 
> > What is the reason for this, and what use is this new interface?
> Because it is very hard to access the SPI flash to read the BIOS contents
> for (security) analysis and this works without the more complex and
> unfinished SPI drivers and it does so on a system where we may not access
> the full /dev/mem.

Why not fix the spi drivers to do this properly?  What is preventing you
from doing that instead of adding a new user/kernel api that you will
have to support for the next 20+ years?

> > > +static int __init flash_mmap_init(void)
> > > +{
> > > +	int ret;
> > > +
> > > +	pdev = platform_device_register_simple("flash_mmap", -1, NULL, 0);
> > > +	if (IS_ERR(pdev))
> > > +		return PTR_ERR(pdev);
> > > +
> > > +	ret = sysfs_create_group(&pdev->dev.kobj, &flash_mmap_group);
> > 
> > You just raced with userspace and lost  >
> > Please set the attribute to the platform driver before you create the
> > device.
> >
> 
> Sorry, but I went through tons of code and could not find a single instance
> where I can use a default group for creation without using a probe function
> that does the magic for me. Please help me find the correct way of doing
> this without manually creating and adding kobjects.

The problem here is that you are abusing the platform driver code and
making a "fake" device that is not attached to anything real, and
without a driver.  That should not be done, as you do not know what
hardware this driver is going to be run on.

Bind to the real hardware resource please.

> > Also, you just bound this driver to ANY platform that it was loaded on,
> > with no actual detection of the hardware present, which feels like it
> > could cause big problems on all platforms.  Please, if you really want
> > to do this, restrict it to hardware that actually has the hardware you
> > are wanting to access, not all machines in the world.
> 
> I ave already proven that it works on all x64 Intel platforms here [1]. It
> nearly impossible to prove for AMD b/c of the lack of documentation, but we
> tested it on several old Bulldozer system and so far the memory was always
> mapped. I feel that adding more hardware detection just adds complexity.
> Anyway, what do you suggest? Use CPUID to check if the vendor is AMD or
> Intel?

Again, without some sort of "we only bind to the hardware on these types
of devices", a driver like this is not going to be allowed, because you
are not checking the hardware at all!

Also, drivers are loaded based on the hardware being found on the system
and having the driver loaded automatically.  That isn't happening here
at all, so that's another reason why this isn't going to be acceptable.

You HAVE to have hardware detection, otherwise the code is broken,
sorry.

thanks,

greg k-h

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Greg KH]

On Tue, Nov 09, 2021 at 11:06:07AM +0100, Philipp Deppenwiese wrote:
> Hi Greg,

<snip>

Sorry, html email is rejected by the mailing lists.  Please fix your
email client to send text-only email and I will be glad to respond.

thanks,

greg k-h

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Hans-Gert Dahmen]

I am really sorry I forgot the link for [1] in my last mail.

[1] https://lkml.org/lkml/2021/6/23/433

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Hans-Gert Dahmen]

On 09.11.21 07:16, Greg KH wrote:
> On Tue, Nov 09, 2021 at 01:01:30AM +0100, Hans-Gert Dahmen wrote:
>> Make the 16MiB long memory-mapped BIOS region of the platform SPI flash
>> on X86_64 system available via /sys/kernel/firmware/flash_mmap/bios_region
>> for pen-testing, security analysis and malware detection on kernels
>> which restrict module loading and/or access to /dev/mem.
> 
> That feels like a big security hole we would be opening up for no good
> reason.
> 
>> It will be used by the open source Converged Security Suite.
>> https://github.com/9elements/converged-security-suite
> 
> What is the reason for this, and what use is this new interface?
Because it is very hard to access the SPI flash to read the BIOS 
contents for (security) analysis and this works without the more complex 
and unfinished SPI drivers and it does so on a system where we may not 
access the full /dev/mem.

>> +static int __init flash_mmap_init(void)
>> +{
>> +	int ret;
>> +
>> +	pdev = platform_device_register_simple("flash_mmap", -1, NULL, 0);
>> +	if (IS_ERR(pdev))
>> +		return PTR_ERR(pdev);
>> +
>> +	ret = sysfs_create_group(&pdev->dev.kobj, &flash_mmap_group);
> 
> You just raced with userspace and lost  >
 > Please set the attribute to the platform driver before you create the
 > device.
 >

Sorry, but I went through tons of code and could not find a single 
instance where I can use a default group for creation without using a 
probe function that does the magic for me. Please help me find the 
correct way of doing this without manually creating and adding kobjects.

> Also, you just bound this driver to ANY platform that it was loaded on,
> with no actual detection of the hardware present, which feels like it
> could cause big problems on all platforms.  Please, if you really want
> to do this, restrict it to hardware that actually has the hardware you
> are wanting to access, not all machines in the world.

I ave already proven that it works on all x64 Intel platforms here [1]. 
It nearly impossible to prove for AMD b/c of the lack of documentation, 
but we tested it on several old Bulldozer system and so far the memory 
was always mapped. I feel that adding more hardware detection just adds 
complexity. Anyway, what do you suggest? Use CPUID to check if the 
vendor is AMD or Intel?

Hans-Gert Dahmen

Re: [PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Greg KH]

On Tue, Nov 09, 2021 at 01:01:30AM +0100, Hans-Gert Dahmen wrote:
> Make the 16MiB long memory-mapped BIOS region of the platform SPI flash
> on X86_64 system available via /sys/kernel/firmware/flash_mmap/bios_region
> for pen-testing, security analysis and malware detection on kernels
> which restrict module loading and/or access to /dev/mem.

That feels like a big security hole we would be opening up for no good
reason.

> It will be used by the open source Converged Security Suite.
> https://github.com/9elements/converged-security-suite

What is the reason for this, and what use is this new interface?

> 
> Signed-off-by: Hans-Gert Dahmen <hans-gert.dahmen@immu.ne>
> ---
>  drivers/firmware/Kconfig             |  9 +++
>  drivers/firmware/Makefile            |  1 +
>  drivers/firmware/x86_64_flash_mmap.c | 86 ++++++++++++++++++++++++++++

You forgot to document the new sysfs files in Documentation/ABI/ :(


>  3 files changed, 96 insertions(+)
>  create mode 100644 drivers/firmware/x86_64_flash_mmap.c
> 
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index 75cb91055c17..27c2d0b074e0 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -293,6 +293,15 @@ config TURRIS_MOX_RWTM
>  	  other manufacturing data and also utilize the Entropy Bit Generator
>  	  for hardware random number generation.
>  
> +config X86_64_FLASH_MMAP
> +	tristate "Export platform flash memory-mapped BIOS region"
> +	depends on X86_64
> +	help
> +	  Export the memory-mapped BIOS region of the platform SPI flash as
> +	  a read-only sysfs binary attribute on X86_64 systems. The first 16MiB
> +	  will be accessible via /sys/devices/platform/flash_mmap/bios_region
> +	  for security and malware analysis for example.
> +
>  source "drivers/firmware/arm_ffa/Kconfig"
>  source "drivers/firmware/broadcom/Kconfig"
>  source "drivers/firmware/cirrus/Kconfig"
> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
> index 4e58cb474a68..60dc4ea08705 100644
> --- a/drivers/firmware/Makefile
> +++ b/drivers/firmware/Makefile
> @@ -24,6 +24,7 @@ obj-$(CONFIG_SYSFB_SIMPLEFB)	+= sysfb_simplefb.o
>  obj-$(CONFIG_TI_SCI_PROTOCOL)	+= ti_sci.o
>  obj-$(CONFIG_TRUSTED_FOUNDATIONS) += trusted_foundations.o
>  obj-$(CONFIG_TURRIS_MOX_RWTM)	+= turris-mox-rwtm.o
> +obj-$(CONFIG_X86_64_FLASH_MMAP)	+= x86_64_flash_mmap.o
>  
>  obj-y				+= arm_ffa/
>  obj-y				+= arm_scmi/
> diff --git a/drivers/firmware/x86_64_flash_mmap.c b/drivers/firmware/x86_64_flash_mmap.c
> new file mode 100644
> index 000000000000..23d8655d17bb
> --- /dev/null
> +++ b/drivers/firmware/x86_64_flash_mmap.c
> @@ -0,0 +1,86 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Export the memory-mapped BIOS region of the platform SPI flash as
> + * a read-only sysfs binary attribute on X86_64 systems.
> + *
> + * Copyright © 2021 immune GmbH
> + */
> +
> +#include <linux/version.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/sysfs.h>
> +#include <linux/platform_device.h>
> +
> +#define FLASH_REGION_START 0xFF000000ULL
> +#define FLASH_REGION_SIZE 0x1000000ULL
> +#define FLASH_REGION_MASK (FLASH_REGION_SIZE - 1)
> +
> +struct platform_device *pdev;
> +
> +static ssize_t bios_region_read(struct file *file, struct kobject *kobj,
> +				struct bin_attribute *bin_attr, char *buffer,
> +				loff_t offset, size_t count)
> +{
> +	resource_size_t pa;
> +	size_t copysize, remapsize;
> +	void __iomem *va;
> +
> +	offset = offset & FLASH_REGION_MASK;
> +	pa = (FLASH_REGION_START + offset) & PAGE_MASK;
> +
> +	if ((offset + count) > FLASH_REGION_SIZE)
> +		copysize = FLASH_REGION_SIZE - offset;
> +	else
> +		copysize = min(count, PAGE_SIZE);
> +
> +	if (((offset & ~PAGE_MASK) + copysize) > PAGE_SIZE)
> +		remapsize = 2 * PAGE_SIZE;
> +	else
> +		remapsize = PAGE_SIZE;
> +
> +	va = ioremap(pa, remapsize);
> +	memcpy_fromio(buffer, va, copysize);
> +	iounmap(va);
> +
> +	return copysize;
> +}
> +
> +static BIN_ATTR_RO(bios_region, FLASH_REGION_SIZE);
> +
> +static struct bin_attribute *flash_mmap_attrs[] = { &bin_attr_bios_region,
> +						    NULL };
> +
> +static const struct attribute_group flash_mmap_group = {
> +	.bin_attrs = flash_mmap_attrs,
> +};
> +
> +static int __init flash_mmap_init(void)
> +{
> +	int ret;
> +
> +	pdev = platform_device_register_simple("flash_mmap", -1, NULL, 0);
> +	if (IS_ERR(pdev))
> +		return PTR_ERR(pdev);
> +
> +	ret = sysfs_create_group(&pdev->dev.kobj, &flash_mmap_group);

You just raced with userspace and lost :(

Please set the attribute to the platform driver before you create the
device.

Also, you just bound this driver to ANY platform that it was loaded on,
with no actual detection of the hardware present, which feels like it
could cause big problems on all platforms.  Please, if you really want
to do this, restrict it to hardware that actually has the hardware you
are wanting to access, not all machines in the world.

thanks,

greg k-h

[PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Hans-Gert Dahmen]

Make the 16MiB long memory-mapped BIOS region of the platform SPI flash
on X86_64 system available via /sys/kernel/firmware/flash_mmap/bios_region
for pen-testing, security analysis and malware detection on kernels
which restrict module loading and/or access to /dev/mem.

It will be used by the open source Converged Security Suite.
https://github.com/9elements/converged-security-suite

Signed-off-by: Hans-Gert Dahmen <hans-gert.dahmen@immu.ne>
---
 drivers/firmware/Kconfig             |  9 +++
 drivers/firmware/Makefile            |  1 +
 drivers/firmware/x86_64_flash_mmap.c | 86 ++++++++++++++++++++++++++++
 3 files changed, 96 insertions(+)
 create mode 100644 drivers/firmware/x86_64_flash_mmap.c

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 75cb91055c17..27c2d0b074e0 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -293,6 +293,15 @@ config TURRIS_MOX_RWTM
 	  other manufacturing data and also utilize the Entropy Bit Generator
 	  for hardware random number generation.
 
+config X86_64_FLASH_MMAP
+	tristate "Export platform flash memory-mapped BIOS region"
+	depends on X86_64
+	help
+	  Export the memory-mapped BIOS region of the platform SPI flash as
+	  a read-only sysfs binary attribute on X86_64 systems. The first 16MiB
+	  will be accessible via /sys/devices/platform/flash_mmap/bios_region
+	  for security and malware analysis for example.
+
 source "drivers/firmware/arm_ffa/Kconfig"
 source "drivers/firmware/broadcom/Kconfig"
 source "drivers/firmware/cirrus/Kconfig"
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 4e58cb474a68..60dc4ea08705 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_SYSFB_SIMPLEFB)	+= sysfb_simplefb.o
 obj-$(CONFIG_TI_SCI_PROTOCOL)	+= ti_sci.o
 obj-$(CONFIG_TRUSTED_FOUNDATIONS) += trusted_foundations.o
 obj-$(CONFIG_TURRIS_MOX_RWTM)	+= turris-mox-rwtm.o
+obj-$(CONFIG_X86_64_FLASH_MMAP)	+= x86_64_flash_mmap.o
 
 obj-y				+= arm_ffa/
 obj-y				+= arm_scmi/
diff --git a/drivers/firmware/x86_64_flash_mmap.c b/drivers/firmware/x86_64_flash_mmap.c
new file mode 100644
index 000000000000..23d8655d17bb
--- /dev/null
+++ b/drivers/firmware/x86_64_flash_mmap.c
@@ -0,0 +1,86 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Export the memory-mapped BIOS region of the platform SPI flash as
+ * a read-only sysfs binary attribute on X86_64 systems.
+ *
+ * Copyright © 2021 immune GmbH
+ */
+
+#include <linux/version.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/sysfs.h>
+#include <linux/platform_device.h>
+
+#define FLASH_REGION_START 0xFF000000ULL
+#define FLASH_REGION_SIZE 0x1000000ULL
+#define FLASH_REGION_MASK (FLASH_REGION_SIZE - 1)
+
+struct platform_device *pdev;
+
+static ssize_t bios_region_read(struct file *file, struct kobject *kobj,
+				struct bin_attribute *bin_attr, char *buffer,
+				loff_t offset, size_t count)
+{
+	resource_size_t pa;
+	size_t copysize, remapsize;
+	void __iomem *va;
+
+	offset = offset & FLASH_REGION_MASK;
+	pa = (FLASH_REGION_START + offset) & PAGE_MASK;
+
+	if ((offset + count) > FLASH_REGION_SIZE)
+		copysize = FLASH_REGION_SIZE - offset;
+	else
+		copysize = min(count, PAGE_SIZE);
+
+	if (((offset & ~PAGE_MASK) + copysize) > PAGE_SIZE)
+		remapsize = 2 * PAGE_SIZE;
+	else
+		remapsize = PAGE_SIZE;
+
+	va = ioremap(pa, remapsize);
+	memcpy_fromio(buffer, va, copysize);
+	iounmap(va);
+
+	return copysize;
+}
+
+static BIN_ATTR_RO(bios_region, FLASH_REGION_SIZE);
+
+static struct bin_attribute *flash_mmap_attrs[] = { &bin_attr_bios_region,
+						    NULL };
+
+static const struct attribute_group flash_mmap_group = {
+	.bin_attrs = flash_mmap_attrs,
+};
+
+static int __init flash_mmap_init(void)
+{
+	int ret;
+
+	pdev = platform_device_register_simple("flash_mmap", -1, NULL, 0);
+	if (IS_ERR(pdev))
+		return PTR_ERR(pdev);
+
+	ret = sysfs_create_group(&pdev->dev.kobj, &flash_mmap_group);
+	if (ret) {
+		dev_err(&pdev->dev, "sysfs creation failed\n");
+		platform_device_unregister(pdev);
+	}
+
+	return ret;
+}
+
+static void __exit flash_mmap_exit(void)
+{
+	sysfs_remove_group(&pdev->dev.kobj, &flash_mmap_group);
+	platform_device_unregister(pdev);
+}
+
+module_init(flash_mmap_init);
+module_exit(flash_mmap_exit);
+MODULE_DESCRIPTION("Export SPI platform flash memory mapped region via sysfs");
+MODULE_AUTHOR("Hans-Gert Dahmen <hans-gert.dahmen@immu.ne>");
+MODULE_LICENSE("GPL");
-- 
2.32.0

[PATCH] firmware: export x86_64 platform flash bios region via sysfs

[Hans-Gert Dahmen]

Make the 16MiB long memory-mapped BIOS region of the platform SPI flash
on X86_64 system available via /sys/kernel/firmware/flash_mmap/bios_region
for pen-testing, security analysis and malware detection on kernels
which restrict module loading and/or access to /dev/mem.

It will be used by the open source Converged Security Suite.
https://github.com/9elements/converged-security-suite

Signed-off-by: Hans-Gert Dahmen <hans-gert.dahmen@immu.ne>
---
 drivers/firmware/Kconfig             |  9 ++++
 drivers/firmware/Makefile            |  1 +
 drivers/firmware/x86_64_flash_mmap.c | 65 ++++++++++++++++++++++++++++
 3 files changed, 75 insertions(+)
 create mode 100644 drivers/firmware/x86_64_flash_mmap.c

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index db0ea2d2d75a..bd77ca2b4fa6 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -296,6 +296,15 @@ config TURRIS_MOX_RWTM
 	  other manufacturing data and also utilize the Entropy Bit Generator
 	  for hardware random number generation.
 
+config X86_64_FLASH_MMAP
+	tristate "Export platform flash memory-mapped BIOS region"
+	depends on X86_64
+	help
+	  Export the memory-mapped BIOS region of the platform SPI flash as
+	  a read-only sysfs binary attribute on X86_64 systems. The first 16MiB
+	  will be accessible via /sys/kernel/firmware/flash_mmap/bios_region
+	  for security and malware analysis for example.
+
 source "drivers/firmware/broadcom/Kconfig"
 source "drivers/firmware/google/Kconfig"
 source "drivers/firmware/efi/Kconfig"
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index 5e013b6a3692..eb7483c5a2ac 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_QCOM_SCM)		+= qcom_scm.o qcom_scm-smc.o qcom_scm-legacy.o
 obj-$(CONFIG_TI_SCI_PROTOCOL)	+= ti_sci.o
 obj-$(CONFIG_TRUSTED_FOUNDATIONS) += trusted_foundations.o
 obj-$(CONFIG_TURRIS_MOX_RWTM)	+= turris-mox-rwtm.o
+obj-$(CONFIG_X86_64_FLASH_MMAP)	+= x86_64_flash_mmap.o
 
 obj-y				+= arm_scmi/
 obj-y				+= broadcom/
diff --git a/drivers/firmware/x86_64_flash_mmap.c b/drivers/firmware/x86_64_flash_mmap.c
new file mode 100644
index 000000000000..f9d871a8b516
--- /dev/null
+++ b/drivers/firmware/x86_64_flash_mmap.c
@@ -0,0 +1,65 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Export the memory-mapped BIOS region of the platform SPI flash as
+ * a read-only sysfs binary attribute on X86_64 systems.
+ *
+ * Copyright © 2021 immune GmbH
+ */
+
+#include <linux/version.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/sysfs.h>
+#include <linux/kobject.h>
+
+#define FLASH_REGION_START 0xFF000000ULL
+#define FLASH_REGION_SIZE 0x1000000ULL
+#define FLASH_REGION_MASK (FLASH_REGION_SIZE - 1)
+
+struct kobject *kobj_ref;
+
+static ssize_t bios_region_read(struct file *file, struct kobject *kobj,
+				struct bin_attribute *bin_attr, char *buffer,
+				loff_t offset, size_t count)
+{
+	resource_size_t pa = FLASH_REGION_START + (offset & FLASH_REGION_MASK);
+	void __iomem *va = ioremap(pa, PAGE_SIZE);
+
+	memcpy_fromio(buffer, va, PAGE_SIZE);
+	iounmap(va);
+
+	return min(count, PAGE_SIZE);
+}
+
+BIN_ATTR_RO(bios_region, FLASH_REGION_SIZE);
+
+static int __init flash_mmap_init(void)
+{
+	int ret = 0;
+
+	kobj_ref = kobject_create_and_add("flash_mmap", firmware_kobj);
+	ret = sysfs_create_bin_file(kobj_ref, &bin_attr_bios_region);
+	if (ret) {
+		pr_err("sysfs_create_bin_file failed\n");
+		goto error;
+	}
+
+	return ret;
+
+error:
+	kobject_put(kobj_ref);
+	return ret;
+}
+
+static void __exit flash_mmap_exit(void)
+{
+	sysfs_remove_bin_file(kernel_kobj, &bin_attr_bios_region);
+	kobject_put(kobj_ref);
+}
+
+module_init(flash_mmap_init);
+module_exit(flash_mmap_exit);
+MODULE_DESCRIPTION("Export SPI platform flash memory mapped region via sysfs");
+MODULE_AUTHOR("Hans-Gert Dahmen <hans-gert.dahmen@immu.ne>");
+MODULE_LICENSE("GPL");
-- 
2.30.2