From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S941258AbcIHT1I (ORCPT ); Thu, 8 Sep 2016 15:27:08 -0400 Received: from mail-wm0-f53.google.com ([74.125.82.53]:35569 "EHLO mail-wm0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932165AbcIHT1G (ORCPT ); Thu, 8 Sep 2016 15:27:06 -0400 MIME-Version: 1.0 In-Reply-To: <20160908114806.10686-1-bigeasy@linutronix.de> References: <20160908114806.10686-1-bigeasy@linutronix.de> From: Kees Cook Date: Thu, 8 Sep 2016 12:27:03 -0700 X-Google-Sender-Auth: eTpfFm7JX_ZsgGnduwWOX60kiPY Message-ID: Subject: Re: [REPOST PATCH 1/2] pstore/ramoops: fixup driver removal To: Sebastian Andrzej Siewior Cc: Andrew Morton , LKML , Anton Vorontsov , Colin Cross , Tony Luck , Namhyung Kim Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Sep 8, 2016 at 4:48 AM, Sebastian Andrzej Siewior 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 > Cc: Colin Cross > Cc: Kees Cook > Cc: Tony Luck > Cc: Namhyung Kim > Acked-by: Namhyung Kim > Signed-off-by: Sebastian Andrzej Siewior Thanks! I've applied this for -next. -Kees > --- > 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@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; > @@ -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 > -- Kees Cook Nexus Security