linux-kernel - Re: [PATCH] pstore/ramoops: fixup driver removal

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: <CADWwUUbjTrwe5+y8Pe=_z4gPJ=X=YM-GLKt7QXa_GQS=kW+5-Q@mail.gmail.com>
Date:   Wed, 31 Aug 2016 12:27:49 +0900
From:   Namhyung Kim <namhyung@...il.com>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Anton Vorontsov <anton@...msg.org>,
        Colin Cross <ccross@...roid.com>,
        Kees Cook <keescook@...omium.org>,
        Tony Luck <tony.luck@...el.com>
Subject: Re: [PATCH] pstore/ramoops: fixup driver removal

(Resending with all recipients, sorry)

On Sat, Aug 20, 2016 at 1:52 AM, Sebastian Andrzej Siewior
<bigeasy@...utronix.de> wrote:
> A basic rmmod ramoops segfaults. Let's see why.
>
> Since commit 34f0ec82e0a9 ("pstore: Correct the max_dump_cnt clearing of
> ramoops") sets ->max_dump_cnt to zero before looping over ->przs but we
> didn't use it before that either.
>
> And since commit ee1d267423a1 ("pstore: add pstore unregister") we free
> that memory on rmmod.
>
> But even then, we looped until a NULL pointer or ERR. I don't see where
> it is ensured that the last member is NULL. Let's try this instead:
> simply error recovery and free. Clean up in error case where ressouces
> were allocated. And then, in the free path, rely on ->max_dump_cnt in
> the free path.
>
> Cc: Anton Vorontsov <anton@...msg.org>
> Cc: Colin Cross <ccross@...roid.com>
> Cc: Kees Cook <keescook@...omium.org>
> Cc: Tony Luck <tony.luck@...el.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>

Fwiw,

Acked-by: Namhyung Kim <namhyung@...nel.org>

Thanks,
Namhyung


> ---
>
> Not sure if I miss something obvious or not.
>
>  fs/pstore/ram.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index 7a034d62cf8c..2340262a7e97 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -377,13 +377,14 @@ static void ramoops_free_przs(struct ramoops_context *cxt)
>  {
>         int i;
>
> -       cxt->max_dump_cnt = 0;
>         if (!cxt->przs)
>                 return;
>
> -       for (i = 0; !IS_ERR_OR_NULL(cxt->przs[i]); i++)
> +       for (i = 0; i < cxt->max_dump_cnt; i++)
>                 persistent_ram_free(cxt->przs[i]);
> +
>         kfree(cxt->przs);
> +       cxt->max_dump_cnt = 0;
>  }
>
>  static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt,
> @@ -408,7 +409,7 @@ static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt,
>                              GFP_KERNEL);
>         if (!cxt->przs) {
>                 dev_err(dev, "failed to initialize a prz array for dumps\n");
> -               goto fail_prz;
> +               goto fail_mem;
>         }
>
>         for (i = 0; i < cxt->max_dump_cnt; i++) {
> @@ -419,6 +420,11 @@ static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt,
>                         err = PTR_ERR(cxt->przs[i]);
>                         dev_err(dev, "failed to request mem region (0x%zx@...llx): %d\n",
>                                 cxt->record_size, (unsigned long long)*paddr, err);
> +
> +                       while (i > 0) {
> +                               i--;
> +                               persistent_ram_free(cxt->przs[i]);
> +                       }
>                         goto fail_prz;
>                 }
>                 *paddr += cxt->record_size;
> @@ -426,7 +432,9 @@ static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt,
>
>         return 0;
>  fail_prz:
> -       ramoops_free_przs(cxt);
> +       kfree(cxt->przs);
> +fail_mem:
> +       cxt->max_dump_cnt = 0;
>         return err;
>  }
>
> @@ -659,7 +667,6 @@ static int ramoops_remove(struct platform_device *pdev)
>         struct ramoops_context *cxt = &oops_cxt;
>
>         pstore_unregister(&cxt->pstore);
> -       cxt->max_dump_cnt = 0;
>
>         kfree(cxt->pstore.buf);
>         cxt->pstore.bufsize = 0;
> --
> 2.9.3
>