[PATCH] devcoredump: provide a one-way disable function

[PATCH] devcoredump: provide a one-way disable function

[Johannes Berg]

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>
---
 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;
+
+	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

Re: [PATCH] devcoredump: provide a one-way disable function

[Kees Cook]

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

Re: [PATCH] devcoredump: provide a one-way disable function

[Johannes Berg]

On Thu, 2014-11-13 at 13:56 -0800, Kees Cook wrote:

> > +       /*
> > +        * 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?

I thought about that too, but what would the point be? The only
operation you ever can/want to do is writing "1" to it to disable the
framework.

johannes

Re: [PATCH] devcoredump: provide a one-way disable function

[Kees Cook]

On Thu, Nov 13, 2014 at 2:36 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2014-11-13 at 13:56 -0800, Kees Cook wrote:
>
>> > +       /*
>> > +        * 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?
>
> I thought about that too, but what would the point be? The only
> operation you ever can/want to do is writing "1" to it to disable the
> framework.

Yup, seems like not a useful thing to do, but figured I'd point it out
just in case. I think the patch is fine as-is.

-Kees

-- 
Kees Cook
Chrome OS Security