linux-kernel - Re: [PATCH v3] ACPI: APEI: EINJ: Allow more types of addresses except MMIO

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CACw3F52BxoT8KsnMPnuSti2cOWWV=V=WPRZ4Z3biZ-X=6HMCww@mail.gmail.com>
Date: Mon, 15 Sep 2025 14:54:33 -0700
From: Jiaqi Yan <jiaqiyan@...gle.com>
To: "Rafael J. Wysocki" <rafael@...nel.org>, Hanjun Guo <guohanjun@...wei.com>
Cc: tony.luck@...el.com, dan.j.williams@...el.com, bp@...en8.de, 
	mchehab@...nel.org, xueshuai@...ux.alibaba.com, linux-acpi@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] ACPI: APEI: EINJ: Allow more types of addresses except MMIO

On Mon, Sep 15, 2025 at 12:33 PM Rafael J. Wysocki <rafael@...nel.org> wrote:
>
> On Wed, Sep 10, 2025 at 4:57 PM Hanjun Guo <guohanjun@...wei.com> wrote:
> >
> > On 2025/9/10 12:45, 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
> > >
> > > Signed-off-by: Jiaqi Yan <jiaqiyan@...gle.com>
> > > ---
> > >
> > > Changelog
> > >
> > > v2 [2] -> v3:
> > > - Remove unnecessary IORES_DESC_CXL per comment from Hanjun [3].
> > > - Minor update to code comment.
> > >
> > > v1 [1] -> v2:
> > > - In addition to allow IORES_DESC_PERSISTENT_MEMORY_LEGACY, open the
> > >    door wider and only exclude MMIO per suggestion from Tony [4].
> > > - 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/20250830030226.918555-1-jiaqiyan@google.com
> > > [3] https://lore.kernel.org/linux-acpi/bc8ad4b8-c000-0298-efd1-4a332c4c7820@huawei.com
> > > [4] https://lore.kernel.org/linux-acpi/SJ1PR11MB60835824926BEE57F094DE6FFC39A@SJ1PR11MB6083.namprd11.prod.outlook.com
> > >
> > > drivers/acpi/apei/einj-core.c | 51 ++++++++++++++++++++++++++++-------
> > >   1 file changed, 42 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/acpi/apei/einj-core.c b/drivers/acpi/apei/einj-core.c
> > > index 2561b045acc7b..3c87953dbd197 100644
> > > --- a/drivers/acpi/apei/einj-core.c
> > > +++ b/drivers/acpi/apei/einj-core.c
> > > @@ -656,6 +656,43 @@ 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,
> > > +     };
> > > +
> > > +     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)
> > > @@ -702,19 +739,15 @@ int einj_error_inject(u32 type, u32 flags, u64 param1, u64 param2, u64 param3,
> > >        * Disallow crazy address masks that give BIOS leeway to pick
> > >        * injection address almost anywhere. Insist on page or
> > >        * better granularity and that target address is normal RAM or
> > > -      * NVDIMM.
> > > +      * as long as is not MMIO.
> >
> > Thanks for updating this as well.
> >
> > >        */
> > >       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))
> >
> > Reviewed-by: Hanjun Guo <guohanjun@...wei.com>
>
> Applied as 6.18 material, thanks!

Thank you both, Hanjun and Rafael!