lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Tue, 13 Jan 2015 09:58:36 -0800
From:	Kees Cook <keescook@...omium.org>
To:	Mark Salyzyn <salyzyn@...roid.com>
Cc:	LKML <linux-kernel@...r.kernel.org>, Joe Perches <joe@...ches.com>,
	Anton Vorontsov <anton@...msg.org>,
	Colin Cross <ccross@...roid.com>,
	Tony Luck <tony.luck@...el.com>
Subject: Re: [PATCH v2 3/3] pstore: add pmsg

On Tue, Jan 6, 2015 at 9:49 AM, Mark Salyzyn <salyzyn@...roid.com> wrote:
> A secured user-space accessible pstore object. Writes
> to /dev/pmsg0 are appended to the buffer, on reboot
> the persistent contents are available in
> /sys/fs/pstore/pmsg-ramoops-[ID].
>
> One possible use is syslogd, or other daemon, can
> write messages, then on reboot provides a means to
> triage user-space activities leading up to a panic
> as a companion to the pstore dmesg or console logs.
>
> Signed-off-by: Mark Salyzyn <salyzyn@...roid.com>
> ---
>  fs/pstore/Kconfig          |  10 +++++
>  fs/pstore/Makefile         |   2 +
>  fs/pstore/inode.c          |   3 ++
>  fs/pstore/internal.h       |   6 +++
>  fs/pstore/platform.c       |   1 +
>  fs/pstore/pmsg.c           | 101 +++++++++++++++++++++++++++++++++++++++++++++
>  fs/pstore/ram.c            |  46 ++++++++++++++++++---
>  include/linux/pstore.h     |   1 +
>  include/linux/pstore_ram.h |   1 +
>  9 files changed, 166 insertions(+), 5 deletions(-)
>  create mode 100644 fs/pstore/pmsg.c
>
> diff --git a/fs/pstore/Kconfig b/fs/pstore/Kconfig
> index 983d951..916b8e2 100644
> --- a/fs/pstore/Kconfig
> +++ b/fs/pstore/Kconfig
> @@ -21,6 +21,16 @@ config PSTORE_CONSOLE
>           When the option is enabled, pstore will log all kernel
>           messages, even if no oops or panic happened.
>
> +config PSTORE_PMSG
> +       bool "Log user space messages"
> +       depends on PSTORE
> +       help
> +         When the option is enabled, pstore will export a character
> +         interface /dev/pmsg0 to log user space messages. On reboot
> +         data can be retrieved from /sys/fs/pstore/pmsg-ramoops-[ID].
> +
> +         If unsure, say N.
> +
>  config PSTORE_FTRACE
>         bool "Persistent function tracer"
>         depends on PSTORE
> diff --git a/fs/pstore/Makefile b/fs/pstore/Makefile
> index 4c9095c..e647d8e 100644
> --- a/fs/pstore/Makefile
> +++ b/fs/pstore/Makefile
> @@ -7,5 +7,7 @@ obj-y += pstore.o
>  pstore-objs += inode.o platform.o
>  obj-$(CONFIG_PSTORE_FTRACE)    += ftrace.o
>
> +obj-$(CONFIG_PSTORE_PMSG)      += pmsg.o
> +
>  ramoops-objs += ram.o ram_core.o
>  obj-$(CONFIG_PSTORE_RAM)       += ramoops.o
> diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
> index 1948567..a241d3e 100644
> --- a/fs/pstore/inode.c
> +++ b/fs/pstore/inode.c
> @@ -361,6 +361,9 @@ int pstore_mkfile(enum pstore_type_id type, char *psname, u64 id, int count,
>                 scnprintf(name, sizeof(name), "powerpc-common-%s-%lld",
>                           psname, id);
>                 break;
> +       case PSTORE_TYPE_PMSG:
> +               scnprintf(name, sizeof(name), "pmsg-%s-%lld", psname, id);
> +               break;
>         case PSTORE_TYPE_UNKNOWN:
>                 scnprintf(name, sizeof(name), "unknown-%s-%lld", psname, id);
>                 break;
> diff --git a/fs/pstore/internal.h b/fs/pstore/internal.h
> index 3b3d305..c36ba2c 100644
> --- a/fs/pstore/internal.h
> +++ b/fs/pstore/internal.h
> @@ -45,6 +45,12 @@ extern void pstore_register_ftrace(void);
>  static inline void pstore_register_ftrace(void) {}
>  #endif
>
> +#ifdef CONFIG_PSTORE_PMSG
> +extern void pstore_register_pmsg(void);
> +#else
> +static inline void pstore_register_pmsg(void) {}
> +#endif
> +
>  extern struct pstore_info *psinfo;
>
>  extern void    pstore_set_kmsg_bytes(int);
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index 0a9b72c..15ee78c 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -447,6 +447,7 @@ int pstore_register(struct pstore_info *psi)
>         if ((psi->flags & PSTORE_FLAGS_FRAGILE) == 0) {
>                 pstore_register_console();
>                 pstore_register_ftrace();
> +               pstore_register_pmsg();
>         }
>
>         if (pstore_update_ms >= 0) {
> diff --git a/fs/pstore/pmsg.c b/fs/pstore/pmsg.c
> new file mode 100644
> index 0000000..d50d818
> --- /dev/null
> +++ b/fs/pstore/pmsg.c
> @@ -0,0 +1,101 @@
> +/*
> + * Copyright 2014  Google, Inc.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that 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.
> + */
> +
> +#include <linux/cdev.h>
> +#include <linux/device.h>
> +#include <linux/fs.h>
> +#include <linux/uaccess.h>
> +#include "internal.h"
> +
> +static loff_t pmsg_lseek(struct file *file, loff_t offset, int orig)
> +{
> +       return file->f_pos = 0;
> +}
> +
> +static ssize_t write_pmsg(struct file *file, const char __user *buf,
> +                         size_t count, loff_t *ppos)
> +{
> +       size_t i;
> +
> +       if (!count)
> +               return 0;
> +
> +       if (!access_ok(VERIFY_READ, buf, count))
> +               return -EFAULT;
> +
> +       for (i = 0; i < count; ) {
> +               char buffer[512];

This feels like a lot of stack space to use for a buffer. Would it
maybe be better to either reduce the copy size or use a larger kmalloc
or vmalloc buffer to act as the bounce buffer to pass to write_buf?

> +               size_t c = min(count - i, sizeof(buffer));
> +               u64 id;
> +               long ret;
> +
> +               ret = __copy_from_user(buffer, buf + i, c);
> +               if (ret != 0)
> +                       return -EFAULT;
> +               psinfo->write_buf(PSTORE_TYPE_PMSG, 0, &id, 0, buffer, 0, c,
> +                                 psinfo);
> +
> +               i += c;
> +       }
> +       return count;
> +}
> +
> +static const struct file_operations pmsg_fops = {
> +       .owner          = THIS_MODULE,
> +       .llseek         = pmsg_lseek,
> +       .write          = write_pmsg,
> +};
> +
> +static struct class *pmsg_class;
> +static int pmsg_major;
> +#define PMSG_NAME "pmsg"
> +
> +static char *pmsg_devnode(struct device *dev, umode_t *mode)
> +{
> +       if (mode)
> +               *mode = 0220;
> +       return NULL;
> +}
> +
> +void pstore_register_pmsg(void)
> +{
> +       struct device *pmsg_device;
> +
> +       pmsg_major = register_chrdev(0, PMSG_NAME, &pmsg_fops);
> +       if (pmsg_major < 0) {
> +               pr_err("register_chrdev failed for pmsg\n");
> +               goto err;
> +       }
> +
> +       pmsg_class = class_create(THIS_MODULE, PMSG_NAME);
> +       if (IS_ERR(pmsg_class)) {
> +               pr_err("pmsg: device class file already in use\n");
> +               goto err_class;
> +       }
> +       pmsg_class->devnode = pmsg_devnode;
> +
> +       pmsg_device = device_create(pmsg_class, NULL, MKDEV(pmsg_major, 0),
> +                                       NULL, "%s%d", PMSG_NAME, 0);
> +       if (IS_ERR(pmsg_device)) {
> +               pr_err("pmsg: failed to create device\n");

Instead of mentioning pmsg directly here and in the pr_err()s above,
maybe set it via the pr_* defines:

#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

And maybe use KBUILD_MODNAME instead of PMSG_NAME or vice versa?

> +               goto err_device;
> +       }
> +       return;
> +
> +err_device:
> +       class_destroy(pmsg_class);
> +err_class:
> +       unregister_chrdev(pmsg_major, PMSG_NAME);
> +err:
> +       return;
> +}
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index 34ed8f8..39d1373 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -51,6 +51,10 @@ static ulong ramoops_ftrace_size = MIN_MEM_SIZE;
>  module_param_named(ftrace_size, ramoops_ftrace_size, ulong, 0400);
>  MODULE_PARM_DESC(ftrace_size, "size of ftrace log");
>
> +static ulong ramoops_pmsg_size = MIN_MEM_SIZE;
> +module_param_named(pmsg_size, ramoops_pmsg_size, ulong, 0400);
> +MODULE_PARM_DESC(pmsg_size, "size of user space message log");
> +
>  static ulong mem_address;
>  module_param(mem_address, ulong, 0400);
>  MODULE_PARM_DESC(mem_address,
> @@ -82,12 +86,14 @@ struct ramoops_context {
>         struct persistent_ram_zone **przs;
>         struct persistent_ram_zone *cprz;
>         struct persistent_ram_zone *fprz;
> +       struct persistent_ram_zone *mprz;
>         phys_addr_t phys_addr;
>         unsigned long size;
>         unsigned int memtype;
>         size_t record_size;
>         size_t console_size;
>         size_t ftrace_size;
> +       size_t pmsg_size;
>         int dump_oops;
>         struct persistent_ram_ecc_info ecc_info;
>         unsigned int max_dump_cnt;
> @@ -96,6 +102,7 @@ struct ramoops_context {
>         unsigned int dump_read_cnt;
>         unsigned int console_read_cnt;
>         unsigned int ftrace_read_cnt;
> +       unsigned int pmsg_read_cnt;
>         struct pstore_info pstore;
>  };
>
> @@ -109,6 +116,7 @@ static int ramoops_pstore_open(struct pstore_info *psi)
>         cxt->dump_read_cnt = 0;
>         cxt->console_read_cnt = 0;
>         cxt->ftrace_read_cnt = 0;
> +       cxt->pmsg_read_cnt = 0;
>         return 0;
>  }
>
> @@ -164,6 +172,12 @@ static int ramoops_read_kmsg_hdr(char *buffer, struct timespec *time,
>         return header_length;
>  }
>
> +static bool prz_ok(struct persistent_ram_zone *prz)
> +{
> +       return !!prz && !!(persistent_ram_old_size(prz) +
> +                          persistent_ram_ecc_string(prz, NULL, 0));
> +}

The addition of prz_ok() seems like a separate change?

> +
>  static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
>                                    int *count, struct timespec *time,
>                                    char **buf, bool *compressed,
> @@ -178,13 +192,16 @@ static ssize_t ramoops_pstore_read(u64 *id, enum pstore_type_id *type,
>         prz = ramoops_get_next_prz(cxt->przs, &cxt->dump_read_cnt,
>                                    cxt->max_dump_cnt, id, type,
>                                    PSTORE_TYPE_DMESG, 1);
> -       if (!prz)
> +       if (!prz_ok(prz))
>                 prz = ramoops_get_next_prz(&cxt->cprz, &cxt->console_read_cnt,
>                                            1, id, type, PSTORE_TYPE_CONSOLE, 0);
> -       if (!prz)
> +       if (!prz_ok(prz))
>                 prz = ramoops_get_next_prz(&cxt->fprz, &cxt->ftrace_read_cnt,
>                                            1, id, type, PSTORE_TYPE_FTRACE, 0);
> -       if (!prz)
> +       if (!prz_ok(prz))
> +               prz = ramoops_get_next_prz(&cxt->mprz, &cxt->pmsg_read_cnt,
> +                                          1, id, type, PSTORE_TYPE_PMSG, 0);
> +       if (!prz_ok(prz))
>                 return 0;
>
>         if (!persistent_ram_old(prz))
> @@ -252,6 +269,11 @@ static int notrace ramoops_pstore_write_buf(enum pstore_type_id type,
>                         return -ENOMEM;
>                 persistent_ram_write(cxt->fprz, buf, size);
>                 return 0;
> +       } else if (type == PSTORE_TYPE_PMSG) {
> +               if (!cxt->mprz)
> +                       return -ENOMEM;
> +               persistent_ram_write(cxt->mprz, buf, size);
> +               return 0;
>         }
>
>         if (type != PSTORE_TYPE_DMESG)
> @@ -309,6 +331,9 @@ static int ramoops_pstore_erase(enum pstore_type_id type, u64 id, int count,
>         case PSTORE_TYPE_FTRACE:
>                 prz = cxt->fprz;
>                 break;
> +       case PSTORE_TYPE_PMSG:
> +               prz = cxt->mprz;
> +               break;
>         default:
>                 return -EINVAL;
>         }
> @@ -435,7 +460,7 @@ static int ramoops_probe(struct platform_device *pdev)
>                 goto fail_out;
>
>         if (!pdata->mem_size || (!pdata->record_size && !pdata->console_size &&
> -                       !pdata->ftrace_size)) {
> +                       !pdata->ftrace_size && !pdata->pmsg_size)) {
>                 pr_err("The memory size and the record/console size must be "
>                         "non-zero\n");
>                 goto fail_out;
> @@ -447,6 +472,8 @@ static int ramoops_probe(struct platform_device *pdev)
>                 pdata->console_size = rounddown_pow_of_two(pdata->console_size);
>         if (pdata->ftrace_size && !is_power_of_2(pdata->ftrace_size))
>                 pdata->ftrace_size = rounddown_pow_of_two(pdata->ftrace_size);
> +       if (pdata->pmsg_size && !is_power_of_2(pdata->pmsg_size))
> +               pdata->pmsg_size = rounddown_pow_of_two(pdata->pmsg_size);
>
>         cxt->size = pdata->mem_size;
>         cxt->phys_addr = pdata->mem_address;
> @@ -454,12 +481,14 @@ static int ramoops_probe(struct platform_device *pdev)
>         cxt->record_size = pdata->record_size;
>         cxt->console_size = pdata->console_size;
>         cxt->ftrace_size = pdata->ftrace_size;
> +       cxt->pmsg_size = pdata->pmsg_size;
>         cxt->dump_oops = pdata->dump_oops;
>         cxt->ecc_info = pdata->ecc_info;
>
>         paddr = cxt->phys_addr;
>
> -       dump_mem_sz = cxt->size - cxt->console_size - cxt->ftrace_size;
> +       dump_mem_sz = cxt->size - cxt->console_size - cxt->ftrace_size
> +                       - cxt->pmsg_size;
>         err = ramoops_init_przs(dev, cxt, &paddr, dump_mem_sz);
>         if (err)
>                 goto fail_out;
> @@ -474,6 +503,10 @@ static int ramoops_probe(struct platform_device *pdev)
>         if (err)
>                 goto fail_init_fprz;
>
> +       err = ramoops_init_prz(dev, cxt, &cxt->mprz, &paddr, cxt->pmsg_size, 0);
> +       if (err)
> +               goto fail_init_mprz;
> +
>         cxt->pstore.data = cxt;
>         /*
>          * Console can handle any buffer size, so prefer LOG_LINE_MAX. If we
> @@ -517,6 +550,8 @@ fail_buf:
>         kfree(cxt->pstore.buf);
>  fail_clear:
>         cxt->pstore.bufsize = 0;
> +       kfree(cxt->mprz);
> +fail_init_mprz:
>         kfree(cxt->fprz);
>  fail_init_fprz:
>         kfree(cxt->cprz);
> @@ -574,6 +609,7 @@ static void ramoops_register_dummy(void)
>         dummy_data->record_size = record_size;
>         dummy_data->console_size = ramoops_console_size;
>         dummy_data->ftrace_size = ramoops_ftrace_size;
> +       dummy_data->pmsg_size = ramoops_pmsg_size;
>         dummy_data->dump_oops = dump_oops;
>         /*
>          * For backwards compatibility ramoops.ecc=1 means 16 bytes ECC
> diff --git a/include/linux/pstore.h b/include/linux/pstore.h
> index ece0c6b..8884f6e 100644
> --- a/include/linux/pstore.h
> +++ b/include/linux/pstore.h
> @@ -39,6 +39,7 @@ enum pstore_type_id {
>         PSTORE_TYPE_PPC_RTAS    = 4,
>         PSTORE_TYPE_PPC_OF      = 5,
>         PSTORE_TYPE_PPC_COMMON  = 6,
> +       PSTORE_TYPE_PMSG        = 7,
>         PSTORE_TYPE_UNKNOWN     = 255
>  };
>
> diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
> index 4af3fdc..9c9d6c1 100644
> --- a/include/linux/pstore_ram.h
> +++ b/include/linux/pstore_ram.h
> @@ -81,6 +81,7 @@ struct ramoops_platform_data {
>         unsigned long   record_size;
>         unsigned long   console_size;
>         unsigned long   ftrace_size;
> +       unsigned long   pmsg_size;
>         int             dump_oops;
>         struct persistent_ram_ecc_info ecc_info;
>  };
> --
> 2.2.0.rc0.207.ga3a616c
>

Some nits above. Overall, this seems like a fine idea. Would it make
sense to enforce a single opener for these writes to avoid
interleaving?

-Kees

-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists