| Message ID | 20170817213946.16939-1-tony.luck@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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 -- 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
> 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 -- 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
"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 -- 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
>> 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 -- 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
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
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 -- 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
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;