1. Patchew
  2. linux
  3. View series

[PATCH v2] ACPI: APEI: EINJ: Allow all types of addresses except MMIO

Jiaqi Yan posted 1 patch 1 month ago 
drivers/acpi/apei/einj-core.c | 50 +++++++++++++++++++++++++++++------
1 file changed, 42 insertions(+), 8 deletions(-)
[PATCH v2] ACPI: APEI: EINJ: Allow all types of addresses except MMIO
Posted by Jiaqi Yan 1 month ago 
EINJ driver today only allows injection request to go through for two
kinds of IORESOURCE_MEM: IORES_DESC_PERSISTENT_MEMORY and
IORES_DESC_SOFT_RESERVED. This check prevents user of EINJ to test
memory corrupted in many interesting areas:

- Legacy persistent memory
- Memory claimed to be used by ACPI tables or NV storage
- Kernel crash memory and others

There is need to test how kernel behaves when something consumes memory
errors in these memory regions. For example, if certain ACPI table is
corrupted, does kernel crash gracefully to prevent "silent data
corruption". For another example, legacy persistent memory, when managed
by Device DAX, does support recovering from Machine Check Exception
raised by memory failure, hence worth to be tested.

However, attempt to inject memory error via EINJ to legacy persistent
memory or ACPI owned memory fails with -EINVAL.

Allow EINJ to inject at address except it is MMIO. Leave it to the BIOS
or firmware to decide what is a legitimate injection target.

In addition to the test done in [1], on a machine having the following
iomem resources:

    ...
    01000000-08ffffff : Crash kernel
    768f0098-768f00a7 : APEI EINJ
    ...
  768f4000-77323fff : ACPI Non-volatile Storage
  77324000-777fefff : ACPI Tables
  777ff000-777fffff : System RAM
  77800000-7fffffff : Reserved
  80000000-8fffffff : PCI MMCONFIG 0000 [bus 00-ff]
  90040000-957fffff : PCI Bus 0000:00
  ...
  300000000-3ffffffff : Persistent Memory (legacy)
  ...

I commented __einj_error_inject during the test and just tested when
injecting a memory error at each start address shown above:
- 0x80000000 and 0x90040000 both failed with EINVAL
- request passed through for all other addresses

Changelog

v1 [1] -> v2:
- In addition to allow IORES_DESC_PERSISTENT_MEMORY_LEGACY, open the
  door wider and only exclude MMIO per suggestion from Tony [2]
- Rebased to commit 11e7861d680c ("Merge tag 'for-linus' of git://git.kernel.org/pub/scm/virt/kvm/kvm")

[1] https://lore.kernel.org/linux-acpi/20250825223348.3780279-1-jiaqiyan@google.com
[2] https://lore.kernel.org/linux-acpi/SJ1PR11MB60835824926BEE57F094DE6FFC39A@SJ1PR11MB6083.namprd11.prod.outlook.com

Signed-off-by: Jiaqi Yan <jiaqiyan@google.com>
---
 drivers/acpi/apei/einj-core.c | 50 +++++++++++++++++++++++++++++------
 1 file changed, 42 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/apei/einj-core.c b/drivers/acpi/apei/einj-core.c
index 2561b045acc7b..904930409fdb2 100644
--- a/drivers/acpi/apei/einj-core.c
+++ b/drivers/acpi/apei/einj-core.c
@@ -656,6 +656,44 @@ static int __einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2,
 	return rc;
 }
 
+/* Allow almost all types of address except MMIO. */
+static bool is_allowed_range(u64 base_addr, u64 size)
+{
+	int i;
+	/*
+	 * MMIO region is usually claimed with IORESOURCE_MEM + IORES_DESC_NONE.
+	 * However, IORES_DESC_NONE is treated like a wildcard when we check if
+	 * region intersects with known resource. So do an allow list check for
+	 * IORES_DESCs that definitely or most likely not MMIO.
+	 */
+	int non_mmio_desc[] = {
+		IORES_DESC_CRASH_KERNEL,
+		IORES_DESC_ACPI_TABLES,
+		IORES_DESC_ACPI_NV_STORAGE,
+		IORES_DESC_PERSISTENT_MEMORY,
+		IORES_DESC_PERSISTENT_MEMORY_LEGACY,
+		/* Treat IORES_DESC_DEVICE_PRIVATE_MEMORY as MMIO. */
+		IORES_DESC_RESERVED,
+		IORES_DESC_SOFT_RESERVED,
+		IORES_DESC_CXL,
+	};
+
+	if (region_intersects(base_addr, size, IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE)
+			      == REGION_INTERSECTS)
+		return true;
+
+	for (i = 0; i < ARRAY_SIZE(non_mmio_desc); ++i) {
+		if (region_intersects(base_addr, size, IORESOURCE_MEM, non_mmio_desc[i])
+				      == REGION_INTERSECTS)
+			return true;
+	}
+
+	if (arch_is_platform_page(base_addr))
+		return true;
+
+	return false;
+}
+
 /* Inject the specified hardware error */
 int einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2, u64 param3,
 		      u64 param4)
@@ -707,14 +745,10 @@ int einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2, u64 param3,
 	base_addr = param1 & param2;
 	size = ~param2 + 1;
 
-	if (((param2 & PAGE_MASK) != PAGE_MASK) ||
-	    ((region_intersects(base_addr, size, IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE)
-				!= REGION_INTERSECTS) &&
-	     (region_intersects(base_addr, size, IORESOURCE_MEM, IORES_DESC_PERSISTENT_MEMORY)
-				!= REGION_INTERSECTS) &&
-	     (region_intersects(base_addr, size, IORESOURCE_MEM, IORES_DESC_SOFT_RESERVED)
-				!= REGION_INTERSECTS) &&
-	     !arch_is_platform_page(base_addr)))
+	if ((param2 & PAGE_MASK) != PAGE_MASK)
+		return -EINVAL;
+
+	if (!is_allowed_range(base_addr, size))
 		return -EINVAL;
 
 	if (is_zero_pfn(base_addr >> PAGE_SHIFT))
-- 
2.51.0.318.gd7df087d1a-goog
Re: [PATCH v2] ACPI: APEI: EINJ: Allow all types of addresses except MMIO
Posted by Hanjun Guo 3 weeks, 3 days ago 
On 2025/8/30 11:02, Jiaqi Yan wrote:
> EINJ driver today only allows injection request to go through for two
> kinds of IORESOURCE_MEM: IORES_DESC_PERSISTENT_MEMORY and
> IORES_DESC_SOFT_RESERVED. This check prevents user of EINJ to test
> memory corrupted in many interesting areas:
> 
> - Legacy persistent memory
> - Memory claimed to be used by ACPI tables or NV storage
> - Kernel crash memory and others
> 
> There is need to test how kernel behaves when something consumes memory
> errors in these memory regions. For example, if certain ACPI table is
> corrupted, does kernel crash gracefully to prevent "silent data
> corruption". For another example, legacy persistent memory, when managed
> by Device DAX, does support recovering from Machine Check Exception
> raised by memory failure, hence worth to be tested.
> 
> However, attempt to inject memory error via EINJ to legacy persistent
> memory or ACPI owned memory fails with -EINVAL.
> 
> Allow EINJ to inject at address except it is MMIO. Leave it to the BIOS
> or firmware to decide what is a legitimate injection target.
> 
> In addition to the test done in [1], on a machine having the following
> iomem resources:
> 
>      ...
>      01000000-08ffffff : Crash kernel
>      768f0098-768f00a7 : APEI EINJ
>      ...
>    768f4000-77323fff : ACPI Non-volatile Storage
>    77324000-777fefff : ACPI Tables
>    777ff000-777fffff : System RAM
>    77800000-7fffffff : Reserved
>    80000000-8fffffff : PCI MMCONFIG 0000 [bus 00-ff]
>    90040000-957fffff : PCI Bus 0000:00
>    ...
>    300000000-3ffffffff : Persistent Memory (legacy)
>    ...
> 
> I commented __einj_error_inject during the test and just tested when
> injecting a memory error at each start address shown above:
> - 0x80000000 and 0x90040000 both failed with EINVAL
> - request passed through for all other addresses
> 

...

> Changelog
> 
> v1 [1] -> v2:
> - In addition to allow IORES_DESC_PERSISTENT_MEMORY_LEGACY, open the
>    door wider and only exclude MMIO per suggestion from Tony [2]
> - Rebased to commit 11e7861d680c ("Merge tag 'for-linus' of git://git.kernel.org/pub/scm/virt/kvm/kvm")
> 
> [1] https://lore.kernel.org/linux-acpi/20250825223348.3780279-1-jiaqiyan@google.com
> [2] https://lore.kernel.org/linux-acpi/SJ1PR11MB60835824926BEE57F094DE6FFC39A@SJ1PR11MB6083.namprd11.prod.outlook.com

For the Changelog, it's better to move it to below...

> 
> Signed-off-by: Jiaqi Yan <jiaqiyan@google.com>
> ---

... here.

>   drivers/acpi/apei/einj-core.c | 50 +++++++++++++++++++++++++++++------
>   1 file changed, 42 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/acpi/apei/einj-core.c b/drivers/acpi/apei/einj-core.c
> index 2561b045acc7b..904930409fdb2 100644
> --- a/drivers/acpi/apei/einj-core.c
> +++ b/drivers/acpi/apei/einj-core.c
> @@ -656,6 +656,44 @@ static int __einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2,
>   	return rc;
>   }
>   
> +/* Allow almost all types of address except MMIO. */
> +static bool is_allowed_range(u64 base_addr, u64 size)
> +{
> +	int i;
> +	/*
> +	 * MMIO region is usually claimed with IORESOURCE_MEM + IORES_DESC_NONE.
> +	 * However, IORES_DESC_NONE is treated like a wildcard when we check if
> +	 * region intersects with known resource. So do an allow list check for
> +	 * IORES_DESCs that definitely or most likely not MMIO.
> +	 */
> +	int non_mmio_desc[] = {
> +		IORES_DESC_CRASH_KERNEL,
> +		IORES_DESC_ACPI_TABLES,
> +		IORES_DESC_ACPI_NV_STORAGE,
> +		IORES_DESC_PERSISTENT_MEMORY,
> +		IORES_DESC_PERSISTENT_MEMORY_LEGACY,
> +		/* Treat IORES_DESC_DEVICE_PRIVATE_MEMORY as MMIO. */
> +		IORES_DESC_RESERVED,
> +		IORES_DESC_SOFT_RESERVED,
> +		IORES_DESC_CXL,

Sorry, I'm not familiar with CXL, but I see the code in einj_error_inject():

/*
  * Injections targeting a CXL 1.0/1.1 port have to be injected
  * via the einj_cxl_rch_error_inject() path as that does the proper
  * validation of the given RCRB base (MMIO) address.
  */
if (einj_is_cxl_error_type(type) && (flags & SETWA_FLAGS_MEM))
	return -EINVAL;

So eject an error for CXL memory, there is a new interface which
means it's not handled here, do we need to remove IORES_DESC_CXL?

Thanks
Hanjun
Re: [PATCH v2] ACPI: APEI: EINJ: Allow all types of addresses except MMIO
Posted by Jiaqi Yan 3 weeks, 2 days ago 
On Tue, Sep 9, 2025 at 1:35 AM Hanjun Guo <guohanjun@huawei.com> wrote:
>
> On 2025/8/30 11:02, Jiaqi Yan wrote:
> > EINJ driver today only allows injection request to go through for two
> > kinds of IORESOURCE_MEM: IORES_DESC_PERSISTENT_MEMORY and
> > IORES_DESC_SOFT_RESERVED. This check prevents user of EINJ to test
> > memory corrupted in many interesting areas:
> >
> > - Legacy persistent memory
> > - Memory claimed to be used by ACPI tables or NV storage
> > - Kernel crash memory and others
> >
> > There is need to test how kernel behaves when something consumes memory
> > errors in these memory regions. For example, if certain ACPI table is
> > corrupted, does kernel crash gracefully to prevent "silent data
> > corruption". For another example, legacy persistent memory, when managed
> > by Device DAX, does support recovering from Machine Check Exception
> > raised by memory failure, hence worth to be tested.
> >
> > However, attempt to inject memory error via EINJ to legacy persistent
> > memory or ACPI owned memory fails with -EINVAL.
> >
> > Allow EINJ to inject at address except it is MMIO. Leave it to the BIOS
> > or firmware to decide what is a legitimate injection target.
> >
> > In addition to the test done in [1], on a machine having the following
> > iomem resources:
> >
> >      ...
> >      01000000-08ffffff : Crash kernel
> >      768f0098-768f00a7 : APEI EINJ
> >      ...
> >    768f4000-77323fff : ACPI Non-volatile Storage
> >    77324000-777fefff : ACPI Tables
> >    777ff000-777fffff : System RAM
> >    77800000-7fffffff : Reserved
> >    80000000-8fffffff : PCI MMCONFIG 0000 [bus 00-ff]
> >    90040000-957fffff : PCI Bus 0000:00
> >    ...
> >    300000000-3ffffffff : Persistent Memory (legacy)
> >    ...
> >
> > I commented __einj_error_inject during the test and just tested when
> > injecting a memory error at each start address shown above:
> > - 0x80000000 and 0x90040000 both failed with EINVAL
> > - request passed through for all other addresses
> >
>
> ...
>
> > Changelog
> >
> > v1 [1] -> v2:
> > - In addition to allow IORES_DESC_PERSISTENT_MEMORY_LEGACY, open the
> >    door wider and only exclude MMIO per suggestion from Tony [2]
> > - Rebased to commit 11e7861d680c ("Merge tag 'for-linus' of git://git.kernel.org/pub/scm/virt/kvm/kvm")
> >
> > [1] https://lore.kernel.org/linux-acpi/20250825223348.3780279-1-jiaqiyan@google.com
> > [2] https://lore.kernel.org/linux-acpi/SJ1PR11MB60835824926BEE57F094DE6FFC39A@SJ1PR11MB6083.namprd11.prod.outlook.com
>
> For the Changelog, it's better to move it to below...
>
> >
> > Signed-off-by: Jiaqi Yan <jiaqiyan@google.com>
> > ---
>
> ... here.

Noted, thanks!

>
> >   drivers/acpi/apei/einj-core.c | 50 +++++++++++++++++++++++++++++------
> >   1 file changed, 42 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/acpi/apei/einj-core.c b/drivers/acpi/apei/einj-core.c
> > index 2561b045acc7b..904930409fdb2 100644
> > --- a/drivers/acpi/apei/einj-core.c
> > +++ b/drivers/acpi/apei/einj-core.c
> > @@ -656,6 +656,44 @@ static int __einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2,
> >       return rc;
> >   }
> >
> > +/* Allow almost all types of address except MMIO. */
> > +static bool is_allowed_range(u64 base_addr, u64 size)
> > +{
> > +     int i;
> > +     /*
> > +      * MMIO region is usually claimed with IORESOURCE_MEM + IORES_DESC_NONE.
> > +      * However, IORES_DESC_NONE is treated like a wildcard when we check if
> > +      * region intersects with known resource. So do an allow list check for
> > +      * IORES_DESCs that definitely or most likely not MMIO.
> > +      */
> > +     int non_mmio_desc[] = {
> > +             IORES_DESC_CRASH_KERNEL,
> > +             IORES_DESC_ACPI_TABLES,
> > +             IORES_DESC_ACPI_NV_STORAGE,
> > +             IORES_DESC_PERSISTENT_MEMORY,
> > +             IORES_DESC_PERSISTENT_MEMORY_LEGACY,
> > +             /* Treat IORES_DESC_DEVICE_PRIVATE_MEMORY as MMIO. */
> > +             IORES_DESC_RESERVED,
> > +             IORES_DESC_SOFT_RESERVED,
> > +             IORES_DESC_CXL,
>
> Sorry, I'm not familiar with CXL, but I see the code in einj_error_inject():
>
> /*
>   * Injections targeting a CXL 1.0/1.1 port have to be injected
>   * via the einj_cxl_rch_error_inject() path as that does the proper
>   * validation of the given RCRB base (MMIO) address.
>   */
> if (einj_is_cxl_error_type(type) && (flags & SETWA_FLAGS_MEM))
>         return -EINVAL;
>
> So eject an error for CXL memory, there is a new interface which
> means it's not handled here, do we need to remove IORES_DESC_CXL?

Thanks for catching this. I agree we should remove IORES_DESC_CXL. It
is unnecessary to check if base_addr intersects IORES_DESC_CXL.

My previous (and totally wrong) thought was, if the error type happens
to be vendor-defined CXL memory, that is, for some reason a vendor
implements EINJ for CXL but not using type bit[12:17],
einj_is_cxl_error_type will be false and the checks for base_addr will
happen, and I wanted the check pass through. Now I think this weird
case should be impossible given no vendor ever tries to add
IORES_DESC_CXL to the previous check.

Will fix both places in V3.

>
> Thanks
> Hanjun

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.