From 4a16d1cb245c56e72fd40a28f3cdb394cde4b341 Mon Sep 17 00:00:00 2001 From: Ankit Kumar Date: Tue, 23 May 2017 11:16:52 +0530 Subject: [PATCH 1/9] pstore: Don't warn if data is uncompressed and type is not PSTORE_TYPE_DMESG commit 9abdcccc3d5f ("pstore: Extract common arguments into structure") moved record decompression to function. decompress_record() gets called without checking type and compressed flag. Warning will be reported if data is uncompressed. Pstore type PSTORE_TYPE_PPC_OPAL, PSTORE_TYPE_PPC_COMMON doesn't contain compressed data and warning get printed part of dmesg. Partial dmesg log: [ 35.848914] pstore: ignored compressed record type 6 [ 35.848927] pstore: ignored compressed record type 8 Above warning should not get printed as it is known that data won't be compressed for above type and it is valid condition. This patch returns if data is not compressed and print warning only if data is compressed and type is not PSTORE_TYPE_DMESG. Reported-by: Anton Blanchard Signed-off-by: Ankit Kumar Reviewed-by: Mahesh Salgaonkar Signed-off-by: Kees Cook Fixes: 9abdcccc3d5f ("pstore: Extract common arguments into structure") Cc: stable@vger.kernel.org --- fs/pstore/platform.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c index d468eec9b8a6..b3045530dd4e 100644 --- a/fs/pstore/platform.c +++ b/fs/pstore/platform.c @@ -770,8 +770,11 @@ static void decompress_record(struct pstore_record *record) int unzipped_len; char *decompressed; + if (!record->compressed) + return; + /* Only PSTORE_TYPE_DMESG support compression. */ - if (!record->compressed || record->type != PSTORE_TYPE_DMESG) { + if (record->type != PSTORE_TYPE_DMESG) { pr_warn("ignored compressed record type %d\n", record->type); return; } From f6525b96dd9f68efe374e5aef864975e628de991 Mon Sep 17 00:00:00 2001 From: Douglas Anderson Date: Tue, 30 May 2017 15:50:38 -0700 Subject: [PATCH 2/9] pstore: Fix leaked pstore_record in pstore_get_backend_records() When the "if (record->size <= 0)" test is true in pstore_get_backend_records() it's pretty clear that nobody holds a reference to the allocated pstore_record, yet we don't free it. Let's free it. Fixes: 2a2b0acf768c ("pstore: Allocate records on heap instead of stack") Signed-off-by: Douglas Anderson Signed-off-by: Kees Cook Cc: stable@vger.kernel.org --- fs/pstore/platform.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c index b3045530dd4e..4c5cd9368460 100644 --- a/fs/pstore/platform.c +++ b/fs/pstore/platform.c @@ -849,8 +849,10 @@ void pstore_get_backend_records(struct pstore_info *psi, record->size = psi->read(record); /* No more records left in backend? */ - if (record->size <= 0) + if (record->size <= 0) { + kfree(record); break; + } decompress_record(record); rc = pstore_mkfile(root, record); From 656de42e83379e5348e3f4236ff1d79353edfb28 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Tue, 16 May 2017 12:03:31 -0700 Subject: [PATCH 3/9] pstore: Avoid potential infinite loop If a backend does not correctly iterate through its records, pstore will get stuck loading entries. Detect this with a large record count, and announce if we ever hit the limit. This will let future backend reading bugs less annoying to debug. Additionally adjust the error about pstore_mkfile() failing. Signed-off-by: Kees Cook --- fs/pstore/platform.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c index 4c5cd9368460..d8289ce00f99 100644 --- a/fs/pstore/platform.c +++ b/fs/pstore/platform.c @@ -822,6 +822,7 @@ void pstore_get_backend_records(struct pstore_info *psi, struct dentry *root, int quiet) { int failed = 0; + unsigned int stop_loop = 65536; if (!psi || !root) return; @@ -835,7 +836,7 @@ void pstore_get_backend_records(struct pstore_info *psi, * may reallocate record.buf. On success, pstore_mkfile() will keep * the record.buf, so free it only on failure. */ - for (;;) { + for (; stop_loop; stop_loop--) { struct pstore_record *record; int rc; @@ -870,8 +871,11 @@ out: mutex_unlock(&psi->read_mutex); if (failed) - pr_warn("failed to load %d record(s) from '%s'\n", + pr_warn("failed to create %d record(s) from '%s'\n", failed, psi->name); + if (!stop_loop) + pr_err("looping? Too many records seen from '%s'\n", + psi->name); } static void pstore_dowork(struct work_struct *work) From efb74e4bb1abeb5bd4c9296b2adfdc253561bc72 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 19 May 2017 13:21:07 -0700 Subject: [PATCH 4/9] efi-pstore: Refactor erase routine Right now, every pass through the EFI variables during erase would build a copy of the old format variable name. Instead, try each name one time through the EFI variables list. Additionally bump up the buffer size to avoid truncation in pathological cases, and wipe the write name buffer. Signed-off-by: Kees Cook --- drivers/firmware/efi/efi-pstore.c | 88 ++++++++++++++----------------- 1 file changed, 40 insertions(+), 48 deletions(-) diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c index ef1fafdad400..d30dc935e20e 100644 --- a/drivers/firmware/efi/efi-pstore.c +++ b/drivers/firmware/efi/efi-pstore.c @@ -4,7 +4,7 @@ #include #include -#define DUMP_NAME_LEN 52 +#define DUMP_NAME_LEN 66 static bool efivars_pstore_disable = IS_ENABLED(CONFIG_EFI_VARS_PSTORE_DEFAULT_DISABLE); @@ -250,6 +250,9 @@ static int efi_pstore_write(struct pstore_record *record) record->id = generic_id(record->time.tv_sec, record->part, record->count); + /* Since we copy the entire length of name, make sure it is wiped. */ + memset(name, 0, sizeof(name)); + snprintf(name, sizeof(name), "dump-type%u-%u-%d-%lu-%c", record->type, record->part, record->count, record->time.tv_sec, record->compressed ? 'C' : 'D'); @@ -267,44 +270,20 @@ static int efi_pstore_write(struct pstore_record *record) return ret; }; -struct pstore_erase_data { - struct pstore_record *record; - efi_char16_t *name; -}; - /* * Clean up an entry with the same name */ static int efi_pstore_erase_func(struct efivar_entry *entry, void *data) { - struct pstore_erase_data *ed = data; + efi_char16_t *efi_name = data; efi_guid_t vendor = LINUX_EFI_CRASH_GUID; - efi_char16_t efi_name_old[DUMP_NAME_LEN]; - efi_char16_t *efi_name = ed->name; - unsigned long ucs2_len = ucs2_strlen(ed->name); - char name_old[DUMP_NAME_LEN]; - int i; + unsigned long ucs2_len = ucs2_strlen(efi_name); if (efi_guidcmp(entry->var.VendorGuid, vendor)) return 0; - if (ucs2_strncmp(entry->var.VariableName, - efi_name, (size_t)ucs2_len)) { - /* - * Check if an old format, which doesn't support - * holding multiple logs, remains. - */ - snprintf(name_old, sizeof(name_old), "dump-type%u-%u-%lu", - ed->record->type, ed->record->part, - ed->record->time.tv_sec); - - for (i = 0; i < DUMP_NAME_LEN; i++) - efi_name_old[i] = name_old[i]; - - if (ucs2_strncmp(entry->var.VariableName, efi_name_old, - ucs2_strlen(efi_name_old))) - return 0; - } + if (ucs2_strncmp(entry->var.VariableName, efi_name, (size_t)ucs2_len)) + return 0; if (entry->scanning) { /* @@ -321,35 +300,48 @@ static int efi_pstore_erase_func(struct efivar_entry *entry, void *data) return 1; } -static int efi_pstore_erase(struct pstore_record *record) +static int efi_pstore_erase_name(const char *name) { - struct pstore_erase_data edata; struct efivar_entry *entry = NULL; - char name[DUMP_NAME_LEN]; efi_char16_t efi_name[DUMP_NAME_LEN]; int found, i; + for (i = 0; i < DUMP_NAME_LEN; i++) { + efi_name[i] = name[i]; + if (name[i] == '\0') + break; + } + + if (efivar_entry_iter_begin()) + return -EINTR; + + found = __efivar_entry_iter(efi_pstore_erase_func, &efivar_sysfs_list, + efi_name, &entry); + efivar_entry_iter_end(); + + if (found && !entry->scanning) + efivar_unregister(entry); + + return found ? 0 : -ENOENT; +} + +static int efi_pstore_erase(struct pstore_record *record) +{ + char name[DUMP_NAME_LEN]; + int ret; + snprintf(name, sizeof(name), "dump-type%u-%u-%d-%lu", record->type, record->part, record->count, record->time.tv_sec); + ret = efi_pstore_erase_name(name); + if (ret != -ENOENT) + return ret; - for (i = 0; i < DUMP_NAME_LEN; i++) - efi_name[i] = name[i]; + snprintf(name, sizeof(name), "dump-type%u-%u-%lu", + record->type, record->part, record->time.tv_sec); + ret = efi_pstore_erase_name(name); - edata.record = record; - edata.name = efi_name; - - if (efivar_entry_iter_begin()) - return -EINTR; - found = __efivar_entry_iter(efi_pstore_erase_func, &efivar_sysfs_list, &edata, &entry); - - if (found && !entry->scanning) { - efivar_entry_iter_end(); - efivar_unregister(entry); - } else - efivar_entry_iter_end(); - - return 0; + return ret; } static struct pstore_info efi_pstore_info = { From e581ca813a40a4ee53c862d8f6303f486c4b4c34 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 19 May 2017 15:10:31 -0700 Subject: [PATCH 5/9] pstore: Create common record initializer In preparation for setting timestamps in the pstore core, create a common initializer routine, instead of using static initializers. Signed-off-by: Kees Cook --- fs/pstore/internal.h | 2 ++ fs/pstore/platform.c | 35 +++++++++++++++++++++-------------- fs/pstore/pmsg.c | 10 +++++----- 3 files changed, 28 insertions(+), 19 deletions(-) diff --git a/fs/pstore/internal.h b/fs/pstore/internal.h index c416e653dc4f..58051265626f 100644 --- a/fs/pstore/internal.h +++ b/fs/pstore/internal.h @@ -30,5 +30,7 @@ extern void pstore_get_backend_records(struct pstore_info *psi, extern int pstore_mkfile(struct dentry *root, struct pstore_record *record); extern bool pstore_is_mounted(void); +extern void pstore_record_init(struct pstore_record *record, + struct pstore_info *psi); #endif diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c index d8289ce00f99..7798041f3fba 100644 --- a/fs/pstore/platform.c +++ b/fs/pstore/platform.c @@ -474,6 +474,14 @@ static size_t copy_kmsg_to_buffer(int hsize, size_t len) return total_len; } +void pstore_record_init(struct pstore_record *record, + struct pstore_info *psinfo) +{ + memset(record, 0, sizeof(*record)); + + record->psi = psinfo; +} + /* * callback from kmsg_dump. (s2,l2) has the most recently * written bytes, older bytes are in (s1,l1). Save as much @@ -509,15 +517,14 @@ static void pstore_dump(struct kmsg_dumper *dumper, int header_size; int zipped_len = -1; size_t dump_size; - struct pstore_record record = { - .type = PSTORE_TYPE_DMESG, - .count = oopscount, - .reason = reason, - .part = part, - .compressed = false, - .buf = psinfo->buf, - .psi = psinfo, - }; + struct pstore_record record; + + pstore_record_init(&record, psinfo); + record.type = PSTORE_TYPE_DMESG; + record.count = oopscount; + record.reason = reason; + record.part = part; + record.buf = psinfo->buf; if (big_oops_buf && is_locked) { dst = big_oops_buf; @@ -587,12 +594,12 @@ static void pstore_console_write(struct console *con, const char *s, unsigned c) const char *e = s + c; while (s < e) { - struct pstore_record record = { - .type = PSTORE_TYPE_CONSOLE, - .psi = psinfo, - }; + struct pstore_record record; unsigned long flags; + pstore_record_init(&record, psinfo); + record.type = PSTORE_TYPE_CONSOLE; + if (c > psinfo->bufsize) c = psinfo->bufsize; @@ -845,7 +852,7 @@ void pstore_get_backend_records(struct pstore_info *psi, pr_err("out of memory creating record\n"); break; } - record->psi = psi; + pstore_record_init(record, psi); record->size = psi->read(record); diff --git a/fs/pstore/pmsg.c b/fs/pstore/pmsg.c index 209755e0d7c8..24db02de1787 100644 --- a/fs/pstore/pmsg.c +++ b/fs/pstore/pmsg.c @@ -22,16 +22,16 @@ static DEFINE_MUTEX(pmsg_lock); static ssize_t write_pmsg(struct file *file, const char __user *buf, size_t count, loff_t *ppos) { - struct pstore_record record = { - .type = PSTORE_TYPE_PMSG, - .size = count, - .psi = psinfo, - }; + struct pstore_record record; int ret; if (!count) return 0; + pstore_record_init(&record, psinfo); + record.type = PSTORE_TYPE_PMSG; + record.size = count; + /* check outside lock, page in any data. write_user also checks */ if (!access_ok(VERIFY_READ, buf, count)) return -EFAULT; From c7f3c595f6ff7a1cfbf7ac782722bf5173e27775 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 19 May 2017 15:29:10 -0700 Subject: [PATCH 6/9] pstore: Populate pstore record->time field The current time will be initially available in the record->time field for all pstore_read() and pstore_write() calls. Backends can either update the field during read(), or use the field during write() instead of fetching time themselves. Signed-off-by: Kees Cook --- drivers/firmware/efi/efi-pstore.c | 3 --- fs/pstore/platform.c | 6 ++++++ fs/pstore/ram.c | 16 +++++----------- include/linux/pstore.h | 5 ++++- 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c index d30dc935e20e..5a0fa939d70f 100644 --- a/drivers/firmware/efi/efi-pstore.c +++ b/drivers/firmware/efi/efi-pstore.c @@ -244,9 +244,6 @@ static int efi_pstore_write(struct pstore_record *record) efi_guid_t vendor = LINUX_EFI_CRASH_GUID; int i, ret = 0; - record->time.tv_sec = get_seconds(); - record->time.tv_nsec = 0; - record->id = generic_id(record->time.tv_sec, record->part, record->count); diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c index 7798041f3fba..96fbff7b87c8 100644 --- a/fs/pstore/platform.c +++ b/fs/pstore/platform.c @@ -480,6 +480,12 @@ void pstore_record_init(struct pstore_record *record, memset(record, 0, sizeof(*record)); record->psi = psinfo; + + /* Report zeroed timestamp if called before timekeeping has resumed. */ + if (__getnstimeofday(&record->time)) { + record->time.tv_sec = 0; + record->time.tv_nsec = 0; + } } /* diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c index 5cb022c8cd33..7125b398d312 100644 --- a/fs/pstore/ram.c +++ b/fs/pstore/ram.c @@ -27,7 +27,6 @@ #include #include #include -#include #include #include #include @@ -356,20 +355,15 @@ out: } static size_t ramoops_write_kmsg_hdr(struct persistent_ram_zone *prz, - bool compressed) + struct pstore_record *record) { char *hdr; - struct timespec timestamp; size_t len; - /* Report zeroed timestamp if called before timekeeping has resumed. */ - if (__getnstimeofday(×tamp)) { - timestamp.tv_sec = 0; - timestamp.tv_nsec = 0; - } hdr = kasprintf(GFP_ATOMIC, RAMOOPS_KERNMSG_HDR "%lu.%lu-%c\n", - (long)timestamp.tv_sec, (long)(timestamp.tv_nsec / 1000), - compressed ? 'C' : 'D'); + record->time.tv_sec, + record->time.tv_nsec / 1000, + record->compressed ? 'C' : 'D'); WARN_ON_ONCE(!hdr); len = hdr ? strlen(hdr) : 0; persistent_ram_write(prz, hdr, len); @@ -440,7 +434,7 @@ static int notrace ramoops_pstore_write(struct pstore_record *record) prz = cxt->dprzs[cxt->dump_write_cnt]; /* Build header and append record contents. */ - hlen = ramoops_write_kmsg_hdr(prz, record->compressed); + hlen = ramoops_write_kmsg_hdr(prz, record); size = record->size; if (size + hlen > prz->buffer_size) size = prz->buffer_size - hlen; diff --git a/include/linux/pstore.h b/include/linux/pstore.h index e2233f50f428..61f806a7fe29 100644 --- a/include/linux/pstore.h +++ b/include/linux/pstore.h @@ -138,7 +138,10 @@ struct pstore_record { * memory allocation may be broken during an Oops. Regardless, * @buf must be proccesed or copied before returning. The * backend is also expected to write @id with something that - 8 can help identify this record to a future @erase callback. + * can help identify this record to a future @erase callback. + * The @time field will be prepopulated with the current time, + * when available. The @size field will have the size of data + * in @buf. * * Returns 0 on success, and non-zero on error. * From d3762358a739e7ef63873d6923938def99affa93 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Mon, 22 May 2017 16:49:52 -0700 Subject: [PATCH 7/9] pstore: Fix format string to use %u for record id The format string for record->id (u64) was using %lld instead of %llu. Signed-off-by: Kees Cook --- fs/pstore/inode.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c index 792a4e5f9226..4d02c3b65061 100644 --- a/fs/pstore/inode.c +++ b/fs/pstore/inode.c @@ -349,48 +349,48 @@ int pstore_mkfile(struct dentry *root, struct pstore_record *record) switch (record->type) { case PSTORE_TYPE_DMESG: - scnprintf(name, sizeof(name), "dmesg-%s-%lld%s", + scnprintf(name, sizeof(name), "dmesg-%s-%llu%s", record->psi->name, record->id, record->compressed ? ".enc.z" : ""); break; case PSTORE_TYPE_CONSOLE: - scnprintf(name, sizeof(name), "console-%s-%lld", + scnprintf(name, sizeof(name), "console-%s-%llu", record->psi->name, record->id); break; case PSTORE_TYPE_FTRACE: - scnprintf(name, sizeof(name), "ftrace-%s-%lld", + scnprintf(name, sizeof(name), "ftrace-%s-%llu", record->psi->name, record->id); break; case PSTORE_TYPE_MCE: - scnprintf(name, sizeof(name), "mce-%s-%lld", + scnprintf(name, sizeof(name), "mce-%s-%llu", record->psi->name, record->id); break; case PSTORE_TYPE_PPC_RTAS: - scnprintf(name, sizeof(name), "rtas-%s-%lld", + scnprintf(name, sizeof(name), "rtas-%s-%llu", record->psi->name, record->id); break; case PSTORE_TYPE_PPC_OF: - scnprintf(name, sizeof(name), "powerpc-ofw-%s-%lld", + scnprintf(name, sizeof(name), "powerpc-ofw-%s-%llu", record->psi->name, record->id); break; case PSTORE_TYPE_PPC_COMMON: - scnprintf(name, sizeof(name), "powerpc-common-%s-%lld", + scnprintf(name, sizeof(name), "powerpc-common-%s-%llu", record->psi->name, record->id); break; case PSTORE_TYPE_PMSG: - scnprintf(name, sizeof(name), "pmsg-%s-%lld", + scnprintf(name, sizeof(name), "pmsg-%s-%llu", record->psi->name, record->id); break; case PSTORE_TYPE_PPC_OPAL: - scnprintf(name, sizeof(name), "powerpc-opal-%s-%lld", + scnprintf(name, sizeof(name), "powerpc-opal-%s-%llu", record->psi->name, record->id); break; case PSTORE_TYPE_UNKNOWN: - scnprintf(name, sizeof(name), "unknown-%s-%lld", + scnprintf(name, sizeof(name), "unknown-%s-%llu", record->psi->name, record->id); break; default: - scnprintf(name, sizeof(name), "type%d-%s-%lld", + scnprintf(name, sizeof(name), "type%d-%s-%llu", record->type, record->psi->name, record->id); break; } From 077090af337f7d833f82be3d556549512398de66 Mon Sep 17 00:00:00 2001 From: Geliang Tang Date: Sat, 29 Apr 2017 09:45:16 +0800 Subject: [PATCH 8/9] pstore: use memdup_user Use memdup_user() helper instead of open-coding to simplify the code. Signed-off-by: Geliang Tang Signed-off-by: Kees Cook --- fs/pstore/platform.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c index 96fbff7b87c8..1b6e0ff6bff5 100644 --- a/fs/pstore/platform.c +++ b/fs/pstore/platform.c @@ -653,19 +653,16 @@ static int pstore_write_user_compat(struct pstore_record *record, if (record->buf) return -EINVAL; - record->buf = kmalloc(record->size, GFP_KERNEL); - if (!record->buf) - return -ENOMEM; - - if (unlikely(copy_from_user(record->buf, buf, record->size))) { - ret = -EFAULT; + record->buf = memdup_user(buf, record->size); + if (unlikely(IS_ERR(record->buf))) { + ret = PTR_ERR(record->buf); goto out; } ret = record->psi->write(record); -out: kfree(record->buf); +out: record->buf = NULL; return unlikely(ret < 0) ? ret : record->size; From 0752e4028c003fba1e2b44c4b3cf6a4482e931b6 Mon Sep 17 00:00:00 2001 From: Geliang Tang Date: Sat, 29 Apr 2017 09:45:13 +0800 Subject: [PATCH 9/9] powerpc/nvram: use memdup_user Use memdup_user() helper instead of open-coding to simplify the code. Signed-off-by: Geliang Tang Signed-off-by: Kees Cook --- arch/powerpc/kernel/nvram_64.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/arch/powerpc/kernel/nvram_64.c b/arch/powerpc/kernel/nvram_64.c index eae61b044e9e..496d6393bd41 100644 --- a/arch/powerpc/kernel/nvram_64.c +++ b/arch/powerpc/kernel/nvram_64.c @@ -792,21 +792,17 @@ static ssize_t dev_nvram_write(struct file *file, const char __user *buf, count = min_t(size_t, count, size - *ppos); count = min(count, PAGE_SIZE); - ret = -ENOMEM; - tmp = kmalloc(count, GFP_KERNEL); - if (!tmp) - goto out; - - ret = -EFAULT; - if (copy_from_user(tmp, buf, count)) + tmp = memdup_user(buf, count); + if (IS_ERR(tmp)) { + ret = PTR_ERR(tmp); goto out; + } ret = ppc_md.nvram_write(tmp, count, ppos); -out: kfree(tmp); +out: return ret; - } static long dev_nvram_ioctl(struct file *file, unsigned int cmd,