| Message ID | 20191008023637.133416-1-houtao1@huawei.com |
|---|---|
| State | Accepted |
| Delegated to: | Vignesh R |
| Headers | show |
| Series | mtd: cfi_cmdset_0002: don't free cfi->cfiq in error path of cfi_amdstd_setup() | expand |
ping ? On 2019/10/8 10:36, Hou Tao wrote: > Else there may be a double-free problem, because cfi->cfiq will > be freed by mtd_do_chip_probe() if both the two invocations of > check_cmd_set() return failure. > > Also check cfi_intelext_setup() & cfi_staa_setup() to find out > that cfi->cfiq is not freed as well in these functions. > > Signed-off-by: Hou Tao <houtao1@huawei.com> > --- > drivers/mtd/chips/cfi_cmdset_0002.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c > index cf8c8be40a9c..7eaa4b523197 100644 > --- a/drivers/mtd/chips/cfi_cmdset_0002.c > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c > @@ -785,7 +785,6 @@ static struct mtd_info *cfi_amdstd_setup(struct mtd_info *mtd) > kfree(mtd->eraseregions); > kfree(mtd); > kfree(cfi->cmdset_priv); > - kfree(cfi->cfiq); > return NULL; > } > >
On Tue, Oct 8, 2019 at 4:29 AM Hou Tao <houtao1@huawei.com> wrote: > > Else there may be a double-free problem, because cfi->cfiq will > be freed by mtd_do_chip_probe() if both the two invocations of > check_cmd_set() return failure. > > Also check cfi_intelext_setup() & cfi_staa_setup() to find out > that cfi->cfiq is not freed as well in these functions. This sentence does not make sense to me. > Signed-off-by: Hou Tao <houtao1@huawei.com> > --- > drivers/mtd/chips/cfi_cmdset_0002.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c > index cf8c8be40a9c..7eaa4b523197 100644 > --- a/drivers/mtd/chips/cfi_cmdset_0002.c > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c > @@ -785,7 +785,6 @@ static struct mtd_info *cfi_amdstd_setup(struct mtd_info *mtd) > kfree(mtd->eraseregions); > kfree(mtd); > kfree(cfi->cmdset_priv); > - kfree(cfi->cfiq); > return NULL; > } Other than that, Reviewed-by: Richard Weinberger <richard@nod.at>
Hi Hou, On 16/10/19 1:17 AM, Richard Weinberger wrote: > On Tue, Oct 8, 2019 at 4:29 AM Hou Tao <houtao1@huawei.com> wrote: >> >> Else there may be a double-free problem, because cfi->cfiq will >> be freed by mtd_do_chip_probe() if both the two invocations of >> check_cmd_set() return failure. >> >> Also check cfi_intelext_setup() & cfi_staa_setup() to find out >> that cfi->cfiq is not freed as well in these functions. > I guess you are trying to imply cfi_amdstd_setup() equivalents in cfi_cmdset_0001.c (cfi_intelext_setup()) and cfi_cmdset_0020.c (cfi_staa_setup()) dont't call kfree(cfi->cfiq). So cfi_amdstd_setup() should not be freeing that pointer either? This reference to other drivers in commit msg is quite confusing. My suggestion would be to drop above line. Let me know if that sound good. I will drop the it while applying. > This sentence does not make sense to me. > >> Signed-off-by: Hou Tao <houtao1@huawei.com> >> --- >> drivers/mtd/chips/cfi_cmdset_0002.c | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c >> index cf8c8be40a9c..7eaa4b523197 100644 >> --- a/drivers/mtd/chips/cfi_cmdset_0002.c >> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c >> @@ -785,7 +785,6 @@ static struct mtd_info *cfi_amdstd_setup(struct mtd_info *mtd) >> kfree(mtd->eraseregions); >> kfree(mtd); >> kfree(cfi->cmdset_priv); >> - kfree(cfi->cfiq); >> return NULL; >> } > > Other than that, > Reviewed-by: Richard Weinberger <richard@nod.at> >
Hi, On 2019/10/16 13:38, Vignesh Raghavendra wrote: > Hi Hou, > > On 16/10/19 1:17 AM, Richard Weinberger wrote: >> On Tue, Oct 8, 2019 at 4:29 AM Hou Tao <houtao1@huawei.com> wrote: >>> >>> Else there may be a double-free problem, because cfi->cfiq will >>> be freed by mtd_do_chip_probe() if both the two invocations of >>> check_cmd_set() return failure. >>> >>> Also check cfi_intelext_setup() & cfi_staa_setup() to find out >>> that cfi->cfiq is not freed as well in these functions. >> > > I guess you are trying to imply cfi_amdstd_setup() equivalents in > cfi_cmdset_0001.c (cfi_intelext_setup()) and cfi_cmdset_0020.c > (cfi_staa_setup()) dont't call kfree(cfi->cfiq). So cfi_amdstd_setup() > should not be freeing that pointer either? > No. The lines are used to illustrate the same mistake doesn't happen in cfi_cmdset_0001 & 0020 instead of cfi_cmdstd_setup() needs to do the same thing as others cmdset. > This reference to other drivers in commit msg is quite confusing. My > suggestion would be to drop above line. > It's OK for me if you drop these line. > Let me know if that sound good. I will drop the it while applying. > > Regards, Tao >> This sentence does not make sense to me. >> >>> Signed-off-by: Hou Tao <houtao1@huawei.com> >>> --- >>> drivers/mtd/chips/cfi_cmdset_0002.c | 1 - >>> 1 file changed, 1 deletion(-) >>> >>> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c >>> index cf8c8be40a9c..7eaa4b523197 100644 >>> --- a/drivers/mtd/chips/cfi_cmdset_0002.c >>> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c >>> @@ -785,7 +785,6 @@ static struct mtd_info *cfi_amdstd_setup(struct mtd_info *mtd) >>> kfree(mtd->eraseregions); >>> kfree(mtd); >>> kfree(cfi->cmdset_priv); >>> - kfree(cfi->cfiq); >>> return NULL; >>> } >> >> Other than that, >> Reviewed-by: Richard Weinberger <richard@nod.at> >> >
diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c index cf8c8be40a9c..7eaa4b523197 100644 --- a/drivers/mtd/chips/cfi_cmdset_0002.c +++ b/drivers/mtd/chips/cfi_cmdset_0002.c @@ -785,7 +785,6 @@ static struct mtd_info *cfi_amdstd_setup(struct mtd_info *mtd) kfree(mtd->eraseregions); kfree(mtd); kfree(cfi->cmdset_priv); - kfree(cfi->cfiq); return NULL; }
Else there may be a double-free problem, because cfi->cfiq will be freed by mtd_do_chip_probe() if both the two invocations of check_cmd_set() return failure. Also check cfi_intelext_setup() & cfi_staa_setup() to find out that cfi->cfiq is not freed as well in these functions. Signed-off-by: Hou Tao <houtao1@huawei.com> --- drivers/mtd/chips/cfi_cmdset_0002.c | 1 - 1 file changed, 1 deletion(-)