[RESEND][PATCH v2] clk_register_clkdev: remove format string interface

[RESEND][PATCH v2] clk_register_clkdev: remove format string interface

[Kees Cook]

Many callers either use NULL or const strings for the third argument of
clk_register_clkdev. For those that do not and use a non-const string,
this is a risk for format strings being accidentally processed (for
example in device names). As this interface is already used as if it
weren't a format string (prints nothing when NULL), and there are zero
users of the format strings, remove the format string interface to make
sure format strings will not leak into the clkdev.

$ git grep '\bclk_register_clkdev\b' | grep % | wc -l
0

Unfortunately, all the internals expect a va_list even though they treat
a NULL format string as special. To deal with this, we must pass either
(..., "%s", string) or (..., NULL) so that a the va_list will be created
correctly (passing the name as an argument, not as a format string).

Signed-off-by: Kees Cook <keescook@chromium.org>
---
v2:
- make calling logic more clear, thanks to rmk.
---
 drivers/clk/clkdev.c   | 31 +++++++++++++++++++++++++------
 include/linux/clkdev.h |  3 +--
 2 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 779b6ff0c7ad..eb20b941154b 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -353,11 +353,25 @@ void clkdev_drop(struct clk_lookup *cl)
 }
 EXPORT_SYMBOL(clkdev_drop);
 
+static struct clk_lookup *__clk_register_clkdev(struct clk_hw *hw,
+						const char *con_id,
+						const char *dev_id, ...)
+{
+	struct clk_lookup *cl;
+	va_list ap;
+
+	va_start(ap, dev_id);
+	cl = vclkdev_create(hw, con_id, dev_id, ap);
+	va_end(ap);
+
+	return cl;
+}
+
 /**
  * clk_register_clkdev - register one clock lookup for a struct clk
  * @clk: struct clk to associate with all clk_lookups
  * @con_id: connection ID string on device
- * @dev_id: format string describing device name
+ * @dev_id: string describing device name
  *
  * con_id or dev_id may be NULL as a wildcard, just as in the rest of
  * clkdev.
@@ -368,17 +382,22 @@ EXPORT_SYMBOL(clkdev_drop);
  * after clk_register().
  */
 int clk_register_clkdev(struct clk *clk, const char *con_id,
-	const char *dev_fmt, ...)
+	const char *dev_id)
 {
 	struct clk_lookup *cl;
-	va_list ap;
 
 	if (IS_ERR(clk))
 		return PTR_ERR(clk);
 
-	va_start(ap, dev_fmt);
-	cl = vclkdev_create(__clk_get_hw(clk), con_id, dev_fmt, ap);
-	va_end(ap);
+	/*
+	 * Since dev_id can be NULL, and NULL is handled specially, we must
+	 * pass it as either a NULL format string, or with "%s".
+	 */
+	if (dev_id)
+		cl = __clk_register_clkdev(__clk_get_hw(clk), con_id, "%s",
+					   dev_id);
+	else
+		cl = __clk_register_clkdev(__clk_get_hw(clk), con_id, NULL);
 
 	return cl ? 0 : -ENOMEM;
 }
diff --git a/include/linux/clkdev.h b/include/linux/clkdev.h
index 08bffcc466de..c2c04f7cbe8a 100644
--- a/include/linux/clkdev.h
+++ b/include/linux/clkdev.h
@@ -44,8 +44,7 @@ struct clk_lookup *clkdev_create(struct clk *clk, const char *con_id,
 void clkdev_add_table(struct clk_lookup *, size_t);
 int clk_add_alias(const char *, const char *, const char *, struct device *);
 
-int clk_register_clkdev(struct clk *, const char *, const char *, ...)
-	__printf(3, 4);
+int clk_register_clkdev(struct clk *, const char *, const char *);
 int clk_register_clkdevs(struct clk *, struct clk_lookup *, size_t);
 
 #ifdef CONFIG_COMMON_CLK
-- 
2.6.3


-- 
Kees Cook
Chrome OS & Brillo Security

[RESEND][PATCH v2] clk_register_clkdev: remove format string interface

[Kees Cook]

On Thu, Jan 7, 2016 at 11:32 AM, Kees Cook <keescook@chromium.org> wrote:
> Many callers either use NULL or const strings for the third argument of
> clk_register_clkdev. For those that do not and use a non-const string,
> this is a risk for format strings being accidentally processed (for
> example in device names). As this interface is already used as if it
> weren't a format string (prints nothing when NULL), and there are zero
> users of the format strings, remove the format string interface to make
> sure format strings will not leak into the clkdev.
>
> $ git grep '\bclk_register_clkdev\b' | grep % | wc -l
> 0
>
> Unfortunately, all the internals expect a va_list even though they treat
> a NULL format string as special. To deal with this, we must pass either
> (..., "%s", string) or (..., NULL) so that a the va_list will be created
> correctly (passing the name as an argument, not as a format string).
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> v2:
> - make calling logic more clear, thanks to rmk.

Should this go via -mm, a clk tree, or through the ARM patch tracker?

Thanks!

-Kees

> ---
>  drivers/clk/clkdev.c   | 31 +++++++++++++++++++++++++------
>  include/linux/clkdev.h |  3 +--
>  2 files changed, 26 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
> index 779b6ff0c7ad..eb20b941154b 100644
> --- a/drivers/clk/clkdev.c
> +++ b/drivers/clk/clkdev.c
> @@ -353,11 +353,25 @@ void clkdev_drop(struct clk_lookup *cl)
>  }
>  EXPORT_SYMBOL(clkdev_drop);
>
> +static struct clk_lookup *__clk_register_clkdev(struct clk_hw *hw,
> +                                               const char *con_id,
> +                                               const char *dev_id, ...)
> +{
> +       struct clk_lookup *cl;
> +       va_list ap;
> +
> +       va_start(ap, dev_id);
> +       cl = vclkdev_create(hw, con_id, dev_id, ap);
> +       va_end(ap);
> +
> +       return cl;
> +}
> +
>  /**
>   * clk_register_clkdev - register one clock lookup for a struct clk
>   * @clk: struct clk to associate with all clk_lookups
>   * @con_id: connection ID string on device
> - * @dev_id: format string describing device name
> + * @dev_id: string describing device name
>   *
>   * con_id or dev_id may be NULL as a wildcard, just as in the rest of
>   * clkdev.
> @@ -368,17 +382,22 @@ EXPORT_SYMBOL(clkdev_drop);
>   * after clk_register().
>   */
>  int clk_register_clkdev(struct clk *clk, const char *con_id,
> -       const char *dev_fmt, ...)
> +       const char *dev_id)
>  {
>         struct clk_lookup *cl;
> -       va_list ap;
>
>         if (IS_ERR(clk))
>                 return PTR_ERR(clk);
>
> -       va_start(ap, dev_fmt);
> -       cl = vclkdev_create(__clk_get_hw(clk), con_id, dev_fmt, ap);
> -       va_end(ap);
> +       /*
> +        * Since dev_id can be NULL, and NULL is handled specially, we must
> +        * pass it as either a NULL format string, or with "%s".
> +        */
> +       if (dev_id)
> +               cl = __clk_register_clkdev(__clk_get_hw(clk), con_id, "%s",
> +                                          dev_id);
> +       else
> +               cl = __clk_register_clkdev(__clk_get_hw(clk), con_id, NULL);
>
>         return cl ? 0 : -ENOMEM;
>  }
> diff --git a/include/linux/clkdev.h b/include/linux/clkdev.h
> index 08bffcc466de..c2c04f7cbe8a 100644
> --- a/include/linux/clkdev.h
> +++ b/include/linux/clkdev.h
> @@ -44,8 +44,7 @@ struct clk_lookup *clkdev_create(struct clk *clk, const char *con_id,
>  void clkdev_add_table(struct clk_lookup *, size_t);
>  int clk_add_alias(const char *, const char *, const char *, struct device *);
>
> -int clk_register_clkdev(struct clk *, const char *, const char *, ...)
> -       __printf(3, 4);
> +int clk_register_clkdev(struct clk *, const char *, const char *);
>  int clk_register_clkdevs(struct clk *, struct clk_lookup *, size_t);
>
>  #ifdef CONFIG_COMMON_CLK
> --
> 2.6.3
>
>
> --
> Kees Cook
> Chrome OS & Brillo Security



-- 
Kees Cook
Chrome OS & Brillo Security