[PATCH] ACPI/APEI: Add BERT data driver

[PATCH] ACPI/APEI: Add BERT data driver

[Luck, Tony]

From: Tony Luck <tony.luck@intel.com>

The ACPI Boot Error Record Table provides a method for platform
firmware to give information to the operating system about error
that occurred prior to boot (of particular interest are problems
that caused the previous OS instance to crash).

The BERT table simply provides the size and address of the error
record in BIOS reserved memory. In an earlier age we might have
used /dev/mem to retrieve this error record, but many systems
disable /dev/mem for security reasons.

This driver provides read-only access to the data via a character
special device "/dev/bert-data".

Cc: Len Brown <lenb@kernel.org>
Cc: Boris Petkov <bp@suse.de>
Cc: Tyler Baicar <tbaicar@codeaurora.org>
Cc: linux-acpi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 drivers/acpi/apei/Kconfig     |   7 +++
 drivers/acpi/apei/Makefile    |   1 +
 drivers/acpi/apei/bert-data.c | 107 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 115 insertions(+)
 create mode 100644 drivers/acpi/apei/bert-data.c

diff --git a/drivers/acpi/apei/Kconfig b/drivers/acpi/apei/Kconfig
index de14d49a5c90..a785bfbf7e8c 100644
--- a/drivers/acpi/apei/Kconfig
+++ b/drivers/acpi/apei/Kconfig
@@ -77,3 +77,10 @@ config ACPI_APEI_ERST_DEBUG
 	  error information to and from a persistent store. Enable this
 	  if you want to debugging and testing the ERST kernel support
 	  and firmware implementation.
+
+config ACPI_APEI_BERT_DATA
+	tristate "APEI BERT data driver"
+	depends on ACPI_APEI
+	help
+	  This driver provides read access to the error record that
+	  the ACPI/APEI/BERT table points at.
diff --git a/drivers/acpi/apei/Makefile b/drivers/acpi/apei/Makefile
index e50573de25f1..f092c1bc60b8 100644
--- a/drivers/acpi/apei/Makefile
+++ b/drivers/acpi/apei/Makefile
@@ -2,5 +2,6 @@ obj-$(CONFIG_ACPI_APEI)		+= apei.o
 obj-$(CONFIG_ACPI_APEI_GHES)	+= ghes.o
 obj-$(CONFIG_ACPI_APEI_EINJ)	+= einj.o
 obj-$(CONFIG_ACPI_APEI_ERST_DEBUG) += erst-dbg.o
+obj-$(CONFIG_ACPI_APEI_BERT_DATA) += bert-data.o
 
 apei-y := apei-base.o hest.o erst.o bert.o
diff --git a/drivers/acpi/apei/bert-data.c b/drivers/acpi/apei/bert-data.c
new file mode 100644
index 000000000000..1590bd82ef63
--- /dev/null
+++ b/drivers/acpi/apei/bert-data.c
@@ -0,0 +1,107 @@
+/*
+ * bert-data: driver to provide read access to the error record(s)
+ * provided by the ACPI BERT table.
+ * See ACPI specification section 18.3.1 "Boot Error Source"
+ *
+ * Copyright (C) 2017 Intel Corporation
+ *
+ * Author:
+ *    Tony Luck <tony.luck@intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+#define pr_fmt(fmt) "bert-data: " fmt
+
+#include <linux/module.h>
+#include <linux/acpi.h>
+#include <acpi/acpiosxf.h>
+#include <linux/fs.h>
+#include <linux/miscdevice.h>
+#include <linux/mm.h>
+#include <linux/uaccess.h>
+
+static u32 bert_size;
+static __iomem void *bert_data;
+
+static int bert_chrdev_open(struct inode *inode, struct file *file)
+{
+	if (file->f_flags & (O_WRONLY | O_RDWR))
+		return -EPERM;
+	inode->i_size = bert_size;
+	return 0;
+}
+
+static ssize_t bert_chrdev_read(struct file *filp, char __user *ubuf,
+				size_t usize, loff_t *off)
+{
+	if (*off > bert_size)
+		return -EINVAL;
+	if (*off + usize > bert_size)
+		usize = bert_size - *off;
+	if (copy_to_user(ubuf, bert_data + *off, usize))
+		return -EFAULT;
+	*off += usize;
+	return usize;
+}
+
+static const struct file_operations bert_chrdev_ops = {
+	.open		= bert_chrdev_open,
+	.read		= bert_chrdev_read,
+	.llseek		= default_llseek,
+};
+
+static struct miscdevice bert_chrdev_device = {
+	.minor		= MISC_DYNAMIC_MINOR,
+	.name		= "bert-data",
+	.fops		= &bert_chrdev_ops,
+	.mode		= 0444,
+};
+
+static __init int bert_init(void)
+{
+	struct acpi_table_bert *bert;
+	acpi_status stat;
+	int err;
+
+	if (acpi_disabled)
+		return -ENODEV;
+
+	stat = acpi_get_table(ACPI_SIG_BERT, 0,
+			      (struct acpi_table_header **)&bert);
+	if (stat == AE_NOT_FOUND)
+		return -ENODEV;
+	if (ACPI_FAILURE(stat)) {
+		pr_err("get table failed, %s.\n", acpi_format_exception(stat));
+		return -EINVAL;
+	}
+
+	bert_size = bert->region_length;
+	bert_data = acpi_os_map_memory(bert->address, bert->region_length);
+	acpi_put_table((struct acpi_table_header *)bert);
+	if (!bert_data)
+		return -ENOMEM;
+	err = misc_register(&bert_chrdev_device);
+	if (err)
+		acpi_os_unmap_memory(bert_data, bert_size);
+
+	return err;
+}
+module_init(bert_init);
+
+static __exit void bert_exit(void)
+{
+	acpi_os_unmap_memory(bert_data, bert_size);
+	misc_deregister(&bert_chrdev_device);
+}
+module_exit(bert_exit);
+
+MODULE_DESCRIPTION("ACPI Boot Error Data");
+MODULE_LICENSE("GPL v2");
-- 
2.11.0

Re: [PATCH] ACPI/APEI: Add BERT data driver

[Punit Agrawal]

Hi Tony,

"Luck, Tony" <tony.luck@intel.com> writes:

> From: Tony Luck <tony.luck@intel.com>
>
> The ACPI Boot Error Record Table provides a method for platform
> firmware to give information to the operating system about error
> that occurred prior to boot (of particular interest are problems
> that caused the previous OS instance to crash).
>
> The BERT table simply provides the size and address of the error
> record in BIOS reserved memory. In an earlier age we might have
> used /dev/mem to retrieve this error record, but many systems
> disable /dev/mem for security reasons.
>
> This driver provides read-only access to the data via a character
> special device "/dev/bert-data".

There is already a bert driver which prints the error record. Would it
make sense to integrate the character device there instead of creating a
new driver?

[...]

Re: [PATCH] ACPI/APEI: Add BERT data driver

[Luck, Tony]

On Tue, Aug 15, 2017 at 11:22:06AM +0100, Punit Agrawal wrote:
> There is already a bert driver which prints the error record. Would it
> make sense to integrate the character device there instead of creating a
> new driver?

Like this?  The source code is smaller. But it doesn't offer the option to unload
the driver and unmap the BERT data region after you have retrieved the error record.

Either looks plausible to me (but I'm hardly a disinterested party).

Votes?

-Tony

>From cbeabf2d83fe91eebd960cd5cc1b61faaeed1441 Mon Sep 17 00:00:00 2001
From: Tony Luck <tony.luck@intel.com>
Date: Tue, 15 Aug 2017 13:48:28 -0700
Subject: [PATCH] ACPI: APEI: Extend BERT driver to provide a character device
 to access data

The BERT table simply provides the size and address of the error
record in BIOS reserved memory. Currently this driver decodes the
record to the console log. But other users of BERT may want to access
the full binary record.

In an earlier age we might have used /dev/mem to retrieve this error
record, but many systems disable /dev/mem for security reasons.

Extend this driver to provide read-only access to the data via a character
special device "/dev/bert-data".

Cc: Len Brown <lenb@kernel.org>
Cc: Boris Petkov <bp@suse.de>
Cc: Tyler Baicar <tbaicar@codeaurora.org>
Cc: Punit Agrawal <punit.agrawal@arm.com>
Cc: linux-acpi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Tony Luck <tony.luck@intel.com>

v2: (suggested by Punit Agrawal) don't make a whole new driver, merge
    this functionality into the existing BERT driver.
---
 drivers/acpi/apei/bert.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 42 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/apei/bert.c b/drivers/acpi/apei/bert.c
index 12771fcf0417..9bc39b1bffde 100644
--- a/drivers/acpi/apei/bert.c
+++ b/drivers/acpi/apei/bert.c
@@ -26,12 +26,16 @@
 #include <linux/init.h>
 #include <linux/acpi.h>
 #include <linux/io.h>
+#include <linux/miscdevice.h>
+#include <linux/uaccess.h>
 
 #include "apei-internal.h"
 
 #undef pr_fmt
 #define pr_fmt(fmt) "BERT: " fmt
 
+static __iomem void *bert_data;
+static unsigned int region_len;
 static int bert_disable;
 
 static void __init bert_print_all(struct acpi_bert_region *region,
@@ -95,12 +99,45 @@ static int __init bert_check_table(struct acpi_table_bert *bert_tab)
 	return 0;
 }
 
+static int bert_chrdev_open(struct inode *inode, struct file *file)
+{
+	if (file->f_flags & (O_WRONLY | O_RDWR))
+		return -EPERM;
+	inode->i_size = region_len;
+	return 0;
+}
+
+static ssize_t bert_chrdev_read(struct file *filp, char __user *ubuf,
+				size_t usize, loff_t *off)
+{
+	if (*off > region_len)
+		return -EINVAL;
+	if (*off + usize > region_len)
+		usize = region_len - *off;
+	if (copy_to_user(ubuf, bert_data + *off, usize))
+		return -EFAULT;
+	*off += usize;
+	return usize;
+}
+
+static const struct file_operations bert_chrdev_ops = {
+	.open		= bert_chrdev_open,
+	.read		= bert_chrdev_read,
+	.llseek		= default_llseek,
+};
+
+static struct miscdevice bert_chrdev_device = {
+	.minor		= MISC_DYNAMIC_MINOR,
+	.name		= "bert-data",
+	.fops		= &bert_chrdev_ops,
+	.mode		= 0444,
+};
+
 static int __init bert_init(void)
 {
-	struct apei_resources bert_resources;
 	struct acpi_bert_region *boot_error_region;
+	struct apei_resources bert_resources;
 	struct acpi_table_bert *bert_tab;
-	unsigned int region_len;
 	acpi_status status;
 	int rc = 0;
 
@@ -139,7 +176,9 @@ static int __init bert_init(void)
 	boot_error_region = ioremap_cache(bert_tab->address, region_len);
 	if (boot_error_region) {
 		bert_print_all(boot_error_region, region_len);
-		iounmap(boot_error_region);
+		bert_data = boot_error_region;
+		if (misc_register(&bert_chrdev_device))
+			iounmap(boot_error_region);
 	} else {
 		rc = -ENOMEM;
 	}
-- 
2.11.0

Re: [PATCH] ACPI/APEI: Add BERT data driver

[Punit Agrawal]

"Luck, Tony" <tony.luck@intel.com> writes:

> On Tue, Aug 15, 2017 at 11:22:06AM +0100, Punit Agrawal wrote:
>> There is already a bert driver which prints the error record. Would it
>> make sense to integrate the character device there instead of creating a
>> new driver?
>
> Like this?  The source code is smaller. But it doesn't offer the option to unload
> the driver and unmap the BERT data region after you have retrieved the error record.

Is there any benefit in being able to unload the driver in real world
usage?

It should be possible to convert the existing driver into a loadable
module - thought that means re-printing the error records to the kernel
log if the module is re-loaded. Not sure if that breaks any existing
usecases.

One thing I missed commenting on in the previous version -

Have you thought of exposing the error records via /sys/firmware/acpi?
The tables are already exposed there and as BERT is part of ACPI
logically that's a better fit compared to a misc device.

>
> Either looks plausible to me (but I'm hardly a disinterested party).
>
> Votes?
>
> -Tony
>
> From cbeabf2d83fe91eebd960cd5cc1b61faaeed1441 Mon Sep 17 00:00:00 2001
> From: Tony Luck <tony.luck@intel.com>
> Date: Tue, 15 Aug 2017 13:48:28 -0700
> Subject: [PATCH] ACPI: APEI: Extend BERT driver to provide a character device
>  to access data
>
> The BERT table simply provides the size and address of the error
> record in BIOS reserved memory. Currently this driver decodes the
> record to the console log. But other users of BERT may want to access
> the full binary record.
>
> In an earlier age we might have used /dev/mem to retrieve this error
> record, but many systems disable /dev/mem for security reasons.
>
> Extend this driver to provide read-only access to the data via a character
> special device "/dev/bert-data".
>
> Cc: Len Brown <lenb@kernel.org>
> Cc: Boris Petkov <bp@suse.de>
> Cc: Tyler Baicar <tbaicar@codeaurora.org>
> Cc: Punit Agrawal <punit.agrawal@arm.com>
> Cc: linux-acpi@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Tony Luck <tony.luck@intel.com>
>
> v2: (suggested by Punit Agrawal) don't make a whole new driver, merge
>     this functionality into the existing BERT driver.
> ---
>  drivers/acpi/apei/bert.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 42 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/acpi/apei/bert.c b/drivers/acpi/apei/bert.c
> index 12771fcf0417..9bc39b1bffde 100644
> --- a/drivers/acpi/apei/bert.c
> +++ b/drivers/acpi/apei/bert.c
> @@ -26,12 +26,16 @@
>  #include <linux/init.h>
>  #include <linux/acpi.h>
>  #include <linux/io.h>
> +#include <linux/miscdevice.h>
> +#include <linux/uaccess.h>
>  
>  #include "apei-internal.h"
>  
>  #undef pr_fmt
>  #define pr_fmt(fmt) "BERT: " fmt
>  
> +static __iomem void *bert_data;
> +static unsigned int region_len;
>  static int bert_disable;
>  
>  static void __init bert_print_all(struct acpi_bert_region *region,
> @@ -95,12 +99,45 @@ static int __init bert_check_table(struct acpi_table_bert *bert_tab)
>  	return 0;
>  }
>  
> +static int bert_chrdev_open(struct inode *inode, struct file *file)
> +{
> +	if (file->f_flags & (O_WRONLY | O_RDWR))
> +		return -EPERM;
> +	inode->i_size = region_len;
> +	return 0;
> +}
> +
> +static ssize_t bert_chrdev_read(struct file *filp, char __user *ubuf,
> +				size_t usize, loff_t *off)
> +{
> +	if (*off > region_len)
> +		return -EINVAL;
> +	if (*off + usize > region_len)
> +		usize = region_len - *off;
> +	if (copy_to_user(ubuf, bert_data + *off, usize))
> +		return -EFAULT;
> +	*off += usize;
> +	return usize;
> +}
> +
> +static const struct file_operations bert_chrdev_ops = {
> +	.open		= bert_chrdev_open,
> +	.read		= bert_chrdev_read,
> +	.llseek		= default_llseek,
> +};
> +
> +static struct miscdevice bert_chrdev_device = {
> +	.minor		= MISC_DYNAMIC_MINOR,
> +	.name		= "bert-data",
> +	.fops		= &bert_chrdev_ops,
> +	.mode		= 0444,
> +};
> +
>  static int __init bert_init(void)
>  {
> -	struct apei_resources bert_resources;
>  	struct acpi_bert_region *boot_error_region;
> +	struct apei_resources bert_resources;
>  	struct acpi_table_bert *bert_tab;
> -	unsigned int region_len;
>  	acpi_status status;
>  	int rc = 0;
>  
> @@ -139,7 +176,9 @@ static int __init bert_init(void)
>  	boot_error_region = ioremap_cache(bert_tab->address, region_len);
>  	if (boot_error_region) {
>  		bert_print_all(boot_error_region, region_len);
> -		iounmap(boot_error_region);
> +		bert_data = boot_error_region;
> +		if (misc_register(&bert_chrdev_device))
> +			iounmap(boot_error_region);
>  	} else {
>  		rc = -ENOMEM;
>  	}

RE: [PATCH] ACPI/APEI: Add BERT data driver

[Luck, Tony]

> One thing I missed commenting on in the previous version -
>
> Have you thought of exposing the error records via /sys/firmware/acpi?
> The tables are already exposed there and as BERT is part of ACPI
> logically that's a better fit compared to a misc device.

That was my first thought :-)

But I got stuck on how to name things.  The BERT entry appears in
/sys/firmware/acpi/tables/ ... but the code doesn't know anything special
about "BERT", it just iterates over all the tables and makes them all
appear.

I thought about making it /sys/firmware/acpi/tables/BERT.data, but that
seemed very ugly (this file isn't a "table", so why does it appear in the
"tables" directory? So maybe /sys/firmware/acpi/table-data/BERT? Now
the driver has to make another directory.

Thoughts?

-Tony

Re: [PATCH] ACPI/APEI: Add BERT data driver

[Punit Agrawal]

"Luck, Tony" <tony.luck@intel.com> writes:

>> One thing I missed commenting on in the previous version -
>>
>> Have you thought of exposing the error records via /sys/firmware/acpi?
>> The tables are already exposed there and as BERT is part of ACPI
>> logically that's a better fit compared to a misc device.
>
> That was my first thought :-)
>
> But I got stuck on how to name things.  The BERT entry appears in
> /sys/firmware/acpi/tables/ ... but the code doesn't know anything special
> about "BERT", it just iterates over all the tables and makes them all
> appear.
>
> I thought about making it /sys/firmware/acpi/tables/BERT.data, but that
> seemed very ugly (this file isn't a "table", so why does it appear in the
> "tables" directory? So maybe /sys/firmware/acpi/table-data/BERT? Now
> the driver has to make another directory.
>
> Thoughts?

I agree that keeping the error record out of the tables directory makes
sense as it's not an ACPI table.

The best I could come up with was bert-data or bert-region in
/sys/firmware/acpi/apei/.

Though, I am OK with "table-data/BERT" as well.

>
> -Tony
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ACPI/APEI: Add BERT data driver

[Luck, Tony]

On Thu, Aug 17, 2017 at 11:25:23AM +0100, Punit Agrawal wrote:
> Though, I am OK with "table-data/BERT" as well.

So I coded this up ... and it looks much better than I thought
it might.  A bit larger than the previous version that modified
drivers/acpi/apei/bert.c. But on the plus side easy to extend if
there are other ACPI tables that point at interesting data.

It doesn't handle dynamic tables. That would add a lot of complexity
and BERT isn't dynamic.  Could be a later patch if someone has a
need for it.

All names negotiable.

-Tony


>From 595562691d2747459235ddbc396949337605ac2a Mon Sep 17 00:00:00 2001
From: Tony Luck <tony.luck@intel.com>
Date: Wed, 16 Aug 2017 15:36:28 -0700
Subject: [PATCH] ACPI / sysfs: Extend ACPI sysfs to provide access to boot
 error region

The ACPI sysfs interface provides a way to read each ACPI table from
userspace via entries in /sys/firmware/acpi/tables/

The BERT table simply provides the size and address of the error
record in BIOS reserved memory and users may want access to this
record.

In an earlier age we might have used /dev/mem to retrieve this error
record, but many systems disable /dev/mem for security reasons.

Extend this driver to provide read-only access to the data via a
file in a new directory /sys/firmware/acpi/tables-data/

Cc: Len Brown <lenb@kernel.org>
Cc: Boris Petkov <bp@suse.de>
Cc: Tyler Baicar <tbaicar@codeaurora.org>
Cc: Punit Agrawal <punit.agrawal@arm.com>
Cc: linux-acpi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Tony Luck <tony.luck@intel.com>

v3: (suggested by Punit Agrawal) extend the /sys/firmware/acpi
code to provide this functionality.
---
 drivers/acpi/sysfs.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
index e414fabf7315..b24549a012f4 100644
--- a/drivers/acpi/sysfs.c
+++ b/drivers/acpi/sysfs.c
@@ -306,11 +306,13 @@ module_param_call(acpica_version, NULL, param_get_acpica_version, NULL, 0444);
 /*
  * ACPI table sysfs I/F:
  * /sys/firmware/acpi/tables/
+ * /sys/firmware/acpi/tables-data/
  * /sys/firmware/acpi/tables/dynamic/
  */
 
 static LIST_HEAD(acpi_table_attr_list);
 static struct kobject *tables_kobj;
+static struct kobject *tables_data_kobj;
 static struct kobject *dynamic_tables_kobj;
 static struct kobject *hotplug_kobj;
 
@@ -325,6 +327,11 @@ struct acpi_table_attr {
 	struct list_head node;
 };
 
+struct acpi_data_attr {
+	struct bin_attribute attr;
+	u64	addr;
+};
+
 static ssize_t acpi_table_show(struct file *filp, struct kobject *kobj,
 			       struct bin_attribute *bin_attr, char *buf,
 			       loff_t offset, size_t count)
@@ -420,6 +427,70 @@ acpi_status acpi_sysfs_table_handler(u32 event, void *table, void *context)
 	return AE_OK;
 }
 
+static ssize_t acpi_data_show(struct file *filp, struct kobject *kobj,
+			      struct bin_attribute *bin_attr, char *buf,
+			      loff_t offset, size_t count)
+{
+	struct acpi_data_attr *data_attr;
+	void __iomem *base;
+	ssize_t rc;
+
+	data_attr = container_of(bin_attr, struct acpi_data_attr, attr);
+
+	base = acpi_os_map_memory(data_attr->addr, data_attr->attr.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;
+}
+
+static int acpi_bert_data_init(void *th, struct acpi_data_attr *data_attr)
+{
+	struct acpi_table_bert *bert = th;
+
+	if (bert->header.length < sizeof(struct acpi_table_bert) ||
+	    bert->region_length < sizeof(struct acpi_hest_generic_status)) {
+		kfree(data_attr);
+		return -EINVAL;
+	}
+	data_attr->addr = bert->address;
+	data_attr->attr.size = bert->region_length;
+	data_attr->attr.attr.name = "BERT-DATA";
+
+	return sysfs_create_bin_file(tables_data_kobj, &data_attr->attr);
+}
+
+static struct acpi_data_obj {
+	char *name;
+	int (*fn)(void *, struct acpi_data_attr *);
+} acpi_data_objs[] = {
+	{ ACPI_SIG_BERT, acpi_bert_data_init },
+};
+
+#define NUM_ACPI_DATA_OBJS ARRAY_SIZE(acpi_data_objs)
+
+static int acpi_table_data_init(struct acpi_table_header *th)
+{
+	struct acpi_data_attr *data_attr;
+	int i;
+
+	for (i = 0; i < NUM_ACPI_DATA_OBJS; i++) {
+		if (ACPI_COMPARE_NAME(th->signature, acpi_data_objs[i].name)) {
+			data_attr = kzalloc(sizeof(*data_attr), GFP_KERNEL);
+			if (!data_attr)
+				return -ENOMEM;
+			sysfs_attr_init(&data_attr->attr.attr);
+			data_attr->attr.read = acpi_data_show;
+			data_attr->attr.attr.mode = 0400;
+			return acpi_data_objs[i].fn(th, data_attr);
+		}
+	}
+	return 0;
+}
+
 static int acpi_tables_sysfs_init(void)
 {
 	struct acpi_table_attr *table_attr;
@@ -432,6 +503,10 @@ static int acpi_tables_sysfs_init(void)
 	if (!tables_kobj)
 		goto err;
 
+	tables_data_kobj = kobject_create_and_add("tables-data", acpi_kobj);
+	if (!tables_data_kobj)
+		goto err_tables_data;
+
 	dynamic_tables_kobj = kobject_create_and_add("dynamic", tables_kobj);
 	if (!dynamic_tables_kobj)
 		goto err_dynamic_tables;
@@ -456,13 +531,17 @@ static int acpi_tables_sysfs_init(void)
 			return ret;
 		}
 		list_add_tail(&table_attr->node, &acpi_table_attr_list);
+		acpi_table_data_init(table_header);
 	}
 
 	kobject_uevent(tables_kobj, KOBJ_ADD);
+	kobject_uevent(tables_data_kobj, KOBJ_ADD);
 	kobject_uevent(dynamic_tables_kobj, KOBJ_ADD);
 
 	return 0;
 err_dynamic_tables:
+	kobject_put(tables_data_kobj);
+err_tables_data:
 	kobject_put(tables_kobj);
 err:
 	return -ENOMEM;
-- 
2.11.0

Re: [PATCH] ACPI/APEI: Add BERT data driver

[Rafael J. Wysocki]

On Thu, Aug 17, 2017 at 7:49 PM, Luck, Tony <tony.luck@intel.com> wrote:
> On Thu, Aug 17, 2017 at 11:25:23AM +0100, Punit Agrawal wrote:
>> Though, I am OK with "table-data/BERT" as well.
>
> So I coded this up ... and it looks much better than I thought
> it might.  A bit larger than the previous version that modified
> drivers/acpi/apei/bert.c. But on the plus side easy to extend if
> there are other ACPI tables that point at interesting data.
>
> It doesn't handle dynamic tables. That would add a lot of complexity
> and BERT isn't dynamic.  Could be a later patch if someone has a
> need for it.
>
> All names negotiable.

OK

What about /sys/firmware/acpi/tables/data/ and then one file per table
under it with the same name as the table file under tables/ ?

So in particular for BERT the data would be in
/sys/firmware/acpi/tables/data/BERT ?

Thanks,
Rafael

Re: [PATCH] ACPI/APEI: Add BERT data driver

[Luck, Tony]

On Thu, Aug 17, 2017 at 09:28:53PM +0200, Rafael J. Wysocki wrote:
> What about /sys/firmware/acpi/tables/data/ and then one file per table
> under it with the same name as the table file under tables/ ?

"data" works. ACPI table names are all upper-case, so it
can't conflict. Any programs that scan the whole directory
have to already skip "dynamic", so they likely would not
be confused by the appearance of a second directory.

For BERT there is only one blob of data. So naming it the
same as the table is fine.  Maybe other tables might have
more than one thing? If so, they could use the table name
as a prefix, or make a subdirectory. We can cross that
bridge if anyone finds additional useful things to add.

> So in particular for BERT the data would be in
> /sys/firmware/acpi/tables/data/BERT ?

Ok. Will re-spin the patch with these names.

-Tony

Re: [PATCH] ACPI/APEI: Add BERT data driver

[Rafael J. Wysocki]

On Thu, Aug 17, 2017 at 10:29 PM, Luck, Tony <tony.luck@intel.com> wrote:
> On Thu, Aug 17, 2017 at 09:28:53PM +0200, Rafael J. Wysocki wrote:
>> What about /sys/firmware/acpi/tables/data/ and then one file per table
>> under it with the same name as the table file under tables/ ?
>
> "data" works. ACPI table names are all upper-case, so it
> can't conflict. Any programs that scan the whole directory
> have to already skip "dynamic", so they likely would not
> be confused by the appearance of a second directory.
>
> For BERT there is only one blob of data. So naming it the
> same as the table is fine.  Maybe other tables might have
> more than one thing? If so, they could use the table name
> as a prefix, or make a subdirectory. We can cross that
> bridge if anyone finds additional useful things to add.

Actually, there already are multiple SSDTs on the majority of systems
and the corresponding files under tables/ are called SSDT1, SSDT2
etc., so we just need to follow the existing convention.

Thanks,
Rafael

[PATCH] ACPI / sysfs: Extend ACPI sysfs to provide access to boot error region

[Luck, Tony]

From: Tony Luck <tony.luck@intel.com>

The ACPI sysfs interface provides a way to read each ACPI table from
userspace via entries in /sys/firmware/acpi/tables/

The BERT table simply provides the size and address of the error
record in BIOS reserved memory and users may want access to this
record.

In an earlier age we might have used /dev/mem to retrieve this error
record, but many systems disable /dev/mem for security reasons.

Extend this driver to provide read-only access to the data via a
file in a new directory /sys/firmware/acpi/tables/data/BERT

Cc: Len Brown <lenb@kernel.org>
Cc: Boris Petkov <bp@suse.de>
Cc: Tyler Baicar <tbaicar@codeaurora.org>
Cc: Punit Agrawal <punit.agrawal@arm.com>
Cc: linux-acpi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Tony Luck <tony.luck@intel.com>

v4: (suggested by Punit Agrawal) extend the /sys/firmware/acpi
code to provide this functionality. File/directory names changed
to Rafael's suggestions
---
 drivers/acpi/sysfs.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
index e414fabf7315..7c4eed0594d0 100644
--- a/drivers/acpi/sysfs.c
+++ b/drivers/acpi/sysfs.c
@@ -306,11 +306,13 @@ module_param_call(acpica_version, NULL, param_get_acpica_version, NULL, 0444);
 /*
  * ACPI table sysfs I/F:
  * /sys/firmware/acpi/tables/
+ * /sys/firmware/acpi/tables-data/
  * /sys/firmware/acpi/tables/dynamic/
  */
 
 static LIST_HEAD(acpi_table_attr_list);
 static struct kobject *tables_kobj;
+static struct kobject *tables_data_kobj;
 static struct kobject *dynamic_tables_kobj;
 static struct kobject *hotplug_kobj;
 
@@ -325,6 +327,11 @@ struct acpi_table_attr {
 	struct list_head node;
 };
 
+struct acpi_data_attr {
+	struct bin_attribute attr;
+	u64	addr;
+};
+
 static ssize_t acpi_table_show(struct file *filp, struct kobject *kobj,
 			       struct bin_attribute *bin_attr, char *buf,
 			       loff_t offset, size_t count)
@@ -420,6 +427,70 @@ acpi_status acpi_sysfs_table_handler(u32 event, void *table, void *context)
 	return AE_OK;
 }
 
+static ssize_t acpi_data_show(struct file *filp, struct kobject *kobj,
+			      struct bin_attribute *bin_attr, char *buf,
+			      loff_t offset, size_t count)
+{
+	struct acpi_data_attr *data_attr;
+	void __iomem *base;
+	ssize_t rc;
+
+	data_attr = container_of(bin_attr, struct acpi_data_attr, attr);
+
+	base = acpi_os_map_memory(data_attr->addr, data_attr->attr.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;
+}
+
+static int acpi_bert_data_init(void *th, struct acpi_data_attr *data_attr)
+{
+	struct acpi_table_bert *bert = th;
+
+	if (bert->header.length < sizeof(struct acpi_table_bert) ||
+	    bert->region_length < sizeof(struct acpi_hest_generic_status)) {
+		kfree(data_attr);
+		return -EINVAL;
+	}
+	data_attr->addr = bert->address;
+	data_attr->attr.size = bert->region_length;
+	data_attr->attr.attr.name = "BERT";
+
+	return sysfs_create_bin_file(tables_data_kobj, &data_attr->attr);
+}
+
+static struct acpi_data_obj {
+	char *name;
+	int (*fn)(void *, struct acpi_data_attr *);
+} acpi_data_objs[] = {
+	{ ACPI_SIG_BERT, acpi_bert_data_init },
+};
+
+#define NUM_ACPI_DATA_OBJS ARRAY_SIZE(acpi_data_objs)
+
+static int acpi_table_data_init(struct acpi_table_header *th)
+{
+	struct acpi_data_attr *data_attr;
+	int i;
+
+	for (i = 0; i < NUM_ACPI_DATA_OBJS; i++) {
+		if (ACPI_COMPARE_NAME(th->signature, acpi_data_objs[i].name)) {
+			data_attr = kzalloc(sizeof(*data_attr), GFP_KERNEL);
+			if (!data_attr)
+				return -ENOMEM;
+			sysfs_attr_init(&data_attr->attr.attr);
+			data_attr->attr.read = acpi_data_show;
+			data_attr->attr.attr.mode = 0400;
+			return acpi_data_objs[i].fn(th, data_attr);
+		}
+	}
+	return 0;
+}
+
 static int acpi_tables_sysfs_init(void)
 {
 	struct acpi_table_attr *table_attr;
@@ -432,6 +503,10 @@ static int acpi_tables_sysfs_init(void)
 	if (!tables_kobj)
 		goto err;
 
+	tables_data_kobj = kobject_create_and_add("data", tables_kobj);
+	if (!tables_data_kobj)
+		goto err_tables_data;
+
 	dynamic_tables_kobj = kobject_create_and_add("dynamic", tables_kobj);
 	if (!dynamic_tables_kobj)
 		goto err_dynamic_tables;
@@ -456,13 +531,17 @@ static int acpi_tables_sysfs_init(void)
 			return ret;
 		}
 		list_add_tail(&table_attr->node, &acpi_table_attr_list);
+		acpi_table_data_init(table_header);
 	}
 
 	kobject_uevent(tables_kobj, KOBJ_ADD);
+	kobject_uevent(tables_data_kobj, KOBJ_ADD);
 	kobject_uevent(dynamic_tables_kobj, KOBJ_ADD);
 
 	return 0;
 err_dynamic_tables:
+	kobject_put(tables_data_kobj);
+err_tables_data:
 	kobject_put(tables_kobj);
 err:
 	return -ENOMEM;
-- 
2.11.0

Re: [PATCH] ACPI / sysfs: Extend ACPI sysfs to provide access to boot error region

[Alan Cox]

On Thu, 17 Aug 2017 14:39:46 -0700
"Luck, Tony" <tony.luck@intel.com> wrote:

> From: Tony Luck <tony.luck@intel.com>
> 
> The ACPI sysfs interface provides a way to read each ACPI table from
> userspace via entries in /sys/firmware/acpi/tables/
> 
> The BERT table simply provides the size and address of the error
> record in BIOS reserved memory and users may want access to this
> record.
> 
> In an earlier age we might have used /dev/mem to retrieve this error
> record, but many systems disable /dev/mem for security reasons.
> 
> Extend this driver to provide read-only access to the data via a
> file in a new directory /sys/firmware/acpi/tables/data/BERT

Should this not also have a capability check. Assuming file permissions
are sufficient for grabbing a chunk of system memory holding error
info doesn't seem too scary but it's at odds with a lot of other cases ?

Alan

RE: [PATCH] ACPI / sysfs: Extend ACPI sysfs to provide access to boot error region

[Luck, Tony]

> Should this not also have a capability check. Assuming file permissions
> are sufficient for grabbing a chunk of system memory holding error
> info doesn't seem too scary but it's at odds with a lot of other cases ?

At least one of those other cases (pstore) added a capability check and now regret
it.  There's a thread on reverting it. Look for:

   Revert "pstore: Honor dmesg_restrict sysctl on dmesg dumps"

-Tony

Re: [PATCH] ACPI / sysfs: Extend ACPI sysfs to provide access to boot error region

[Punit Agrawal]

"Luck, Tony" <tony.luck@intel.com> writes:

> From: Tony Luck <tony.luck@intel.com>
>
> The ACPI sysfs interface provides a way to read each ACPI table from
> userspace via entries in /sys/firmware/acpi/tables/
>
> The BERT table simply provides the size and address of the error
> record in BIOS reserved memory and users may want access to this
> record.
>
> In an earlier age we might have used /dev/mem to retrieve this error
> record, but many systems disable /dev/mem for security reasons.
>
> Extend this driver to provide read-only access to the data via a
> file in a new directory /sys/firmware/acpi/tables/data/BERT
>
> Cc: Len Brown <lenb@kernel.org>
> Cc: Boris Petkov <bp@suse.de>
> Cc: Tyler Baicar <tbaicar@codeaurora.org>
> Cc: Punit Agrawal <punit.agrawal@arm.com>
> Cc: linux-acpi@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Tony Luck <tony.luck@intel.com>
>
> v4: (suggested by Punit Agrawal) extend the /sys/firmware/acpi
> code to provide this functionality. File/directory names changed
> to Rafael's suggestions
> ---
>  drivers/acpi/sysfs.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
>
> diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
> index e414fabf7315..7c4eed0594d0 100644
> --- a/drivers/acpi/sysfs.c
> +++ b/drivers/acpi/sysfs.c
> @@ -306,11 +306,13 @@ module_param_call(acpica_version, NULL, param_get_acpica_version, NULL, 0444);
>  /*
>   * ACPI table sysfs I/F:
>   * /sys/firmware/acpi/tables/
> + * /sys/firmware/acpi/tables-data/

Typo: Should be "/sys/firmware/acpi/tables/data/"

>   * /sys/firmware/acpi/tables/dynamic/
>   */
>  
>  static LIST_HEAD(acpi_table_attr_list);
>  static struct kobject *tables_kobj;
> +static struct kobject *tables_data_kobj;
>  static struct kobject *dynamic_tables_kobj;
>  static struct kobject *hotplug_kobj;
>  
> @@ -325,6 +327,11 @@ struct acpi_table_attr {
>  	struct list_head node;
>  };
>  
> +struct acpi_data_attr {
> +	struct bin_attribute attr;
> +	u64	addr;
> +};

If all we are going to need is an address, it could also be stored in
the "private" member of attr (similar to how it's done in
pci-sysfs.c). But it goes against usage in the rest of the file so I am
fine either ways.

> +
>  static ssize_t acpi_table_show(struct file *filp, struct kobject *kobj,
>  			       struct bin_attribute *bin_attr, char *buf,
>  			       loff_t offset, size_t count)
> @@ -420,6 +427,70 @@ acpi_status acpi_sysfs_table_handler(u32 event, void *table, void *context)
>  	return AE_OK;
>  }
>  
> +static ssize_t acpi_data_show(struct file *filp, struct kobject *kobj,
> +			      struct bin_attribute *bin_attr, char *buf,
> +			      loff_t offset, size_t count)
> +{
> +	struct acpi_data_attr *data_attr;
> +	void __iomem *base;
> +	ssize_t rc;
> +
> +	data_attr = container_of(bin_attr, struct acpi_data_attr, attr);
> +
> +	base = acpi_os_map_memory(data_attr->addr, data_attr->attr.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;
> +}
> +
> +static int acpi_bert_data_init(void *th, struct acpi_data_attr *data_attr)
> +{
> +	struct acpi_table_bert *bert = th;
> +
> +	if (bert->header.length < sizeof(struct acpi_table_bert) ||
> +	    bert->region_length < sizeof(struct acpi_hest_generic_status)) {
> +		kfree(data_attr);
> +		return -EINVAL;
> +	}
> +	data_attr->addr = bert->address;
> +	data_attr->attr.size = bert->region_length;
> +	data_attr->attr.attr.name = "BERT";
> +
> +	return sysfs_create_bin_file(tables_data_kobj, &data_attr->attr);
> +}
> +
> +static struct acpi_data_obj {
> +	char *name;
> +	int (*fn)(void *, struct acpi_data_attr *);
> +} acpi_data_objs[] = {
> +	{ ACPI_SIG_BERT, acpi_bert_data_init },
> +};
> +
> +#define NUM_ACPI_DATA_OBJS ARRAY_SIZE(acpi_data_objs)
> +
> +static int acpi_table_data_init(struct acpi_table_header *th)
> +{
> +	struct acpi_data_attr *data_attr;
> +	int i;
> +
> +	for (i = 0; i < NUM_ACPI_DATA_OBJS; i++) {
> +		if (ACPI_COMPARE_NAME(th->signature, acpi_data_objs[i].name)) {
> +			data_attr = kzalloc(sizeof(*data_attr), GFP_KERNEL);
> +			if (!data_attr)
> +				return -ENOMEM;
> +			sysfs_attr_init(&data_attr->attr.attr);
> +			data_attr->attr.read = acpi_data_show;
> +			data_attr->attr.attr.mode = 0400;
> +			return acpi_data_objs[i].fn(th, data_attr);
> +		}
> +	}
> +	return 0;
> +}
> +
>  static int acpi_tables_sysfs_init(void)
>  {
>  	struct acpi_table_attr *table_attr;
> @@ -432,6 +503,10 @@ static int acpi_tables_sysfs_init(void)
>  	if (!tables_kobj)
>  		goto err;
>  
> +	tables_data_kobj = kobject_create_and_add("data", tables_kobj);
> +	if (!tables_data_kobj)
> +		goto err_tables_data;
> +
>  	dynamic_tables_kobj = kobject_create_and_add("dynamic", tables_kobj);
>  	if (!dynamic_tables_kobj)
>  		goto err_dynamic_tables;
> @@ -456,13 +531,17 @@ static int acpi_tables_sysfs_init(void)
>  			return ret;
>  		}
>  		list_add_tail(&table_attr->node, &acpi_table_attr_list);
> +		acpi_table_data_init(table_header);
>  	}
>  
>  	kobject_uevent(tables_kobj, KOBJ_ADD);
> +	kobject_uevent(tables_data_kobj, KOBJ_ADD);
>  	kobject_uevent(dynamic_tables_kobj, KOBJ_ADD);
>  
>  	return 0;
>  err_dynamic_tables:
> +	kobject_put(tables_data_kobj);
> +err_tables_data:
>  	kobject_put(tables_kobj);
>  err:
>  	return -ENOMEM;

I like how this fits into existing infrastructure.

With the typo addressed -

Acked-by: Punit Agrawal <punit.agrawal@arm.com>

Thanks,
Punit

[PATCH v4] ACPI / sysfs: Extend ACPI sysfs to provide access to boot error region

[Luck, Tony]

From: Tony Luck <tony.luck@intel.com>

The ACPI sysfs interface provides a way to read each ACPI table from
userspace via entries in /sys/firmware/acpi/tables/

The BERT table simply provides the size and address of the error
record in BIOS reserved memory and users may want access to this
record.

In an earlier age we might have used /dev/mem to retrieve this error
record, but many systems disable /dev/mem for security reasons.

Extend this driver to provide read-only access to the data via a
file in a new directory /sys/firmware/acpi/tables/data/BERT

Cc: Len Brown <lenb@kernel.org>
Cc: Boris Petkov <bp@suse.de>
Cc: Tyler Baicar <tbaicar@codeaurora.org>
Cc: linux-acpi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Acked-by: Punit Agrawal <punit.agrawal@arm.com>
Signed-off-by: Tony Luck <tony.luck@intel.com>

v4: fix typo reported by Punit
---
 drivers/acpi/sysfs.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
index e414fabf7315..faa1aa3ed0e1 100644
--- a/drivers/acpi/sysfs.c
+++ b/drivers/acpi/sysfs.c
@@ -306,11 +306,13 @@ module_param_call(acpica_version, NULL, param_get_acpica_version, NULL, 0444);
 /*
  * ACPI table sysfs I/F:
  * /sys/firmware/acpi/tables/
+ * /sys/firmware/acpi/tables/data/
  * /sys/firmware/acpi/tables/dynamic/
  */
 
 static LIST_HEAD(acpi_table_attr_list);
 static struct kobject *tables_kobj;
+static struct kobject *tables_data_kobj;
 static struct kobject *dynamic_tables_kobj;
 static struct kobject *hotplug_kobj;
 
@@ -325,6 +327,11 @@ struct acpi_table_attr {
 	struct list_head node;
 };
 
+struct acpi_data_attr {
+	struct bin_attribute attr;
+	u64	addr;
+};
+
 static ssize_t acpi_table_show(struct file *filp, struct kobject *kobj,
 			       struct bin_attribute *bin_attr, char *buf,
 			       loff_t offset, size_t count)
@@ -420,6 +427,70 @@ acpi_status acpi_sysfs_table_handler(u32 event, void *table, void *context)
 	return AE_OK;
 }
 
+static ssize_t acpi_data_show(struct file *filp, struct kobject *kobj,
+			      struct bin_attribute *bin_attr, char *buf,
+			      loff_t offset, size_t count)
+{
+	struct acpi_data_attr *data_attr;
+	void __iomem *base;
+	ssize_t rc;
+
+	data_attr = container_of(bin_attr, struct acpi_data_attr, attr);
+
+	base = acpi_os_map_memory(data_attr->addr, data_attr->attr.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;
+}
+
+static int acpi_bert_data_init(void *th, struct acpi_data_attr *data_attr)
+{
+	struct acpi_table_bert *bert = th;
+
+	if (bert->header.length < sizeof(struct acpi_table_bert) ||
+	    bert->region_length < sizeof(struct acpi_hest_generic_status)) {
+		kfree(data_attr);
+		return -EINVAL;
+	}
+	data_attr->addr = bert->address;
+	data_attr->attr.size = bert->region_length;
+	data_attr->attr.attr.name = "BERT";
+
+	return sysfs_create_bin_file(tables_data_kobj, &data_attr->attr);
+}
+
+static struct acpi_data_obj {
+	char *name;
+	int (*fn)(void *, struct acpi_data_attr *);
+} acpi_data_objs[] = {
+	{ ACPI_SIG_BERT, acpi_bert_data_init },
+};
+
+#define NUM_ACPI_DATA_OBJS ARRAY_SIZE(acpi_data_objs)
+
+static int acpi_table_data_init(struct acpi_table_header *th)
+{
+	struct acpi_data_attr *data_attr;
+	int i;
+
+	for (i = 0; i < NUM_ACPI_DATA_OBJS; i++) {
+		if (ACPI_COMPARE_NAME(th->signature, acpi_data_objs[i].name)) {
+			data_attr = kzalloc(sizeof(*data_attr), GFP_KERNEL);
+			if (!data_attr)
+				return -ENOMEM;
+			sysfs_attr_init(&data_attr->attr.attr);
+			data_attr->attr.read = acpi_data_show;
+			data_attr->attr.attr.mode = 0400;
+			return acpi_data_objs[i].fn(th, data_attr);
+		}
+	}
+	return 0;
+}
+
 static int acpi_tables_sysfs_init(void)
 {
 	struct acpi_table_attr *table_attr;
@@ -432,6 +503,10 @@ static int acpi_tables_sysfs_init(void)
 	if (!tables_kobj)
 		goto err;
 
+	tables_data_kobj = kobject_create_and_add("data", tables_kobj);
+	if (!tables_data_kobj)
+		goto err_tables_data;
+
 	dynamic_tables_kobj = kobject_create_and_add("dynamic", tables_kobj);
 	if (!dynamic_tables_kobj)
 		goto err_dynamic_tables;
@@ -456,13 +531,17 @@ static int acpi_tables_sysfs_init(void)
 			return ret;
 		}
 		list_add_tail(&table_attr->node, &acpi_table_attr_list);
+		acpi_table_data_init(table_header);
 	}
 
 	kobject_uevent(tables_kobj, KOBJ_ADD);
+	kobject_uevent(tables_data_kobj, KOBJ_ADD);
 	kobject_uevent(dynamic_tables_kobj, KOBJ_ADD);
 
 	return 0;
 err_dynamic_tables:
+	kobject_put(tables_data_kobj);
+err_tables_data:
 	kobject_put(tables_kobj);
 err:
 	return -ENOMEM;
-- 
2.11.0

RE: [PATCH] ACPI / sysfs: Extend ACPI sysfs to provide access to boot error region

[Luck, Tony]

>> Should this not also have a capability check. Assuming file permissions
>> are sufficient for grabbing a chunk of system memory holding error
>> info doesn't seem too scary but it's at odds with a lot of other cases ?
>
> At least one of those other cases (pstore) added a capability check and now regret
> it.  There's a thread on reverting it. Look for:
>
>   Revert "pstore: Honor dmesg_restrict sysctl on dmesg dumps"

Here's at least part of that thread:

https://marc.info/?l=linux-kernel&m=150301241114262&w=2

Kees: you were OK with removing the capability check from pstore, right?

I'm now adding another place to access a blob of memory containing
crash information from the previous kernel (pointed at by the ACPI
BERT record).

-Tony

Re: [PATCH v4] ACPI / sysfs: Extend ACPI sysfs to provide access to boot error region

[Rafael J. Wysocki]

On Saturday, August 19, 2017 1:19:00 AM CEST Luck, Tony wrote:
> From: Tony Luck <tony.luck@intel.com>
> 
> The ACPI sysfs interface provides a way to read each ACPI table from
> userspace via entries in /sys/firmware/acpi/tables/
> 
> The BERT table simply provides the size and address of the error
> record in BIOS reserved memory and users may want access to this
> record.
> 
> In an earlier age we might have used /dev/mem to retrieve this error
> record, but many systems disable /dev/mem for security reasons.
> 
> Extend this driver to provide read-only access to the data via a
> file in a new directory /sys/firmware/acpi/tables/data/BERT
> 
> Cc: Len Brown <lenb@kernel.org>
> Cc: Boris Petkov <bp@suse.de>
> Cc: Tyler Baicar <tbaicar@codeaurora.org>
> Cc: linux-acpi@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Acked-by: Punit Agrawal <punit.agrawal@arm.com>
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> 
> v4: fix typo reported by Punit
> ---
>  drivers/acpi/sysfs.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
> 
> diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
> index e414fabf7315..faa1aa3ed0e1 100644
> --- a/drivers/acpi/sysfs.c
> +++ b/drivers/acpi/sysfs.c
> @@ -306,11 +306,13 @@ module_param_call(acpica_version, NULL, param_get_acpica_version, NULL, 0444);
>  /*
>   * ACPI table sysfs I/F:
>   * /sys/firmware/acpi/tables/
> + * /sys/firmware/acpi/tables/data/
>   * /sys/firmware/acpi/tables/dynamic/
>   */
>  
>  static LIST_HEAD(acpi_table_attr_list);
>  static struct kobject *tables_kobj;
> +static struct kobject *tables_data_kobj;
>  static struct kobject *dynamic_tables_kobj;
>  static struct kobject *hotplug_kobj;
>  
> @@ -325,6 +327,11 @@ struct acpi_table_attr {
>  	struct list_head node;
>  };
>  
> +struct acpi_data_attr {
> +	struct bin_attribute attr;
> +	u64	addr;
> +};
> +
>  static ssize_t acpi_table_show(struct file *filp, struct kobject *kobj,
>  			       struct bin_attribute *bin_attr, char *buf,
>  			       loff_t offset, size_t count)
> @@ -420,6 +427,70 @@ acpi_status acpi_sysfs_table_handler(u32 event, void *table, void *context)
>  	return AE_OK;
>  }
>  
> +static ssize_t acpi_data_show(struct file *filp, struct kobject *kobj,
> +			      struct bin_attribute *bin_attr, char *buf,
> +			      loff_t offset, size_t count)
> +{
> +	struct acpi_data_attr *data_attr;
> +	void __iomem *base;
> +	ssize_t rc;
> +
> +	data_attr = container_of(bin_attr, struct acpi_data_attr, attr);
> +
> +	base = acpi_os_map_memory(data_attr->addr, data_attr->attr.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;
> +}
> +
> +static int acpi_bert_data_init(void *th, struct acpi_data_attr *data_attr)
> +{
> +	struct acpi_table_bert *bert = th;
> +
> +	if (bert->header.length < sizeof(struct acpi_table_bert) ||
> +	    bert->region_length < sizeof(struct acpi_hest_generic_status)) {
> +		kfree(data_attr);
> +		return -EINVAL;
> +	}
> +	data_attr->addr = bert->address;
> +	data_attr->attr.size = bert->region_length;
> +	data_attr->attr.attr.name = "BERT";
> +
> +	return sysfs_create_bin_file(tables_data_kobj, &data_attr->attr);
> +}
> +
> +static struct acpi_data_obj {
> +	char *name;
> +	int (*fn)(void *, struct acpi_data_attr *);
> +} acpi_data_objs[] = {
> +	{ ACPI_SIG_BERT, acpi_bert_data_init },
> +};
> +
> +#define NUM_ACPI_DATA_OBJS ARRAY_SIZE(acpi_data_objs)
> +
> +static int acpi_table_data_init(struct acpi_table_header *th)
> +{
> +	struct acpi_data_attr *data_attr;
> +	int i;
> +
> +	for (i = 0; i < NUM_ACPI_DATA_OBJS; i++) {
> +		if (ACPI_COMPARE_NAME(th->signature, acpi_data_objs[i].name)) {
> +			data_attr = kzalloc(sizeof(*data_attr), GFP_KERNEL);
> +			if (!data_attr)
> +				return -ENOMEM;
> +			sysfs_attr_init(&data_attr->attr.attr);
> +			data_attr->attr.read = acpi_data_show;
> +			data_attr->attr.attr.mode = 0400;
> +			return acpi_data_objs[i].fn(th, data_attr);
> +		}
> +	}
> +	return 0;
> +}
> +
>  static int acpi_tables_sysfs_init(void)
>  {
>  	struct acpi_table_attr *table_attr;
> @@ -432,6 +503,10 @@ static int acpi_tables_sysfs_init(void)
>  	if (!tables_kobj)
>  		goto err;
>  
> +	tables_data_kobj = kobject_create_and_add("data", tables_kobj);
> +	if (!tables_data_kobj)
> +		goto err_tables_data;
> +
>  	dynamic_tables_kobj = kobject_create_and_add("dynamic", tables_kobj);
>  	if (!dynamic_tables_kobj)
>  		goto err_dynamic_tables;
> @@ -456,13 +531,17 @@ static int acpi_tables_sysfs_init(void)
>  			return ret;
>  		}
>  		list_add_tail(&table_attr->node, &acpi_table_attr_list);
> +		acpi_table_data_init(table_header);
>  	}
>  
>  	kobject_uevent(tables_kobj, KOBJ_ADD);
> +	kobject_uevent(tables_data_kobj, KOBJ_ADD);
>  	kobject_uevent(dynamic_tables_kobj, KOBJ_ADD);
>  
>  	return 0;
>  err_dynamic_tables:
> +	kobject_put(tables_data_kobj);
> +err_tables_data:
>  	kobject_put(tables_kobj);
>  err:
>  	return -ENOMEM;
> 

Applied, thanks!

Re: [PATCH] ACPI / sysfs: Extend ACPI sysfs to provide access to boot error region

[Kees Cook]

On Wed, Aug 23, 2017 at 7:56 AM, Luck, Tony <tony.luck@intel.com> wrote:
>>> Should this not also have a capability check. Assuming file permissions
>>> are sufficient for grabbing a chunk of system memory holding error
>>> info doesn't seem too scary but it's at odds with a lot of other cases ?
>>
>> At least one of those other cases (pstore) added a capability check and now regret
>> it.  There's a thread on reverting it. Look for:
>>
>>   Revert "pstore: Honor dmesg_restrict sysctl on dmesg dumps"
>
> Here's at least part of that thread:
>
> https://marc.info/?l=linux-kernel&m=150301241114262&w=2
>
> Kees: you were OK with removing the capability check from pstore, right?

Yeah, as long as there is comparable protections.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH] ACPI / sysfs: Extend ACPI sysfs to provide access to boot error region

[Luck, Tony]

On Mon, Aug 28, 2017 at 05:10:33PM -0700, Kees Cook wrote:
> On Wed, Aug 23, 2017 at 7:56 AM, Luck, Tony <tony.luck@intel.com> wrote:
> >>> Should this not also have a capability check. Assuming file permissions
> >>> are sufficient for grabbing a chunk of system memory holding error
> >>> info doesn't seem too scary but it's at odds with a lot of other cases ?
> >>
> >> At least one of those other cases (pstore) added a capability check and now regret
> >> it.  There's a thread on reverting it. Look for:
> >>
> >>   Revert "pstore: Honor dmesg_restrict sysctl on dmesg dumps"
> >
> > Here's at least part of that thread:
> >
> > https://marc.info/?l=linux-kernel&m=150301241114262&w=2
> >
> > Kees: you were OK with removing the capability check from pstore, right?
> 
> Yeah, as long as there is comparable protections.

File system permission protection is "0400":

	# ls -al /sys/firmware/acpi/tables/data
	total 0
	drwxr-xr-x. 2 root root     0 Aug 28 14:13 .
	drwxr-xr-x. 4 root root     0 Aug 28 14:10 ..
	-r--------. 1 root root 32768 Aug 28 14:13 BERT

-Tony