linux-kernel - Re: [RESEND][PATCH v2] clk_register_clkdev: remove format string interface

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] [day] [month] [year] [list]

Message-ID: <CAGXu5jKvOwL-EhsR7kmBFMeFzq=ra10u2LqL9R1MVOv5N0PH5A@mail.gmail.com>
Date:	Wed, 13 Jan 2016 12:37:50 -0800
From:	Kees Cook <keescook@...omium.org>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Russell King <linux@....linux.org.uk>,
	Nicolas Iooss <nicolas.iooss_linux@....org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RESEND][PATCH v2] clk_register_clkdev: remove format string interface

On Thu, Jan 7, 2016 at 11:32 AM, Kees Cook <keescook@...omium.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@...omium.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