| 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
| ||
|
Message-ID: <345c99af-4f65-6776-fb21-f9fe8662566c@os.amperecomputing.com>
Date: Mon, 7 Mar 2022 13:22:03 -0800
From: doug rady OS <dcrady@...amperecomputing.com>
To: Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
Henry Willard <henry.willard@...cle.com>
Cc: James Morse <James.Morse@....com>,
Mark Rutland <Mark.Rutland@....com>,
Colin King <colin.king@...onical.com>,
"Rafael J . Wysocki" <rjw@...ysocki.net>,
Len Brown <lenb@...nel.org>,
Alexander Viro <viro@...iv.linux.org.uk>,
Andrew Morton <akpm@...ux-foundation.org>,
"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"kernel-janitors@...r.kernel.org" <kernel-janitors@...r.kernel.org>
Subject: Re: [PATCH][V2] ACPI: sysfs: copy ACPI data using io memory copying
On 22-03-01 08:00, Lorenzo Pieralisi wrote:
> On Mon, Feb 28, 2022 at 11:51:23PM +0000, Henry Willard wrote:
>>
>>
>>> On Apr 1, 2020, at 5:44 AM, James Morse <james.morse@....com> wrote:
>>>
>>> Hello!
>>>
>>> On 3/20/20 1:19 PM, Mark Rutland wrote:
>>>> [adding James and Lorenzo]
>>>
>>> (but not actually...)
>>>
>>>
>>>> On Tue, Mar 17, 2020 at 04:54:09PM +0000, Colin King wrote:
>>>>> From: Colin Ian King <colin.king@...onical.com>
>>>>>
>>>>> Reading ACPI data on ARM64 at a non-aligned offset from
>>>>> /sys/firmware/acpi/tables/data/BERT will cause a splat because
>>>>> the data is I/O memory mapped
>>>
>>> On your platform, on someone else's it may be in memory.
>>>
>>> Which platform is this on?
>>> (I've never seen one generate a BERT!)
>>
>> I have seen this on several platforms. The latest is an Altra based machine. It shows up in the Linux Test Project: ltp test "read_all -d /sys -q -r 10”.
>>>
>>>
>>>>> and being read with just a memcpy.
>>>>> Fix this by introducing an I/O variant of memory_read_from_buffer
>>>>> and using I/O memory mapped copies instead.
>>>
>>>> Just to check, is that correct is it correct to map those tables with
>>>> Device attributes in the first place, or should we be mapping the tables
>>>> with Normal Cacheable attributes with memremap()?
>>>>
>>>> If the FW placed those into memory using cacheavble attributes, reading
>>>> them using Device attributes could result in stale values, which could
>>>> be garbage.
>>>
>>> Yes. The BERT code should be using arch_apei_get_mem_attribute() to use the
>>> correct attributes. See ghes_map() for an example. bert_init() will need to use
>>> a version of ioremap() that takes the pgprot_t.
>>>
>>> Always using ioremap_cache() means you get a cacheable mapping, regardless of
>>> how firmware described this region in the UEFI memory map. This doesn't explain
>>> why you got an alignment fault.
>>
>> The BERT error region doesn’t appear in the UEFI memory map on any of the systems I have looked at. This means that acpi_os_map_memory() will always map the area as PROT_DEVICE_nGnRnE, which results in an alignment fault on an unaligned access. For some reason this does not fail on some implementations.
>>
>> It isn’t clear to me from the ACPI spec whether this can be in anything other than normal memory as bert_init() seems to assume it is.
>>
>> We have used this patch to resolve this problem on the assumption it
>> will eventually make it into the mainline kernel. Is there any chance
>> this will happen?
>
> Yes this should be fixed. I put together a patch that I needed to post,
> below.
>
> -- >8 --
> Subject: [PATCH] ACPI: osl: Fix BERT error region memory mapping
I hit this read issue while debugging something else and this addressed
the problem for me on Ampere Altra and AltraMax systems. Previously I
could only read the BERT file using dd with BS of 1-4.
With this patch, now I can 'cp' the BERT file.
Tested-by: Doug Rady <dcrady@...amperecomputing.com>
>
> Currently the sysfs interface maps the BERT error region as "memory"
> (through acpi_os_map_memory()) in order to copy the error records into
> memory buffers through memory operations (eg memory_read_from_buffer()).
>
> The OS system cannot detect whether the BERT error region is part of
> system RAM or it is "device memory" (eg BMC memory) and therefore it
> cannot detect which memory attributes the bus to memory support (and
> corresponding kernel mapping, unless firmware provides the required
> information).
>
> The acpi_os_map_memory() arch backend implementation determines the
> mapping attributes. On arm64, if the BERT error region is not present in
> the EFI memory map, the error region is mapped as device-nGnRnE; this
> triggers alignment faults since memcpy unaligned accesses are not
> allowed in device-nGnRnE regions.
>
> The ACPI sysfs code cannot therefore map by default the BERT error
> region with memory semantics but should use a safer default.
>
> Change the sysfs code to map the BERT error region as MMIO (through
> acpi_os_map_iomem()) and use the memcpy_fromio() interface to read the
> error region into the kernel buffer.
>
> Link: https://lore.kernel.org/linux-arm-kernel/31ffe8fc-f5ee-2858-26c5-0fd8bdd68702@arm.com
> Link: https://lore.kernel.org/linux-acpi/CAJZ5v0g+OVbhuUUDrLUCfX_mVqY_e8ubgLTU98=jfjTeb4t+Pw@mail.gmail.com
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@....com>
> Cc: Ard Biesheuvel <ardb@...nel.org>
> Cc: Will Deacon <will@...nel.org>
> Cc: Hanjun Guo <guohanjun@...wei.com>
> Cc: Sudeep Holla <sudeep.holla@....com>
> Cc: Catalin Marinas <catalin.marinas@....com>
> Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>
> ---
> drivers/acpi/sysfs.c | 25 ++++++++++++++++++-------
> 1 file changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
> index a4b638bea6f1..cc2fe0618178 100644
> --- a/drivers/acpi/sysfs.c
> +++ b/drivers/acpi/sysfs.c
> @@ -415,19 +415,30 @@ static ssize_t acpi_data_show(struct file *filp, struct kobject *kobj,
> loff_t offset, size_t count)
> {
> struct acpi_data_attr *data_attr;
> - void *base;
> - ssize_t rc;
> + void __iomem *base;
> + ssize_t size;
>
> data_attr = container_of(bin_attr, struct acpi_data_attr, attr);
> + size = data_attr->attr.size;
> +
> + if (offset < 0)
> + return -EINVAL;
> +
> + if (offset >= size)
> + return 0;
>
> - base = acpi_os_map_memory(data_attr->addr, data_attr->attr.size);
> + if (count > size - offset)
> + count = size - offset;
> +
> + base = acpi_os_map_iomem(data_attr->addr, size);
> if (!base)
> return -ENOMEM;
> - rc = memory_read_from_buffer(buf, count, &offset, base,
> - data_attr->attr.size);
> - acpi_os_unmap_memory(base, data_attr->attr.size);
>
> - return rc;
> + memcpy_fromio(buf, base + offset, count);
> +
> + acpi_os_unmap_iomem(base, size);
> +
> + return count;
> }
>
> static int acpi_bert_data_init(void *th, struct acpi_data_attr *data_attr)
Powered by blists - more mailing lists