From 7918cfc46cfad784b2aafdbbc690a96af0ae78d0 Mon Sep 17 00:00:00 2001 From: Julius Werner Date: Tue, 2 May 2017 15:16:29 -0700 Subject: firmware: google: memconsole: Make memconsole interface more flexible This patch redesigns the interface between the generic memconsole driver and its implementations to become more flexible than a flat memory buffer with unchanging bounds. This allows memconsoles like coreboot's to include lines that were added by runtime firmware after the driver was initialized. Since the console log size is thus no longer static, this means that the /sys/firmware/log file has to become unseekable. Signed-off-by: Julius Werner Signed-off-by: Greg Kroah-Hartman --- drivers/firmware/google/memconsole-coreboot.c | 12 +++++++++--- drivers/firmware/google/memconsole-x86-legacy.c | 18 +++++++++++++++--- drivers/firmware/google/memconsole.c | 14 ++++++-------- drivers/firmware/google/memconsole.h | 7 ++++--- 4 files changed, 34 insertions(+), 17 deletions(-) (limited to 'drivers/firmware') diff --git a/drivers/firmware/google/memconsole-coreboot.c b/drivers/firmware/google/memconsole-coreboot.c index 02711114dece..d48a80c3042d 100644 --- a/drivers/firmware/google/memconsole-coreboot.c +++ b/drivers/firmware/google/memconsole-coreboot.c @@ -33,6 +33,14 @@ struct cbmem_cons { static struct cbmem_cons __iomem *cbmem_console; +static ssize_t memconsole_coreboot_read(char *buf, loff_t pos, size_t count) +{ + return memory_read_from_buffer(buf, count, &pos, + cbmem_console->buffer_body, + min(cbmem_console->buffer_cursor, + cbmem_console->buffer_size)); +} + static int memconsole_coreboot_init(phys_addr_t physaddr) { struct cbmem_cons __iomem *tmp_cbmc; @@ -50,9 +58,7 @@ static int memconsole_coreboot_init(phys_addr_t physaddr) if (!cbmem_console) return -ENOMEM; - memconsole_setup(cbmem_console->buffer_body, - min(cbmem_console->buffer_cursor, cbmem_console->buffer_size)); - + memconsole_setup(memconsole_coreboot_read); return 0; } diff --git a/drivers/firmware/google/memconsole-x86-legacy.c b/drivers/firmware/google/memconsole-x86-legacy.c index 1f279ee883b9..8c1bf6dbdaa6 100644 --- a/drivers/firmware/google/memconsole-x86-legacy.c +++ b/drivers/firmware/google/memconsole-x86-legacy.c @@ -48,6 +48,15 @@ struct biosmemcon_ebda { }; } __packed; +static char *memconsole_baseaddr; +static size_t memconsole_length; + +static ssize_t memconsole_read(char *buf, loff_t pos, size_t count) +{ + return memory_read_from_buffer(buf, count, &pos, memconsole_baseaddr, + memconsole_length); +} + static void found_v1_header(struct biosmemcon_ebda *hdr) { pr_info("memconsole: BIOS console v1 EBDA structure found at %p\n", @@ -56,7 +65,9 @@ static void found_v1_header(struct biosmemcon_ebda *hdr) hdr->v1.buffer_addr, hdr->v1.start, hdr->v1.end, hdr->v1.num_chars); - memconsole_setup(phys_to_virt(hdr->v1.buffer_addr), hdr->v1.num_chars); + memconsole_baseaddr = phys_to_virt(hdr->v1.buffer_addr); + memconsole_length = hdr->v1.num_chars; + memconsole_setup(memconsole_read); } static void found_v2_header(struct biosmemcon_ebda *hdr) @@ -67,8 +78,9 @@ static void found_v2_header(struct biosmemcon_ebda *hdr) hdr->v2.buffer_addr, hdr->v2.start, hdr->v2.end, hdr->v2.num_bytes); - memconsole_setup(phys_to_virt(hdr->v2.buffer_addr + hdr->v2.start), - hdr->v2.end - hdr->v2.start); + memconsole_baseaddr = phys_to_virt(hdr->v2.buffer_addr + hdr->v2.start); + memconsole_length = hdr->v2.end - hdr->v2.start; + memconsole_setup(memconsole_read); } /* diff --git a/drivers/firmware/google/memconsole.c b/drivers/firmware/google/memconsole.c index 94e200ddb4fa..166f07c68c02 100644 --- a/drivers/firmware/google/memconsole.c +++ b/drivers/firmware/google/memconsole.c @@ -22,15 +22,15 @@ #include "memconsole.h" -static char *memconsole_baseaddr; -static size_t memconsole_length; +static ssize_t (*memconsole_read_func)(char *, loff_t, size_t); static ssize_t memconsole_read(struct file *filp, struct kobject *kobp, struct bin_attribute *bin_attr, char *buf, loff_t pos, size_t count) { - return memory_read_from_buffer(buf, count, &pos, memconsole_baseaddr, - memconsole_length); + if (WARN_ON_ONCE(!memconsole_read_func)) + return -EIO; + return memconsole_read_func(buf, pos, count); } static struct bin_attribute memconsole_bin_attr = { @@ -38,16 +38,14 @@ static struct bin_attribute memconsole_bin_attr = { .read = memconsole_read, }; -void memconsole_setup(void *baseaddr, size_t length) +void memconsole_setup(ssize_t (*read_func)(char *, loff_t, size_t)) { - memconsole_baseaddr = baseaddr; - memconsole_length = length; + memconsole_read_func = read_func; } EXPORT_SYMBOL(memconsole_setup); int memconsole_sysfs_init(void) { - memconsole_bin_attr.size = memconsole_length; return sysfs_create_bin_file(firmware_kobj, &memconsole_bin_attr); } EXPORT_SYMBOL(memconsole_sysfs_init); diff --git a/drivers/firmware/google/memconsole.h b/drivers/firmware/google/memconsole.h index 190fc03a51ae..ff1592dc7d1a 100644 --- a/drivers/firmware/google/memconsole.h +++ b/drivers/firmware/google/memconsole.h @@ -18,13 +18,14 @@ #ifndef __FIRMWARE_GOOGLE_MEMCONSOLE_H #define __FIRMWARE_GOOGLE_MEMCONSOLE_H +#include + /* * memconsole_setup * - * Initialize the memory console from raw (virtual) base - * address and length. + * Initialize the memory console, passing the function to handle read accesses. */ -void memconsole_setup(void *baseaddr, size_t length); +void memconsole_setup(ssize_t (*read_func)(char *, loff_t, size_t)); /* * memconsole_sysfs_init -- cgit v1.2.3 From a5061d028594a31dbf70f4554e0b7d83e5ce770f Mon Sep 17 00:00:00 2001 From: Julius Werner Date: Tue, 2 May 2017 15:16:30 -0700 Subject: firmware: google: memconsole: Adapt to new coreboot ring buffer format The upstream coreboot implementation of memconsole was enhanced from a single-boot console to a persistent ring buffer (https://review.coreboot.org/#/c/18301). This patch changes the kernel memconsole driver to be able to read the new format in all cases. Signed-off-by: Julius Werner Signed-off-by: Greg Kroah-Hartman --- drivers/firmware/google/memconsole-coreboot.c | 47 ++++++++++++++++++++++----- 1 file changed, 39 insertions(+), 8 deletions(-) (limited to 'drivers/firmware') diff --git a/drivers/firmware/google/memconsole-coreboot.c b/drivers/firmware/google/memconsole-coreboot.c index d48a80c3042d..7d39f4ef5d9e 100644 --- a/drivers/firmware/google/memconsole-coreboot.c +++ b/drivers/firmware/google/memconsole-coreboot.c @@ -26,19 +26,50 @@ /* CBMEM firmware console log descriptor. */ struct cbmem_cons { - u32 buffer_size; - u32 buffer_cursor; - u8 buffer_body[0]; + u32 size; + u32 cursor; + u8 body[0]; } __packed; +#define CURSOR_MASK ((1 << 28) - 1) +#define OVERFLOW (1 << 31) + static struct cbmem_cons __iomem *cbmem_console; +/* + * The cbmem_console structure is read again on every access because it may + * change at any time if runtime firmware logs new messages. This may rarely + * lead to race conditions where the firmware overwrites the beginning of the + * ring buffer with more lines after we have already read |cursor|. It should be + * rare and harmless enough that we don't spend extra effort working around it. + */ static ssize_t memconsole_coreboot_read(char *buf, loff_t pos, size_t count) { - return memory_read_from_buffer(buf, count, &pos, - cbmem_console->buffer_body, - min(cbmem_console->buffer_cursor, - cbmem_console->buffer_size)); + u32 cursor = cbmem_console->cursor & CURSOR_MASK; + u32 flags = cbmem_console->cursor & ~CURSOR_MASK; + u32 size = cbmem_console->size; + struct seg { /* describes ring buffer segments in logical order */ + u32 phys; /* physical offset from start of mem buffer */ + u32 len; /* length of segment */ + } seg[2] = { {0}, {0} }; + size_t done = 0; + int i; + + if (flags & OVERFLOW) { + if (cursor > size) /* Shouldn't really happen, but... */ + cursor = 0; + seg[0] = (struct seg){.phys = cursor, .len = size - cursor}; + seg[1] = (struct seg){.phys = 0, .len = cursor}; + } else { + seg[0] = (struct seg){.phys = 0, .len = min(cursor, size)}; + } + + for (i = 0; i < ARRAY_SIZE(seg) && count > done; i++) { + done += memory_read_from_buffer(buf + done, count - done, &pos, + cbmem_console->body + seg[i].phys, seg[i].len); + pos -= seg[i].len; + } + return done; } static int memconsole_coreboot_init(phys_addr_t physaddr) @@ -51,7 +82,7 @@ static int memconsole_coreboot_init(phys_addr_t physaddr) return -ENOMEM; cbmem_console = memremap(physaddr, - tmp_cbmc->buffer_size + sizeof(*cbmem_console), + tmp_cbmc->size + sizeof(*cbmem_console), MEMREMAP_WB); memunmap(tmp_cbmc); -- cgit v1.2.3 From 40fbb23881291bb57e4e25e859de8e2287426dac Mon Sep 17 00:00:00 2001 From: Julius Werner Date: Tue, 23 May 2017 16:48:17 -0700 Subject: firmware: google: memconsole: Prevent overrun attack on coreboot console The recent coreboot memory console update (firmware: google: memconsole: Adapt to new coreboot ring buffer format) introduced a small security issue in the driver: The new driver implementation parses the memory console structure again on every access. This is intentional so that additional lines added concurrently by runtime firmware can be read out. However, if an attacker can write to the structure, they could increase the size value to a point where the driver would read potentially sensitive memory areas from outside the original console buffer during the next access. This can be done through /dev/mem, since the console buffer usually resides in firmware-reserved memory that is not covered by STRICT_DEVMEM. This patch resolves that problem by reading the buffer's size value only once during boot (where we can still trust the structure). Other parts of the structure can still be modified at runtime, but the driver's bounds checks make sure that it will never read outside the buffer. Fixes: a5061d028 ("firmware: google: memconsole: Adapt to new coreboot ring buffer format") Signed-off-by: Julius Werner Signed-off-by: Greg Kroah-Hartman --- drivers/firmware/google/memconsole-coreboot.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) (limited to 'drivers/firmware') diff --git a/drivers/firmware/google/memconsole-coreboot.c b/drivers/firmware/google/memconsole-coreboot.c index 7d39f4ef5d9e..52738887735c 100644 --- a/drivers/firmware/google/memconsole-coreboot.c +++ b/drivers/firmware/google/memconsole-coreboot.c @@ -26,7 +26,7 @@ /* CBMEM firmware console log descriptor. */ struct cbmem_cons { - u32 size; + u32 size_dont_access_after_boot; u32 cursor; u8 body[0]; } __packed; @@ -35,6 +35,7 @@ struct cbmem_cons { #define OVERFLOW (1 << 31) static struct cbmem_cons __iomem *cbmem_console; +static u32 cbmem_console_size; /* * The cbmem_console structure is read again on every access because it may @@ -47,7 +48,7 @@ static ssize_t memconsole_coreboot_read(char *buf, loff_t pos, size_t count) { u32 cursor = cbmem_console->cursor & CURSOR_MASK; u32 flags = cbmem_console->cursor & ~CURSOR_MASK; - u32 size = cbmem_console->size; + u32 size = cbmem_console_size; struct seg { /* describes ring buffer segments in logical order */ u32 phys; /* physical offset from start of mem buffer */ u32 len; /* length of segment */ @@ -81,8 +82,10 @@ static int memconsole_coreboot_init(phys_addr_t physaddr) if (!tmp_cbmc) return -ENOMEM; + /* Read size only once to prevent overrun attack through /dev/mem. */ + cbmem_console_size = tmp_cbmc->size_dont_access_after_boot; cbmem_console = memremap(physaddr, - tmp_cbmc->size + sizeof(*cbmem_console), + cbmem_console_size + sizeof(*cbmem_console), MEMREMAP_WB); memunmap(tmp_cbmc); -- cgit v1.2.3 From 3eec6a1c0cf31b982cad560c16e19fdb1851ae91 Mon Sep 17 00:00:00 2001 From: Dmitry Torokhov Date: Tue, 23 May 2017 17:07:42 -0700 Subject: firmware: vpd: use kdtrndup when copying section key Instead of open-coding kstrndup with kzalloc + memcpy, let's use the helper. Signed-off-by: Dmitry Torokhov Reviewed-by: Guenter Roeck Signed-off-by: Greg Kroah-Hartman --- drivers/firmware/google/vpd.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'drivers/firmware') diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c index 1e7860f02f4f..8bd51eaededd 100644 --- a/drivers/firmware/google/vpd.c +++ b/drivers/firmware/google/vpd.c @@ -118,14 +118,13 @@ static int vpd_section_attrib_add(const u8 *key, s32 key_len, info = kzalloc(sizeof(*info), GFP_KERNEL); if (!info) return -ENOMEM; - info->key = kzalloc(key_len + 1, GFP_KERNEL); + + info->key = kstrndup(key, key_len, GFP_KERNEL); if (!info->key) { ret = -ENOMEM; goto free_info; } - memcpy(info->key, key, key_len); - sysfs_bin_attr_init(&info->bin_attr); info->bin_attr.attr.name = info->key; info->bin_attr.attr.mode = 0444; -- cgit v1.2.3 From 9920a33e3573a207cd49895080e2806134e5e56f Mon Sep 17 00:00:00 2001 From: Dmitry Torokhov Date: Tue, 23 May 2017 17:07:45 -0700 Subject: firmware: vpd: use kasprintf() when forming name of 'raw' attribute When creating name for the "raw" attribute, let's switch to using kaspeintf() instead of doing it by hand. Also make sure we handle errors. Signed-off-by: Dmitry Torokhov Reviewed-by: Guenter Roeck Signed-off-by: Greg Kroah-Hartman --- drivers/firmware/google/vpd.c | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) (limited to 'drivers/firmware') diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c index 8bd51eaededd..66fd0230605e 100644 --- a/drivers/firmware/google/vpd.c +++ b/drivers/firmware/google/vpd.c @@ -190,8 +190,7 @@ static int vpd_section_create_attribs(struct vpd_section *sec) static int vpd_section_init(const char *name, struct vpd_section *sec, phys_addr_t physaddr, size_t size) { - int ret; - int raw_len; + int err; sec->baseaddr = memremap(physaddr, size, MEMREMAP_WB); if (!sec->baseaddr) @@ -200,10 +199,11 @@ static int vpd_section_init(const char *name, struct vpd_section *sec, sec->name = name; /* We want to export the raw partion with name ${name}_raw */ - raw_len = strlen(name) + 5; - sec->raw_name = kzalloc(raw_len, GFP_KERNEL); - strncpy(sec->raw_name, name, raw_len); - strncat(sec->raw_name, "_raw", raw_len); + sec->raw_name = kasprintf(GFP_KERNEL, "%s_raw", name); + if (!sec->raw_name) { + err = -ENOMEM; + goto err_iounmap; + } sysfs_bin_attr_init(&sec->bin_attr); sec->bin_attr.attr.name = sec->raw_name; @@ -212,14 +212,14 @@ static int vpd_section_init(const char *name, struct vpd_section *sec, sec->bin_attr.read = vpd_section_read; sec->bin_attr.private = sec; - ret = sysfs_create_bin_file(vpd_kobj, &sec->bin_attr); - if (ret) - goto free_sec; + err = sysfs_create_bin_file(vpd_kobj, &sec->bin_attr); + if (err) + goto err_free_raw_name; sec->kobj = kobject_create_and_add(name, vpd_kobj); if (!sec->kobj) { - ret = -EINVAL; - goto sysfs_remove; + err = -EINVAL; + goto err_sysfs_remove; } INIT_LIST_HEAD(&sec->attribs); @@ -229,14 +229,13 @@ static int vpd_section_init(const char *name, struct vpd_section *sec, return 0; -sysfs_remove: +err_sysfs_remove: sysfs_remove_bin_file(vpd_kobj, &sec->bin_attr); - -free_sec: +err_free_raw_name: kfree(sec->raw_name); +err_iounmap: iounmap(sec->baseaddr); - - return ret; + return err; } static int vpd_section_destroy(struct vpd_section *sec) -- cgit v1.2.3 From dd246486f94694edb86e898a24a61ebf2f2fdba4 Mon Sep 17 00:00:00 2001 From: Dmitry Torokhov Date: Tue, 23 May 2017 17:07:46 -0700 Subject: firmware: vpd: do not clear statically allocated data ro_vpd and rw_vpd are static module-scope variables that are guaranteed to be initialized with zeroes, there is no need for explicit memset(). Signed-off-by: Dmitry Torokhov Reviewed-by: Guenter Roeck Signed-off-by: Greg Kroah-Hartman --- drivers/firmware/google/vpd.c | 3 --- 1 file changed, 3 deletions(-) (limited to 'drivers/firmware') diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c index 66fd0230605e..4f8f99edbbfa 100644 --- a/drivers/firmware/google/vpd.c +++ b/drivers/firmware/google/vpd.c @@ -317,9 +317,6 @@ static int __init vpd_platform_init(void) if (!vpd_kobj) return -ENOMEM; - memset(&ro_vpd, 0, sizeof(ro_vpd)); - memset(&rw_vpd, 0, sizeof(rw_vpd)); - platform_driver_register(&vpd_driver); return 0; -- cgit v1.2.3 From 7975bd4cca05a99aa14964cfa22366ee64da50ad Mon Sep 17 00:00:00 2001 From: Dmitry Torokhov Date: Tue, 23 May 2017 17:07:47 -0700 Subject: firmware: vpd: remove platform driver There is no reason why VPD should register platform device and driver, given that we do not use their respective kobjects to attach attributes, nor do we need suspend/resume hooks, or any other features of device core. Signed-off-by: Dmitry Torokhov Reviewed-by: Guenter Roeck Signed-off-by: Greg Kroah-Hartman --- drivers/firmware/google/vpd.c | 44 ++++++++++++++++--------------------------- 1 file changed, 16 insertions(+), 28 deletions(-) (limited to 'drivers/firmware') diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c index 4f8f99edbbfa..d28f62fed50f 100644 --- a/drivers/firmware/google/vpd.c +++ b/drivers/firmware/google/vpd.c @@ -22,8 +22,6 @@ #include #include #include -#include -#include #include #include @@ -279,47 +277,37 @@ static int vpd_sections_init(phys_addr_t physaddr) ret = vpd_section_init("rw", &rw_vpd, physaddr + sizeof(struct vpd_cbmem) + header.ro_size, header.rw_size); - if (ret) + if (ret) { + vpd_section_destroy(&ro_vpd); return ret; + } } return 0; } -static int vpd_probe(struct platform_device *pdev) -{ - int ret; - struct lb_cbmem_ref entry; - - ret = coreboot_table_find(CB_TAG_VPD, &entry, sizeof(entry)); - if (ret) - return ret; - - return vpd_sections_init(entry.cbmem_addr); -} - -static struct platform_driver vpd_driver = { - .probe = vpd_probe, - .driver = { - .name = "vpd", - }, -}; - static int __init vpd_platform_init(void) { - struct platform_device *pdev; - - pdev = platform_device_register_simple("vpd", -1, NULL, 0); - if (IS_ERR(pdev)) - return PTR_ERR(pdev); + struct lb_cbmem_ref entry; + int err; vpd_kobj = kobject_create_and_add("vpd", firmware_kobj); if (!vpd_kobj) return -ENOMEM; - platform_driver_register(&vpd_driver); + err = coreboot_table_find(CB_TAG_VPD, &entry, sizeof(entry)); + if (err) + goto err_kobject_put; + + err = vpd_sections_init(entry.cbmem_addr); + if (err) + goto err_kobject_put; return 0; + +err_kobject_put: + kobject_put(vpd_kobj); + return err; } static void __exit vpd_platform_exit(void) -- cgit v1.2.3 From 46505c802a55189955d97195c8567ee263168747 Mon Sep 17 00:00:00 2001 From: Dmitry Torokhov Date: Fri, 26 May 2017 13:57:49 -0700 Subject: Revert "firmware: vpd: remove platform driver" This reverts commit 7975bd4cca05a99aa14964cfa22366ee64da50ad, because VPD relies on driver core to handle deferrals returned by coreboot_table_find(). Signed-off-by: Dmitry Torokhov Reviewed-by: Guenter Roeck Signed-off-by: Greg Kroah-Hartman --- drivers/firmware/google/vpd.c | 44 +++++++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 16 deletions(-) (limited to 'drivers/firmware') diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c index d28f62fed50f..4f8f99edbbfa 100644 --- a/drivers/firmware/google/vpd.c +++ b/drivers/firmware/google/vpd.c @@ -22,6 +22,8 @@ #include #include #include +#include +#include #include #include @@ -277,37 +279,47 @@ static int vpd_sections_init(phys_addr_t physaddr) ret = vpd_section_init("rw", &rw_vpd, physaddr + sizeof(struct vpd_cbmem) + header.ro_size, header.rw_size); - if (ret) { - vpd_section_destroy(&ro_vpd); + if (ret) return ret; - } } return 0; } -static int __init vpd_platform_init(void) +static int vpd_probe(struct platform_device *pdev) { + int ret; struct lb_cbmem_ref entry; - int err; + + ret = coreboot_table_find(CB_TAG_VPD, &entry, sizeof(entry)); + if (ret) + return ret; + + return vpd_sections_init(entry.cbmem_addr); +} + +static struct platform_driver vpd_driver = { + .probe = vpd_probe, + .driver = { + .name = "vpd", + }, +}; + +static int __init vpd_platform_init(void) +{ + struct platform_device *pdev; + + pdev = platform_device_register_simple("vpd", -1, NULL, 0); + if (IS_ERR(pdev)) + return PTR_ERR(pdev); vpd_kobj = kobject_create_and_add("vpd", firmware_kobj); if (!vpd_kobj) return -ENOMEM; - err = coreboot_table_find(CB_TAG_VPD, &entry, sizeof(entry)); - if (err) - goto err_kobject_put; - - err = vpd_sections_init(entry.cbmem_addr); - if (err) - goto err_kobject_put; + platform_driver_register(&vpd_driver); return 0; - -err_kobject_put: - kobject_put(vpd_kobj); - return err; } static void __exit vpd_platform_exit(void) -- cgit v1.2.3