From 6ae56031f17d04ebcf380fd1c074b7c352d4ac26 Mon Sep 17 00:00:00 2001 From: Sebastian Andrzej Siewior Date: Thu, 8 Sep 2016 13:48:05 +0200 Subject: [PATCH] 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 Cc: Colin Cross Cc: Kees Cook Cc: Tony Luck Cc: Namhyung Kim Acked-by: Namhyung Kim Signed-off-by: Sebastian Andrzej Siewior Signed-off-by: Kees Cook Signed-off-by: Greg Kroah-Hartman --- 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 319c3a60cfa5..905caba36529 100644 --- 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; -- 2.34.1