linux-kernel - Re: [PATCH v2] pstore: Avoid duplicate call of persistent_ram

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>] [day] [month] [year] [list]
Message-ID: <CAGXu5jJ_Xsf6KGpDCt5yvdwST9smutdwEeh3eRgj0zEmjz8zKg@mail.gmail.com>
Date:   Sun, 28 Oct 2018 16:03:17 +0000
From:   Kees Cook <keescook@...omium.org>
To:     Peng15 Wang 王鹏 <wangpeng15@...omi.com>
Cc:     "anton@...msg.org" <anton@...msg.org>,
        "ccross@...roid.com" <ccross@...roid.com>,
        "tony.luck@...el.com" <tony.luck@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Joel Fernandes <joel@...lfernandes.org>
Subject: Re: [PATCH v2] pstore: Avoid duplicate call of persistent_ram_zap()

On Sat, Oct 27, 2018 at 2:08 PM, Peng15 Wang 王鹏 <wangpeng15@...omi.com> wrote:
> When initialing prz with invalid data in buffer(no PERSISTENT_RAM_SIG),
> function call path is like this:
>
> ramoops_init_prz ->
> |
> |-> persistent_ram_new -> persistent_ram_post_init -> persistent_ram_zap
> |
> |-> persistent_ram_zap
>
> As we can see, persistent_ram_zap() is called twice.
> We can avoid this by adding an option to persistent_ram_new(), and
> only call persistent_ram_zap() when it is needed.
>
> Signed-off-by: Peng Wang <wangpeng15@...omi.com>
> ---
>  fs/pstore/ram.c            |  5 +++--
>  fs/pstore/ram_core.c       | 11 +++++++----
>  include/linux/pstore_ram.h |  3 ++-
>  3 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index ffcff6516e89..3044274de2f0 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -596,7 +596,8 @@ static int ramoops_init_przs(const char *name,
>                                           name, i, *cnt - 1);
>                 prz_ar[i] = persistent_ram_new(*paddr, zone_sz, sig,
>                                                &cxt->ecc_info,
> -                                              cxt->memtype, flags, label);
> +                                              cxt->memtype, flags,
> +                                              label, true);
>                 if (IS_ERR(prz_ar[i])) {
>                         err = PTR_ERR(prz_ar[i]);
>                         dev_err(dev, "failed to request %s mem region (0x%zx@...llx): %d\n",
> @@ -640,7 +641,7 @@ static int ramoops_init_prz(const char *name,
>
>         label = kasprintf(GFP_KERNEL, "ramoops:%s", name);
>         *prz = persistent_ram_new(*paddr, sz, sig, &cxt->ecc_info,
> -                                 cxt->memtype, 0, label);
> +                                 cxt->memtype, 0, label, false);
>         if (IS_ERR(*prz)) {
>                 int err = PTR_ERR(*prz);
>
> diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
> index 12e21f789194..d8a520c8741c 100644
> --- a/fs/pstore/ram_core.c
> +++ b/fs/pstore/ram_core.c
> @@ -486,7 +486,8 @@ static int persistent_ram_buffer_map(phys_addr_t start, phys_addr_t size,
>  }
>
>  static int persistent_ram_post_init(struct persistent_ram_zone *prz, u32 sig,
> -                                   struct persistent_ram_ecc_info *ecc_info)
> +                                   struct persistent_ram_ecc_info *ecc_info,
> +                                   bool zap_option)
>  {
>         int ret;
>
> @@ -514,7 +515,8 @@ static int persistent_ram_post_init(struct persistent_ram_zone *prz, u32 sig,
>
>         /* Rewind missing or invalid memory area. */
>         prz->buffer->sig = sig;
> -       persistent_ram_zap(prz);
> +       if (zap_option)
> +               persistent_ram_zap(prz);

This part of persistent_ram_post_init() handles the "invalid buffer"
case, which should always zap. The question is whether or not to zap
in the case of a valid buffer (the "return 0" earlier in the
function). I think you v2 patch needs similar changes found in your
v1: the v2 patch also needs to remove the "return 0" and replace it
with "zap_option = true;" and to remove the zap call from
ramoops_init_prz(). Then I think all the paths will be consolidated.

-Kees

>
>         return 0;
>  }
> @@ -548,7 +550,8 @@ void persistent_ram_free(struct persistent_ram_zone *prz)
>
>  struct persistent_ram_zone *persistent_ram_new(phys_addr_t start, size_t size,
>                         u32 sig, struct persistent_ram_ecc_info *ecc_info,
> -                       unsigned int memtype, u32 flags, char *label)
> +                       unsigned int memtype, u32 flags, char *label,
> +                       bool zap_option)
>  {
>         struct persistent_ram_zone *prz;
>         int ret = -ENOMEM;
> @@ -568,7 +571,7 @@ struct persistent_ram_zone *persistent_ram_new(phys_addr_t start, size_t size,
>         if (ret)
>                 goto err;
>
> -       ret = persistent_ram_post_init(prz, sig, ecc_info);
> +       ret = persistent_ram_post_init(prz, sig, ecc_info, zap_option);
>         if (ret)
>                 goto err;
>
> diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
> index 602d64725222..5d4acad9fe56 100644
> --- a/include/linux/pstore_ram.h
> +++ b/include/linux/pstore_ram.h
> @@ -66,7 +66,8 @@ struct persistent_ram_zone {
>
>  struct persistent_ram_zone *persistent_ram_new(phys_addr_t start, size_t size,
>                         u32 sig, struct persistent_ram_ecc_info *ecc_info,
> -                       unsigned int memtype, u32 flags, char *label);
> +                       unsigned int memtype, u32 flags, char *label,
> +                       bool zap_option);
>  void persistent_ram_free(struct persistent_ram_zone *prz);
>  void persistent_ram_zap(struct persistent_ram_zone *prz);
>
> --
> 2.19.1



-- 
Kees Cook