linux-kernel - Re: [PATCH] regmap: Disable debugfs when locking is disabled

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]

Message-ID: <CAMRc=McRUqhPx7x=ZKmdQuHioP+XTkc9q3QqcLQEEhTz0iA9ug@mail.gmail.com>
Date:   Wed, 13 Dec 2017 16:50:20 +0100
From:   Bartosz Golaszewski <brgl@...ev.pl>
To:     Mark Brown <broonie@...nel.org>
Cc:     Lars-Peter Clausen <lars@...afoo.de>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] regmap: Disable debugfs when locking is disabled

2017-12-12 18:13 GMT+01:00 Mark Brown <broonie@...nel.org>:
> The recently added support for disabling the regmap internal locking left
> debugfs enabled for devices with the locking disabled. This is a problem
> since debugfs allows userspace to do things like initiate reads from the
> hardware which will use the scratch buffers protected by the regmap locking
> so could cause data corruption.
>
> For safety address this by just disabling debugfs for these devices. That
> is overly conservative since some of the debugfs files just read internal
> data structures but it's much simpler to implmement and less likely to
> lead to problems with tooling that works with debugfs.
>
> Reported-by: Lars-Peter Clausen <lars@...afoo.de>
> Signed-off-by: Mark Brown <broonie@...nel.org>
> ---
>
> Compile tested only so far.
>
>  drivers/base/regmap/internal.h       | 8 ++++++++
>  drivers/base/regmap/regmap-debugfs.c | 3 +++
>  drivers/base/regmap/regmap.c         | 1 +
>  3 files changed, 12 insertions(+)
>
> diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
> index 8641183cac2f..8d652023f8bd 100644
> --- a/drivers/base/regmap/internal.h
> +++ b/drivers/base/regmap/internal.h
> @@ -77,6 +77,7 @@ struct regmap {
>         int async_ret;
>
>  #ifdef CONFIG_DEBUG_FS
> +       bool debugfs_disable;
>         struct dentry *debugfs;
>         const char *debugfs_name;
>
> @@ -215,10 +216,17 @@ struct regmap_field {
>  extern void regmap_debugfs_initcall(void);
>  extern void regmap_debugfs_init(struct regmap *map, const char *name);
>  extern void regmap_debugfs_exit(struct regmap *map);
> +
> +static void regmap_debugfs_disable(struct regmap *map)
> +{
> +       map->debugfs_disable = true;
> +}
> +
>  #else
>  static inline void regmap_debugfs_initcall(void) { }
>  static inline void regmap_debugfs_init(struct regmap *map, const char *name) { }
>  static inline void regmap_debugfs_exit(struct regmap *map) { }
> +static inline void regmap_debugfs_disable(struct regmap *map) { }
>  #endif
>
>  /* regcache core declarations */
> diff --git a/drivers/base/regmap/regmap-debugfs.c b/drivers/base/regmap/regmap-debugfs.c
> index 36ce3511c733..c8ecefd75d6f 100644
> --- a/drivers/base/regmap/regmap-debugfs.c
> +++ b/drivers/base/regmap/regmap-debugfs.c
> @@ -529,6 +529,9 @@ void regmap_debugfs_init(struct regmap *map, const char *name)
>         struct regmap_range_node *range_node;
>         const char *devname = "dummy";
>
> +       if (map->debugfs_disable)
> +               return;
> +

I would still add the dev_dbg() here like in my version, since someone
may start wondering why the debugfs files are not being creating for
certain regmaps.

Thanks,
Bartosz

>         /* If we don't have the debugfs root yet, postpone init */
>         if (!regmap_debugfs_root) {
>                 struct regmap_debugfs_node *node;
> diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
> index 1da2a9fc40b0..b884da59dd71 100644
> --- a/drivers/base/regmap/regmap.c
> +++ b/drivers/base/regmap/regmap.c
> @@ -674,6 +674,7 @@ struct regmap *__regmap_init(struct device *dev,
>
>         if (config->disable_locking) {
>                 map->lock = map->unlock = regmap_lock_unlock_empty;
> +               regmap_debugfs_disable(map);
>         } else if (config->lock && config->unlock) {
>                 map->lock = config->lock;
>                 map->unlock = config->unlock;
> --
> 2.15.1
>