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

From: Kees Cook
Date: Thu Nov 13 2014 - 16:56:51 EST


On Thu, Nov 13, 2014 at 1:16 PM, Johannes Berg
<johannes@xxxxxxxxxxxxxxxx> wrote:
> From: Johannes Berg <johannes.berg@xxxxxxxxx>
>
> 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@xxxxxxxxx>

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

Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>

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