pstore/ramoops: fixup driver removal

commit 4407de74df18ed405cc5998990004c813ccfdbde upstream.

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 resources
were allocated. And then, in the free path, rely on ->max_dump_cnt in
the free path.

Cc: Anton Vorontsov <anton@enomsg.org>
Cc: Colin Cross <ccross@android.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Acked-by: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 319c3a60cfa52622cada3922b9023262e21c57fb..905caba36529fe071971fff8bffd3ce22199bd83 100644 (file)
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -375,13 +375,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,
@@ -406,7 +407,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++) {
@@ -417,6 +418,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@0x%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;
@@ -424,7 +430,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;
 }
 
@@ -583,7 +591,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;