MIME-Version: 1.0
In-Reply-To: <1415913389-17734-1-git-send-email-johannes@sipsolutions.net>
References: <1415913389-17734-1-git-send-email-johannes@sipsolutions.net>
Date: Thu, 13 Nov 2014 13:56:44 -0800
Message-ID: <CAGXu5jL8CBPXyiZM+AQpNUOvF=rQxay7ozmEaLuRL8WSiRQ2tA@mail.gmail.com>
Subject: Re: [PATCH] devcoredump: provide a one-way disable function
From: Kees Cook <keescook@google.com>
To: Johannes Berg <johannes@sipsolutions.net>
Cc: Greg Kroah-Hartmann <gregkh@linux-foundation.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Johannes Berg <johannes.berg@intel.com>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org

On Thu, Nov 13, 2014 at 1:16 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> Since device/firmware coredumps can contain private data, it can
> be desirable to turn them off unconditionally to be certain that
> no such data will be collected by the system.
>
> To achieve this, provide a "disabled" sysfs class attribute that
> can only be changed from 0 to 1 and not back. Upon disabling,
> discard existing coredumps and stop storing new ones.
>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>

Great; thanks for implementing this! This will come in handy for Chrome OS. :)

Reviewed-by: Kees Cook <keescook@chromium.org>

> ---
>  drivers/base/devcoredump.c | 56 +++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 48 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/base/devcoredump.c b/drivers/base/devcoredump.c
> index 96614b04544c..1bd120a0b084 100644
> --- a/drivers/base/devcoredump.c
> +++ b/drivers/base/devcoredump.c
> @@ -31,6 +31,11 @@
>  #include <linux/fs.h>
>  #include <linux/workqueue.h>
>
> +static struct class devcd_class;
> +
> +/* global disable flag, for security purposes */
> +static bool devcd_disabled;
> +
>  /* if data isn't read by userspace after 5 minutes then delete it */
>  #define DEVCD_TIMEOUT  (HZ * 60 * 5)
>
> @@ -121,11 +126,51 @@ static const struct attribute_group *devcd_dev_groups[] = {
>         &devcd_dev_group, NULL,
>  };
>
> +static int devcd_free(struct device *dev, void *data)
> +{
> +       struct devcd_entry *devcd = dev_to_devcd(dev);
> +
> +       flush_delayed_work(&devcd->del_wk);
> +       return 0;
> +}
> +
> +static ssize_t disabled_show(struct class *class, struct class_attribute *attr,
> +                            char *buf)
> +{
> +       return sprintf(buf, "%d\n", devcd_disabled);
> +}
> +
> +static ssize_t disabled_store(struct class *class, struct class_attribute *attr,
> +                             const char *buf, size_t count)
> +{
> +       long tmp = simple_strtol(buf, NULL, 10);
> +
> +       /*
> +        * This essentially makes the attribute write-once, since you can't
> +        * go back to not having it disabled. This is intentional, it serves
> +        * as a system lockdown feature.
> +        */
> +       if (tmp != 1)
> +               return -EINVAL;

Just a nit, but writing "0" is valid if devcd_disabled = false?

> +
> +       devcd_disabled = true;
> +
> +       class_for_each_device(&devcd_class, NULL, NULL, devcd_free);
> +
> +       return count;
> +}
> +
> +static struct class_attribute devcd_class_attrs[] = {
> +       __ATTR_RW(disabled),
> +       __ATTR_NULL
> +};
> +
>  static struct class devcd_class = {
>         .name           = "devcoredump",
>         .owner          = THIS_MODULE,
>         .dev_release    = devcd_dev_release,
>         .dev_groups     = devcd_dev_groups,
> +       .class_attrs    = devcd_class_attrs,
>  };
>
>  static ssize_t devcd_readv(char *buffer, loff_t offset, size_t count,
> @@ -192,6 +237,9 @@ void dev_coredumpm(struct device *dev, struct module *owner,
>         struct devcd_entry *devcd;
>         struct device *existing;
>
> +       if (devcd_disabled)
> +               goto free;
> +
>         existing = class_find_device(&devcd_class, NULL, dev,
>                                      devcd_match_failing);
>         if (existing) {
> @@ -249,14 +297,6 @@ static int __init devcoredump_init(void)
>  }
>  __initcall(devcoredump_init);
>
> -static int devcd_free(struct device *dev, void *data)
> -{
> -       struct devcd_entry *devcd = dev_to_devcd(dev);
> -
> -       flush_delayed_work(&devcd->del_wk);
> -       return 0;
> -}
> -
>  static void __exit devcoredump_exit(void)
>  {
>         class_for_each_device(&devcd_class, NULL, NULL, devcd_free);
> --
> 2.1.1
>

-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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/