Commit 4407de74 authored by Sebastian Andrzej Siewior's avatar Sebastian Andrzej Siewior Committed by Kees Cook

pstore/ramoops: fixup driver removal

A basic rmmod ramoops segfaults. Let's see why.

Since commit 34f0ec82 ("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 ee1d2674 ("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: default avatarNamhyung Kim <namhyung@kernel.org>
Signed-off-by: default avatarSebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: default avatarKees Cook <keescook@chromium.org>
Cc: stable@vger.kernel.org # 4.4.x-
parent d71f0586
...@@ -377,13 +377,14 @@ static void ramoops_free_przs(struct ramoops_context *cxt) ...@@ -377,13 +377,14 @@ static void ramoops_free_przs(struct ramoops_context *cxt)
{ {
int i; int i;
cxt->max_dump_cnt = 0;
if (!cxt->przs) if (!cxt->przs)
return; 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]); persistent_ram_free(cxt->przs[i]);
kfree(cxt->przs); kfree(cxt->przs);
cxt->max_dump_cnt = 0;
} }
static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt, 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, ...@@ -408,7 +409,7 @@ static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt,
GFP_KERNEL); GFP_KERNEL);
if (!cxt->przs) { if (!cxt->przs) {
dev_err(dev, "failed to initialize a prz array for dumps\n"); 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++) { 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, ...@@ -419,6 +420,11 @@ static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt,
err = PTR_ERR(cxt->przs[i]); err = PTR_ERR(cxt->przs[i]);
dev_err(dev, "failed to request mem region (0x%zx@0x%llx): %d\n", dev_err(dev, "failed to request mem region (0x%zx@0x%llx): %d\n",
cxt->record_size, (unsigned long long)*paddr, err); cxt->record_size, (unsigned long long)*paddr, err);
while (i > 0) {
i--;
persistent_ram_free(cxt->przs[i]);
}
goto fail_prz; goto fail_prz;
} }
*paddr += cxt->record_size; *paddr += cxt->record_size;
...@@ -426,7 +432,9 @@ static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt, ...@@ -426,7 +432,9 @@ static int ramoops_init_przs(struct device *dev, struct ramoops_context *cxt,
return 0; return 0;
fail_prz: fail_prz:
ramoops_free_przs(cxt); kfree(cxt->przs);
fail_mem:
cxt->max_dump_cnt = 0;
return err; return err;
} }
...@@ -659,7 +667,6 @@ static int ramoops_remove(struct platform_device *pdev) ...@@ -659,7 +667,6 @@ static int ramoops_remove(struct platform_device *pdev)
struct ramoops_context *cxt = &oops_cxt; struct ramoops_context *cxt = &oops_cxt;
pstore_unregister(&cxt->pstore); pstore_unregister(&cxt->pstore);
cxt->max_dump_cnt = 0;
kfree(cxt->pstore.buf); kfree(cxt->pstore.buf);
cxt->pstore.bufsize = 0; cxt->pstore.bufsize = 0;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment