mtd: cfi_cmdset_0002: don't free cfi->cfiq in error path of cfi_amdstd_setup()
diff mbox series

Message ID 20191008023637.133416-1-houtao1@huawei.com
State New
Delegated to: Vignesh R
Headers show
Series
  • mtd: cfi_cmdset_0002: don't free cfi->cfiq in error path of cfi_amdstd_setup()
Related show

Commit Message

Hou Tao Oct. 8, 2019, 2:36 a.m. UTC
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(-)

Comments

Hou Tao Oct. 14, 2019, 12:23 p.m. UTC | #1
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;
>  }
>  
>
Richard Weinberger Oct. 15, 2019, 7:47 p.m. UTC | #2
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>
Vignesh Raghavendra Oct. 16, 2019, 5:38 a.m. UTC | #3
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>
>
Hou Tao Oct. 16, 2019, 6:43 a.m. UTC | #4
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>
>>
>

Patch
diff mbox series

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;
 }