[BUG] kernel side can NOT trigger memory error with einj

[BUG] kernel side can NOT trigger memory error with einj

[Shuai Xue]

Hi folks,

If we inject an memory error at physical memory address, e.g. 0x92f033038,
used by a user space process:

	echo 0x92f033038 > /sys/kernel/debug/apei/einj/param1
	echo 0xfffffffffffff000 > /sys/kernel/debug/apei/einj/param2
	echo 0x1 > /sys/kernel/debug/apei/einj/flags
	echo 0x8 > /sys/kernel/debug/apei/einj/error_type
	echo 1 > /sys/kernel/debug/apei/einj/error_inject

Then the following error will be reported in dmesg:

    ACPI: [Firmware Bug]: requested region covers kernel memory @ 0x000000092f033038

After digging into einj trigger interface, I think it's a kernel bug.

On our platform, firmware relies on kernel to trigger an injected error.
Specifically, it populates trigger_tab with the injected physical memory
address, which is set in param1. It is expected to map the RAM address and
run read action. And the execution path is as follows:

    __einj_error_trigger
        => apei_resources_request
            => apei_exec_pre_map_gars
                => apei_exec_run

The root cause is because:

1. Commit fdea163d8c17 ("ACPI, APEI, EINJ, Fix resource conflict on some
machine") removes the injecting memory address range which conflits with
regular memory from trigger table resources. It make sense when calling
apei_resources_request(). **However, the actual mapping operation in
apei_exec_pre_map_gars() with trigger_ctx. And the conflit physical address
is still in trigger_ctx.**

2. Then apei_exec_pre_map_gars() will finally call acpi_os_ioremap().
The injected physical memory address is EFI_CONVENTIONAL_MEMORY and
memblock_is_map_memory is true (arch/arm64/kernel/acpi.c) so that we see
the printed message.

        case EFI_CONVENTIONAL_MEMORY:
        case EFI_PERSISTENT_MEMORY:
            if (memblock_is_map_memory(phys) ||
                !memblock_is_region_memory(phys, size)) {
                pr_warn(FW_BUG "requested region covers kernel memory @ %pa\n", &phys);
                return NULL;
            }

3. On the other hand, commit ba242d5b1a84 ("ACPI, APEI: Add RAM mapping support to ACPI")
add RAM support with kmap. But after commit aafc65c731fe ("ACPI: add arm64 to the
platforms that use ioremap"), ioremap is used to map memory. However, the
ioremap implementation (arch/arm64/mm/ioremap.c) not allowed to map RAM at
all.

    /*
     * Don't allow RAM to be mapped.
     */
    if (WARN_ON(pfn_valid(__phys_to_pfn(phys_addr))))
        return NULL;

**As a result, the error could not be triggered, which is not expected if we want
to inject an error to a physical page used by process.**

A normal workflow maps Generic Address Register (GAR) by acpi_os_ioremap
and add its virtual address into acpi_ioremaps. The execution path is as
follows:

    apei_exec_pre_map_gars
        => pre_map_gar_callback
            => apei_map_generic_address
                => acpi_os_map_generic_address
                    => acpi_os_map_iomem    /* add mapped VA into acpi_ioremaps */
                        =>    acpi_map
                            => acpi_os_ioremap /**/

Then, a read or write action is taken. It will check if the physical
address is mapped from acpi_ioremap. If yes, the value is read directly.
Otherwise, acpi_os_ioremap the physical address first. The execution path
is as follows:

    __apei_exec_run
        => apei_exec_read_register
            => apei_read
                => acpi_os_read_memory
                    => acpi_map_vaddr_lookup    /* lookup VA of PA from acpi_ioremap */
                    => acpi_os_ioremap

It works well for reserved memory, but not for common case in which we want
to inject normal memory.


A hacking way to address this issue is that map RAM memory with kmap
instead of apei_exec_pre_map_gars, and read it directly instead of
apei_exec_run.
-       rc = apei_exec_pre_map_gars(&trigger_ctx);
-       if (rc)
-               goto out_release;
+       volatile long *ptr;
+       long tmp;
+       unsigned long pfn;
+       pfn = param1 >> PAGE_SHIFT;

-       rc = apei_exec_run(&trigger_ctx, ACPI_EINJ_TRIGGER_ERROR);
+       ptr = kmap(pfn_to_page(pfn));
+       tmp = *(ptr + (param1 & ~ PAGE_MASK));

-       apei_exec_post_unmap_gars(&trigger_ctx);

I am wondering that should we use kmap to map RAM in acpi_map or add a
another path to address this issue? Any comment is welcomed.

Best Regards,
Shuai

Re: [BUG] kernel side can NOT trigger memory error with einj

[Luck, Tony]

On Tue, Mar 08, 2022 at 01:19:12PM +0800, Shuai Xue wrote:
> Hi folks,
> 
> If we inject an memory error at physical memory address, e.g. 0x92f033038,
> used by a user space process:
> 
> 	echo 0x92f033038 > /sys/kernel/debug/apei/einj/param1
> 	echo 0xfffffffffffff000 > /sys/kernel/debug/apei/einj/param2
> 	echo 0x1 > /sys/kernel/debug/apei/einj/flags
> 	echo 0x8 > /sys/kernel/debug/apei/einj/error_type
> 	echo 1 > /sys/kernel/debug/apei/einj/error_inject
> 
> Then the following error will be reported in dmesg:
> 
>     ACPI: [Firmware Bug]: requested region covers kernel memory @ 0x000000092f033038
> 
> After digging into einj trigger interface, I think it's a kernel bug.

I think you are right. This isn't the first bug where Linux tries
to validate addresses supplied by EINJ for Linux to read/write.

I hadn't come across it because I almost always set:

# echo 1 > notrigger

so that I can have some application, or function in the kernel
trigger the error. Instead of running the EINJ trigger action
to make it happen right away.

> I am wondering that should we use kmap to map RAM in acpi_map or add a
> another path to address this issue? Any comment is welcomed.

Perhaps just drop the sanity checks? Just trusting the BIOS? Sounds
radical, but this is validation code where the user is deliberately
injecting errors. If there are BIOS bugs, then people doing validation
may be well positioned to find the BIOS people to make them fix
things.

Problem with this approach is that EINJ calls into the APEI code
that is used for other things besides error injection for validation.
So a blanket removal of sanity checks wouldn't be a good idea.

-Tony

Re: [BUG] kernel side can NOT trigger memory error with einj

[Shuai Xue]

Hi, Tony,

Thank you for your quick reply.

在 2022/3/17 AM1:29, Luck, Tony 写道:
> On Tue, Mar 08, 2022 at 01:19:12PM +0800, Shuai Xue wrote:
>> Hi folks,
>>
>> If we inject an memory error at physical memory address, e.g. 0x92f033038,
>> used by a user space process:
>>
>> 	echo 0x92f033038 > /sys/kernel/debug/apei/einj/param1
>> 	echo 0xfffffffffffff000 > /sys/kernel/debug/apei/einj/param2
>> 	echo 0x1 > /sys/kernel/debug/apei/einj/flags
>> 	echo 0x8 > /sys/kernel/debug/apei/einj/error_type
>> 	echo 1 > /sys/kernel/debug/apei/einj/error_inject
>>
>> Then the following error will be reported in dmesg:
>>
>>     ACPI: [Firmware Bug]: requested region covers kernel memory @ 0x000000092f033038
>>
>> After digging into einj trigger interface, I think it's a kernel bug.
> 
> I think you are right. This isn't the first bug where Linux tries
> to validate addresses supplied by EINJ for Linux to read/write.
> 
> I hadn't come across it because I almost always set:
> 
> # echo 1 > notrigger
> 
> so that I can have some application, or function in the kernel
> trigger the error. Instead of running the EINJ trigger action
> to make it happen right away.

Haha, I know your great test suit, ras-tools. All cases are not triggered
by EINJ tigger action. I have learned a lot from it.

>> I am wondering that should we use kmap to map RAM in acpi_map or add a
>> another path to address this issue? Any comment is welcomed.
> 
> Perhaps just drop the sanity checks? Just trusting the BIOS? Sounds
> radical, but this is validation code where the user is deliberately
> injecting errors. If there are BIOS bugs, then people doing validation
> may be well positioned to find the BIOS people to make them fix
> things.
> 
> Problem with this approach is that EINJ calls into the APEI code
> that is used for other things besides error injection for validation.
> So a blanket removal of sanity checks wouldn't be a good idea.

Agree. A blanket removal of APEI sanity checks is not a good idea. How about
requesting memory with kmap instead APEI API only in __einj_error_trigger()?
Then we would not break the validation of APEI code and could trigger the
injected error.

I have provided a rough code in last mail.

> A hacking way to address this issue is that map RAM memory with kmap
> instead of apei_exec_pre_map_gars, and read it directly instead of
> apei_exec_run.
> -       rc = apei_exec_pre_map_gars(&trigger_ctx);
> -       if (rc)
> -               goto out_release;
> +       volatile long *ptr;
> +       long tmp;
> +       unsigned long pfn;
> +       pfn = param1 >> PAGE_SHIFT;
>
> -       rc = apei_exec_run(&trigger_ctx, ACPI_EINJ_TRIGGER_ERROR);
> +       ptr = kmap(pfn_to_page(pfn));
> +       tmp = *(ptr + (param1 & ~ PAGE_MASK));
>
> -       apei_exec_post_unmap_gars(&trigger_ctx);


Best Regards.
Shuai

RE: [BUG] kernel side can NOT trigger memory error with einj

[Luck, Tony]

> -       rc = apei_exec_run(&trigger_ctx, ACPI_EINJ_TRIGGER_ERROR);
> +       ptr = kmap(pfn_to_page(pfn));
> +       tmp = *(ptr + (param1 & ~ PAGE_MASK));

That hack works when the trigger action is just trying to access the injected
location. But on Intel platforms the trigger "kicks" the patrol scrubber in the
memory controller to access the address. So the error is triggered not by
an access from the core, but by internal memory controller access.

This results in a different error signature (for an uncorrected error injection
it will be a UCNA or SRAO in Intel acronym-speak).

-Tony

Re: [BUG] kernel side can NOT trigger memory error with einj

[Shuai Xue]

在 2022/3/18 AM12:57, Luck, Tony 写道:
>> -       rc = apei_exec_run(&trigger_ctx, ACPI_EINJ_TRIGGER_ERROR);
>> +       ptr = kmap(pfn_to_page(pfn));
>> +       tmp = *(ptr + (param1 & ~ PAGE_MASK));
> 
> That hack works when the trigger action is just trying to access the injected
> location. But on Intel platforms the trigger "kicks" the patrol scrubber in the
> memory controller to access the address. So the error is triggered not by
> an access from the core, but by internal memory controller access.
> 
> This results in a different error signature (for an uncorrected error injection
> it will be a UCNA or SRAO in Intel acronym-speak).

As far as I know, APEI only defines five injection instructions, ACPI_EINJ_READ_REGISTER,
ACPI_EINJ_READ_REGISTER_VALUE, ACPI_EINJ_WRITE_REGISTER, ACPI_EINJ_WRITE_REGISTER_VALUE and
ACPI_EINJ_NOOP. ACPI_EINJ_TRIGGER_ERROR action should run one of them, I don't see
any of them will kick the patrol scrubber. For example, trigger with ACPI_EINJ_READ_REGISTER:

apei_exec_run(&trigger_ctx, ACPI_EINJ_TRIGGER_ERROR)
    __apei_exec_run	// ins=0
        => apei_exec_read_register
            => apei_read
                => acpi_os_read_memory
                    => acpi_map_vaddr_lookup    /* lookup VA of PA from acpi_ioremap */
                    => acpi_os_ioremap
		    => acpi_os_read_iomem
			=> *(u32 *) value = readl(virt_addr);

As we can see, the error is triggered by access from the core. However, the physical
address can NOT be mapped by acpi_os_ioremap.

If I missed anything, please let me know. Thank you very much.

Best Regards,
Shuai

Re: [BUG] kernel side can NOT trigger memory error with einj

[Huang, Ying]

Shuai Xue <xueshuai@linux.alibaba.com> writes:

> 在 2022/3/18 AM12:57, Luck, Tony 写道:
>>> -       rc = apei_exec_run(&trigger_ctx, ACPI_EINJ_TRIGGER_ERROR);
>>> +       ptr = kmap(pfn_to_page(pfn));
>>> +       tmp = *(ptr + (param1 & ~ PAGE_MASK));
>> 
>> That hack works when the trigger action is just trying to access the injected
>> location. But on Intel platforms the trigger "kicks" the patrol scrubber in the
>> memory controller to access the address. So the error is triggered not by
>> an access from the core, but by internal memory controller access.
>> 
>> This results in a different error signature (for an uncorrected error injection
>> it will be a UCNA or SRAO in Intel acronym-speak).
>
> As far as I know, APEI only defines five injection instructions, ACPI_EINJ_READ_REGISTER,
> ACPI_EINJ_READ_REGISTER_VALUE, ACPI_EINJ_WRITE_REGISTER, ACPI_EINJ_WRITE_REGISTER_VALUE and
> ACPI_EINJ_NOOP. ACPI_EINJ_TRIGGER_ERROR action should run one of them, I don't see
> any of them will kick the patrol scrubber. For example, trigger with ACPI_EINJ_READ_REGISTER:
>
> apei_exec_run(&trigger_ctx, ACPI_EINJ_TRIGGER_ERROR)
>     __apei_exec_run	// ins=0
>         => apei_exec_read_register
>             => apei_read
>                 => acpi_os_read_memory
>                     => acpi_map_vaddr_lookup    /* lookup VA of PA from acpi_ioremap */
>                     => acpi_os_ioremap
> 		    => acpi_os_read_iomem
> 			=> *(u32 *) value = readl(virt_addr);
>
> As we can see, the error is triggered by access from the core. However, the physical
> address can NOT be mapped by acpi_os_ioremap.
>
> If I missed anything, please let me know. Thank you very much.

As the name suggested, ACPI_EINJ_READ/WRITE_REGISTER are used to
read/write device registers via iomem.  They aren't used to read/write
normal physical memory.  If that's needed, you can try some other method
I guess.

If you write a device register, the device can kick the patrol scrubber
for you.  This device behavior needs not to be defined in APEI spec.

Best Regards,
Huang, Ying

> Best Regards,
> Shuai

Re: [BUG] kernel side can NOT trigger memory error with einj

[Shuai Xue]

在 2022/3/21 AM10:43, Huang, Ying 写道:
> Shuai Xue <xueshuai@linux.alibaba.com> writes:
> 
>> 在 2022/3/18 AM12:57, Luck, Tony 写道:
>>>> -       rc = apei_exec_run(&trigger_ctx, ACPI_EINJ_TRIGGER_ERROR);
>>>> +       ptr = kmap(pfn_to_page(pfn));
>>>> +       tmp = *(ptr + (param1 & ~ PAGE_MASK));
>>>
>>> That hack works when the trigger action is just trying to access the injected
>>> location. But on Intel platforms the trigger "kicks" the patrol scrubber in the
>>> memory controller to access the address. So the error is triggered not by
>>> an access from the core, but by internal memory controller access.
>>>
>>> This results in a different error signature (for an uncorrected error injection
>>> it will be a UCNA or SRAO in Intel acronym-speak).
>>
>> As far as I know, APEI only defines five injection instructions, ACPI_EINJ_READ_REGISTER,
>> ACPI_EINJ_READ_REGISTER_VALUE, ACPI_EINJ_WRITE_REGISTER, ACPI_EINJ_WRITE_REGISTER_VALUE and
>> ACPI_EINJ_NOOP. ACPI_EINJ_TRIGGER_ERROR action should run one of them, I don't see
>> any of them will kick the patrol scrubber. For example, trigger with ACPI_EINJ_READ_REGISTER:
>>
>> apei_exec_run(&trigger_ctx, ACPI_EINJ_TRIGGER_ERROR)
>>     __apei_exec_run	// ins=0
>>         => apei_exec_read_register
>>             => apei_read
>>                 => acpi_os_read_memory
>>                     => acpi_map_vaddr_lookup    /* lookup VA of PA from acpi_ioremap */
>>                     => acpi_os_ioremap
>> 		    => acpi_os_read_iomem
>> 			=> *(u32 *) value = readl(virt_addr);
>>
>> As we can see, the error is triggered by access from the core. However, the physical
>> address can NOT be mapped by acpi_os_ioremap.
>>
>> If I missed anything, please let me know. Thank you very much.


> If you write a device register, the device can kick the patrol scrubber
> for you.  This device behavior needs not to be defined in APEI spec.

I see, thank you. In our platform, patrol scrubber triggers deferred error, and the fatal
error is triggered by an access from CPU.

> As the name suggested, ACPI_EINJ_READ/WRITE_REGISTER are used to
> read/write device registers via iomem.  They aren't used to read/write
> normal physical memory.  If that's needed, you can try some other method
> I guess.

I think so, should we add new injection instructions to address this problem,
e.g. ACPI_EINJ_READ_MEMORY implemented by kmap?

By the way, commit fdea163d8c17 ("ACPI, APEI, EINJ, Fix resource conflict on some
machine") removes the injecting memory address range which conflits with
regular memory from trigger table resources. It make sense when calling
apei_resources_request(). **However, the actual mapping operation in
apei_exec_pre_map_gars() with trigger_ctx. And the conflit physical address
is still in trigger_ctx.**

		// drivers/acpi/apei/einj.c: __einj_error_trigger
		trigger_param_region = einj_get_trigger_parameter_region(
			trigger_tab, param1, param2);
		if (trigger_param_region) {
			...
		}

If the trigger_param_region is valid which means that the triggered address is
ACPI_ADR_SPACE_SYSTEM_MEMORY, then we should not use apei_exec_pre_map_gars to
map like a register, right? If we have ACPI_EINJ_READ_MEMORY, then we can directly
run ACPI_EINJ_TRIGGER_ERROR through ACPI_EINJ_READ_MEMORY.

Best Regards
Shuai

RE: [BUG] kernel side can NOT trigger memory error with einj

[Luck, Tony]

> As far as I know, APEI only defines five injection instructions, ACPI_EINJ_READ_REGISTER,
> ACPI_EINJ_READ_REGISTER_VALUE, ACPI_EINJ_WRITE_REGISTER, ACPI_EINJ_WRITE_REGISTER_VALUE and
> ACPI_EINJ_NOOP. ACPI_EINJ_TRIGGER_ERROR action should run one of them, I don't see
> any of them will kick the patrol scrubber. For example, trigger with ACPI_EINJ_READ_REGISTER:

Kicking the patrol scrubber is done with a trigger action that writes
to memory controller registers using ACPI_EINJ_WRITE_REGISTER_VALUE.

-Tony