diff mbox series

[linux-next,v2,1/3] platform/x86/intel/tpmi: Fix double free in tpmi_create_device()

Message ID 20230309040107.534716-2-dzm91@hust.edu.cn
State New
Headers
Series [linux-next,v2,1/3] platform/x86/intel/tpmi: Fix double free in tpmi_create_device() |

Commit Message

Dongliang Mu March 9, 2023, 4:01 a.m. UTC 
  The previous commit 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix
double free reported by Smatch") incorrectly handle the deallocation of
res variable. As shown in the comment, intel_vsec_add_aux handles all
the deallocation of res and feature_vsec_dev. Therefore, kfree(res) can
still cause double free if intel_vsec_add_aux returns error.

Fix this by adjusting the error handling part in tpmi_create_device,
following the function intel_vsec_add_dev.

Fixes: 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix double free reported by Smatch")
Signed-off-by: Dongliang Mu <dzm91@hust.edu.cn>
---
 drivers/platform/x86/intel/tpmi.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

Comments

Dan Carpenter March 14, 2023, 2:22 p.m. UTC | #1 
On Thu, Mar 09, 2023 at 12:01:05PM +0800, Dongliang Mu wrote:
> The previous commit 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix
> double free reported by Smatch") incorrectly handle the deallocation of
> res variable. As shown in the comment, intel_vsec_add_aux handles all
> the deallocation of res and feature_vsec_dev. Therefore, kfree(res) can
> still cause double free if intel_vsec_add_aux returns error.
> 
> Fix this by adjusting the error handling part in tpmi_create_device,
> following the function intel_vsec_add_dev.
> 
> Fixes: 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix double free reported by Smatch")
> Signed-off-by: Dongliang Mu <dzm91@hust.edu.cn>
> ---

Yeah.  These patches are right.  The earlier fix still has a double
free.  Devres stuff is confusing...

Reviewed-by: Dan Carpenter <error27@gmail.com>

regards,
dan carpenter
Hans de Goede March 16, 2023, 2:25 p.m. UTC | #2 
Hi,

On 3/9/23 05:01, Dongliang Mu wrote:
> The previous commit 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix
> double free reported by Smatch") incorrectly handle the deallocation of
> res variable. As shown in the comment, intel_vsec_add_aux handles all
> the deallocation of res and feature_vsec_dev. Therefore, kfree(res) can
> still cause double free if intel_vsec_add_aux returns error.
> 
> Fix this by adjusting the error handling part in tpmi_create_device,
> following the function intel_vsec_add_dev.
> 
> Fixes: 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix double free reported by Smatch")
> Signed-off-by: Dongliang Mu <dzm91@hust.edu.cn>

IIRC then after this v2 was posted I still saw some comments on the original v1 which was not posted on the list. Without the v1 comments being on the list and this archived, I have lost track of what the status of these patches is.

Srinivas, can you let me know if I should merge these, or if more changes are necessary ?

From the off-list discussion of v1 I got the impression more changes are necessary, but I'm not sure.

Regards,

Hans




> ---
>  drivers/platform/x86/intel/tpmi.c | 17 ++++-------------
>  1 file changed, 4 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/tpmi.c b/drivers/platform/x86/intel/tpmi.c
> index c999732b0f1e..882fe5e4763f 100644
> --- a/drivers/platform/x86/intel/tpmi.c
> +++ b/drivers/platform/x86/intel/tpmi.c
> @@ -215,8 +215,8 @@ static int tpmi_create_device(struct intel_tpmi_info *tpmi_info,
>  
>  	feature_vsec_dev = kzalloc(sizeof(*feature_vsec_dev), GFP_KERNEL);
>  	if (!feature_vsec_dev) {
> -		ret = -ENOMEM;
> -		goto free_res;
> +		kfree(res);
> +		return -ENOMEM;
>  	}
>  
>  	snprintf(feature_id_name, sizeof(feature_id_name), "tpmi-%s", name);
> @@ -242,17 +242,8 @@ static int tpmi_create_device(struct intel_tpmi_info *tpmi_info,
>  	 * feature_vsec_dev memory is also freed as part of device
>  	 * delete.
>  	 */
> -	ret = intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev->auxdev.dev,
> -				 feature_vsec_dev, feature_id_name);
> -	if (ret)
> -		goto free_res;
> -
> -	return 0;
> -
> -free_res:
> -	kfree(res);
> -
> -	return ret;
> +	return intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev->auxdev.dev,
> +				  feature_vsec_dev, feature_id_name);
>  }
>  
>  static int tpmi_create_devices(struct intel_tpmi_info *tpmi_info)
srinivas pandruvada March 16, 2023, 6:18 p.m. UTC | #3 
Hi Hans,

On Thu, 2023-03-16 at 15:25 +0100, Hans de Goede wrote:
> Hi,
> 
> On 3/9/23 05:01, Dongliang Mu wrote:
> > The previous commit 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix
> > double free reported by Smatch") incorrectly handle the
> > deallocation of
> > res variable. As shown in the comment, intel_vsec_add_aux handles
> > all
> > the deallocation of res and feature_vsec_dev. Therefore, kfree(res)
> > can
> > still cause double free if intel_vsec_add_aux returns error.
> > 
> > Fix this by adjusting the error handling part in
> > tpmi_create_device,
> > following the function intel_vsec_add_dev.
> > 
> > Fixes: 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix double free
> > reported by Smatch")
> > Signed-off-by: Dongliang Mu <dzm91@hust.edu.cn>

Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

> 
> IIRC then after this v2 was posted I still saw some comments on the
> original v1 which was not posted on the list. Without the v1 comments
> being on the list and this archived, I have lost track of what the
> status of these patches is.
> 
> Srinivas, can you let me know if I should merge these, or if more
> changes are necessary ?
> 
> From the off-list discussion of v1 I got the impression more changes
> are necessary, but I'm not sure.

I was looking for changes submitted by the following patch
"
[PATCH linux-next v2 3/3] drivers/platform/x86/intel: fix a memory leak
in intel_vsec_add_aux
"

Since I was not copied on this, I was unaware. So I was requesting this
change.

Thanks,
Srinivas

> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> > ---
> >  drivers/platform/x86/intel/tpmi.c | 17 ++++-------------
> >  1 file changed, 4 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel/tpmi.c
> > b/drivers/platform/x86/intel/tpmi.c
> > index c999732b0f1e..882fe5e4763f 100644
> > --- a/drivers/platform/x86/intel/tpmi.c
> > +++ b/drivers/platform/x86/intel/tpmi.c
> > @@ -215,8 +215,8 @@ static int tpmi_create_device(struct
> > intel_tpmi_info *tpmi_info,
> >  
> >         feature_vsec_dev = kzalloc(sizeof(*feature_vsec_dev),
> > GFP_KERNEL);
> >         if (!feature_vsec_dev) {
> > -               ret = -ENOMEM;
> > -               goto free_res;
> > +               kfree(res);
> > +               return -ENOMEM;
> >         }
> >  
> >         snprintf(feature_id_name, sizeof(feature_id_name), "tpmi-
> > %s", name);
> > @@ -242,17 +242,8 @@ static int tpmi_create_device(struct
> > intel_tpmi_info *tpmi_info,
> >          * feature_vsec_dev memory is also freed as part of device
> >          * delete.
> >          */
> > -       ret = intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev-
> > >auxdev.dev,
> > -                                feature_vsec_dev,
> > feature_id_name);
> > -       if (ret)
> > -               goto free_res;
> > -
> > -       return 0;
> > -
> > -free_res:
> > -       kfree(res);
> > -
> > -       return ret;
> > +       return intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev-
> > >auxdev.dev,
> > +                                 feature_vsec_dev,
> > feature_id_name);
> >  }
> >  
> >  static int tpmi_create_devices(struct intel_tpmi_info *tpmi_info)
>
Dongliang Mu March 17, 2023, 1:28 a.m. UTC | #4 
On 2023/3/17 02:18, srinivas pandruvada wrote:
> Hi Hans,
>
> On Thu, 2023-03-16 at 15:25 +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 3/9/23 05:01, Dongliang Mu wrote:
>>> The previous commit 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix
>>> double free reported by Smatch") incorrectly handle the
>>> deallocation of
>>> res variable. As shown in the comment, intel_vsec_add_aux handles
>>> all
>>> the deallocation of res and feature_vsec_dev. Therefore, kfree(res)
>>> can
>>> still cause double free if intel_vsec_add_aux returns error.
>>>
>>> Fix this by adjusting the error handling part in
>>> tpmi_create_device,
>>> following the function intel_vsec_add_dev.
>>>
>>> Fixes: 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix double free
>>> reported by Smatch")
>>> Signed-off-by: Dongliang Mu <dzm91@hust.edu.cn>
> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>
>> IIRC then after this v2 was posted I still saw some comments on the
>> original v1 which was not posted on the list. Without the v1 comments
>> being on the list and this archived, I have lost track of what the
>> status of these patches is.
>>
>> Srinivas, can you let me know if I should merge these, or if more
>> changes are necessary ?
>>
>>  From the off-list discussion of v1 I got the impression more changes
>> are necessary, but I'm not sure.
> I was looking for changes submitted by the following patch
> "
> [PATCH linux-next v2 3/3] drivers/platform/x86/intel: fix a memory leak
> in intel_vsec_add_aux
> "
>
> Since I was not copied on this, I was unaware. So I was requesting this
> change.
>
> Thanks,
> Srinivas

Hi Srinivas and Hans,

How about folding these three patches into one patch and resend a v3 patch?

This will get all people together and avoid the previous embarrassing 
sitation.

Dongliang Mu

>
>> Regards,
>>
>> Hans
>>
>>
>>
>>
>>> ---
>>>   drivers/platform/x86/intel/tpmi.c | 17 ++++-------------
>>>   1 file changed, 4 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/intel/tpmi.c
>>> b/drivers/platform/x86/intel/tpmi.c
>>> index c999732b0f1e..882fe5e4763f 100644
>>> --- a/drivers/platform/x86/intel/tpmi.c
>>> +++ b/drivers/platform/x86/intel/tpmi.c
>>> @@ -215,8 +215,8 @@ static int tpmi_create_device(struct
>>> intel_tpmi_info *tpmi_info,
>>>   
>>>          feature_vsec_dev = kzalloc(sizeof(*feature_vsec_dev),
>>> GFP_KERNEL);
>>>          if (!feature_vsec_dev) {
>>> -               ret = -ENOMEM;
>>> -               goto free_res;
>>> +               kfree(res);
>>> +               return -ENOMEM;
>>>          }
>>>   
>>>          snprintf(feature_id_name, sizeof(feature_id_name), "tpmi-
>>> %s", name);
>>> @@ -242,17 +242,8 @@ static int tpmi_create_device(struct
>>> intel_tpmi_info *tpmi_info,
>>>           * feature_vsec_dev memory is also freed as part of device
>>>           * delete.
>>>           */
>>> -       ret = intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev-
>>>> auxdev.dev,
>>> -                                feature_vsec_dev,
>>> feature_id_name);
>>> -       if (ret)
>>> -               goto free_res;
>>> -
>>> -       return 0;
>>> -
>>> -free_res:
>>> -       kfree(res);
>>> -
>>> -       return ret;
>>> +       return intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev-
>>>> auxdev.dev,
>>> +                                 feature_vsec_dev,
>>> feature_id_name);
>>>   }
>>>   
>>>   static int tpmi_create_devices(struct intel_tpmi_info *tpmi_info)
Hans de Goede March 17, 2023, 8:51 a.m. UTC | #5 
Hi,

On 3/17/23 02:28, Dongliang Mu wrote:
> 
> On 2023/3/17 02:18, srinivas pandruvada wrote:
>> Hi Hans,
>>
>> On Thu, 2023-03-16 at 15:25 +0100, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 3/9/23 05:01, Dongliang Mu wrote:
>>>> The previous commit 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix
>>>> double free reported by Smatch") incorrectly handle the
>>>> deallocation of
>>>> res variable. As shown in the comment, intel_vsec_add_aux handles
>>>> all
>>>> the deallocation of res and feature_vsec_dev. Therefore, kfree(res)
>>>> can
>>>> still cause double free if intel_vsec_add_aux returns error.
>>>>
>>>> Fix this by adjusting the error handling part in
>>>> tpmi_create_device,
>>>> following the function intel_vsec_add_dev.
>>>>
>>>> Fixes: 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix double free
>>>> reported by Smatch")
>>>> Signed-off-by: Dongliang Mu <dzm91@hust.edu.cn>
>> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>>
>>> IIRC then after this v2 was posted I still saw some comments on the
>>> original v1 which was not posted on the list. Without the v1 comments
>>> being on the list and this archived, I have lost track of what the
>>> status of these patches is.
>>>
>>> Srinivas, can you let me know if I should merge these, or if more
>>> changes are necessary ?
>>>
>>>  From the off-list discussion of v1 I got the impression more changes
>>> are necessary, but I'm not sure.
>> I was looking for changes submitted by the following patch
>> "
>> [PATCH linux-next v2 3/3] drivers/platform/x86/intel: fix a memory leak
>> in intel_vsec_add_aux
>> "
>>
>> Since I was not copied on this, I was unaware. So I was requesting this
>> change.
>>
>> Thanks,
>> Srinivas
> 
> Hi Srinivas and Hans,
> 
> How about folding these three patches into one patch and resend a v3 patch?
> 
> This will get all people together and avoid the previous embarrassing sitation.

If I understand things correctly then patch 1/3 needs 3/3 to function correctly, right ?

I would not fold them together, smaller patches are easier to review / understand, but maybe change the order and put patch 3/3 first? (so make it 1/3) ?

I can even do that when applying if you agree that is the better order.

Regards,

Hans



>>>> ---
>>>>   drivers/platform/x86/intel/tpmi.c | 17 ++++-------------
>>>>   1 file changed, 4 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/platform/x86/intel/tpmi.c
>>>> b/drivers/platform/x86/intel/tpmi.c
>>>> index c999732b0f1e..882fe5e4763f 100644
>>>> --- a/drivers/platform/x86/intel/tpmi.c
>>>> +++ b/drivers/platform/x86/intel/tpmi.c
>>>> @@ -215,8 +215,8 @@ static int tpmi_create_device(struct
>>>> intel_tpmi_info *tpmi_info,
>>>>            feature_vsec_dev = kzalloc(sizeof(*feature_vsec_dev),
>>>> GFP_KERNEL);
>>>>          if (!feature_vsec_dev) {
>>>> -               ret = -ENOMEM;
>>>> -               goto free_res;
>>>> +               kfree(res);
>>>> +               return -ENOMEM;
>>>>          }
>>>>            snprintf(feature_id_name, sizeof(feature_id_name), "tpmi-
>>>> %s", name);
>>>> @@ -242,17 +242,8 @@ static int tpmi_create_device(struct
>>>> intel_tpmi_info *tpmi_info,
>>>>           * feature_vsec_dev memory is also freed as part of device
>>>>           * delete.
>>>>           */
>>>> -       ret = intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev-
>>>>> auxdev.dev,
>>>> -                                feature_vsec_dev,
>>>> feature_id_name);
>>>> -       if (ret)
>>>> -               goto free_res;
>>>> -
>>>> -       return 0;
>>>> -
>>>> -free_res:
>>>> -       kfree(res);
>>>> -
>>>> -       return ret;
>>>> +       return intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev-
>>>>> auxdev.dev,
>>>> +                                 feature_vsec_dev,
>>>> feature_id_name);
>>>>   }
>>>>     static int tpmi_create_devices(struct intel_tpmi_info *tpmi_info)
srinivas pandruvada March 17, 2023, 10:23 a.m. UTC | #6 
Hi,

...
...

> > 

> > Hi Srinivas and Hans,
> > 
> > How about folding these three patches into one patch and resend a
> > v3 patch?
> > 
> > This will get all people together and avoid the previous
> > embarrassing sitation.
> 
> If I understand things correctly then patch 1/3 needs 3/3 to function
> correctly, right ?
> 
> I would not fold them together, smaller patches are easier to review
> / understand, but maybe change the order and put patch 3/3 first? (so
> make it 1/3) ?

That should be correct order. The patch 3/3 should be the first.

> 
> I can even do that when applying if you agree that is the better
> order.
Agree.

Thanks,
Srinivas

> 
> Regards,
> 
> Hans
> 
> 
> 
> > > > > ---
> > > > >   drivers/platform/x86/intel/tpmi.c | 17 ++++-------------
> > > > >   1 file changed, 4 insertions(+), 13 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/platform/x86/intel/tpmi.c
> > > > > b/drivers/platform/x86/intel/tpmi.c
> > > > > index c999732b0f1e..882fe5e4763f 100644
> > > > > --- a/drivers/platform/x86/intel/tpmi.c
> > > > > +++ b/drivers/platform/x86/intel/tpmi.c
> > > > > @@ -215,8 +215,8 @@ static int tpmi_create_device(struct
> > > > > intel_tpmi_info *tpmi_info,
> > > > >            feature_vsec_dev =
> > > > > kzalloc(sizeof(*feature_vsec_dev),
> > > > > GFP_KERNEL);
> > > > >          if (!feature_vsec_dev) {
> > > > > -               ret = -ENOMEM;
> > > > > -               goto free_res;
> > > > > +               kfree(res);
> > > > > +               return -ENOMEM;
> > > > >          }
> > > > >            snprintf(feature_id_name, sizeof(feature_id_name),
> > > > > "tpmi-
> > > > > %s", name);
> > > > > @@ -242,17 +242,8 @@ static int tpmi_create_device(struct
> > > > > intel_tpmi_info *tpmi_info,
> > > > >           * feature_vsec_dev memory is also freed as part of
> > > > > device
> > > > >           * delete.
> > > > >           */
> > > > > -       ret = intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev-
> > > > > > auxdev.dev,
> > > > > -                                feature_vsec_dev,
> > > > > feature_id_name);
> > > > > -       if (ret)
> > > > > -               goto free_res;
> > > > > -
> > > > > -       return 0;
> > > > > -
> > > > > -free_res:
> > > > > -       kfree(res);
> > > > > -
> > > > > -       return ret;
> > > > > +       return intel_vsec_add_aux(vsec_dev->pcidev,
> > > > > &vsec_dev-
> > > > > > auxdev.dev,
> > > > > +                                 feature_vsec_dev,
> > > > > feature_id_name);
> > > > >   }
> > > > >     static int tpmi_create_devices(struct intel_tpmi_info
> > > > > *tpmi_info)
>
srinivas pandruvada March 17, 2023, 10:27 a.m. UTC | #7 
Hi Dongliang,

> 
...
...


> Hi Srinivas and Hans,
> 
> How about folding these three patches into one patch and resend a v3
> patch?
> 
> This will get all people together and avoid the previous embarrassing
> sitation.

This is NOT an embarrassing situation.
Thanks for finding and fixing the issue.

Thanks,
Srinivas


> 
> Dongliang Mu
> 
> > 
> > > Regards,
> > > 
> > > Hans
> > > 
> > > 
> > > 
> > > 
> > > > ---
> > > >   drivers/platform/x86/intel/tpmi.c | 17 ++++-------------
> > > >   1 file changed, 4 insertions(+), 13 deletions(-)
> > > > 
> > > > diff --git a/drivers/platform/x86/intel/tpmi.c
> > > > b/drivers/platform/x86/intel/tpmi.c
> > > > index c999732b0f1e..882fe5e4763f 100644
> > > > --- a/drivers/platform/x86/intel/tpmi.c
> > > > +++ b/drivers/platform/x86/intel/tpmi.c
> > > > @@ -215,8 +215,8 @@ static int tpmi_create_device(struct
> > > > intel_tpmi_info *tpmi_info,
> > > >   
> > > >          feature_vsec_dev = kzalloc(sizeof(*feature_vsec_dev),
> > > > GFP_KERNEL);
> > > >          if (!feature_vsec_dev) {
> > > > -               ret = -ENOMEM;
> > > > -               goto free_res;
> > > > +               kfree(res);
> > > > +               return -ENOMEM;
> > > >          }
> > > >   
> > > >          snprintf(feature_id_name, sizeof(feature_id_name),
> > > > "tpmi-
> > > > %s", name);
> > > > @@ -242,17 +242,8 @@ static int tpmi_create_device(struct
> > > > intel_tpmi_info *tpmi_info,
> > > >           * feature_vsec_dev memory is also freed as part of
> > > > device
> > > >           * delete.
> > > >           */
> > > > -       ret = intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev-
> > > > > auxdev.dev,
> > > > -                                feature_vsec_dev,
> > > > feature_id_name);
> > > > -       if (ret)
> > > > -               goto free_res;
> > > > -
> > > > -       return 0;
> > > > -
> > > > -free_res:
> > > > -       kfree(res);
> > > > -
> > > > -       return ret;
> > > > +       return intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev-
> > > > > auxdev.dev,
> > > > +                                 feature_vsec_dev,
> > > > feature_id_name);
> > > >   }
> > > >   
> > > >   static int tpmi_create_devices(struct intel_tpmi_info
> > > > *tpmi_info)
Dongliang Mu March 17, 2023, 11:57 a.m. UTC | #8 
On 2023/3/17 18:23, srinivas pandruvada wrote:
> Hi,
>
> ...
> ...
>
>>> Hi Srinivas and Hans,
>>>
>>> How about folding these three patches into one patch and resend a
>>> v3 patch?
>>>
>>> This will get all people together and avoid the previous
>>> embarrassing sitation.
>> If I understand things correctly then patch 1/3 needs 3/3 to function
>> correctly, right ?
>>
>> I would not fold them together, smaller patches are easier to review
>> / understand, but maybe change the order and put patch 3/3 first? (so
>> make it 1/3) ?
> That should be correct order. The patch 3/3 should be the first.

Oh, yeah. The memory leak fix 3/3 should be the first. This is more 
reasonable.

BTW, I separated this memory leak fix due to that the kernel mainline is 
still vulnerable to this memory leak problem.

>
>> I can even do that when applying if you agree that is the better
>> order.
> Agree.
>
> Thanks,
> Srinivas
>
>> Regards,
>>
>> Hans
>>
>>
>>
>>>>>> ---
>>>>>>    drivers/platform/x86/intel/tpmi.c | 17 ++++-------------
>>>>>>    1 file changed, 4 insertions(+), 13 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/platform/x86/intel/tpmi.c
>>>>>> b/drivers/platform/x86/intel/tpmi.c
>>>>>> index c999732b0f1e..882fe5e4763f 100644
>>>>>> --- a/drivers/platform/x86/intel/tpmi.c
>>>>>> +++ b/drivers/platform/x86/intel/tpmi.c
>>>>>> @@ -215,8 +215,8 @@ static int tpmi_create_device(struct
>>>>>> intel_tpmi_info *tpmi_info,
>>>>>>             feature_vsec_dev =
>>>>>> kzalloc(sizeof(*feature_vsec_dev),
>>>>>> GFP_KERNEL);
>>>>>>           if (!feature_vsec_dev) {
>>>>>> -               ret = -ENOMEM;
>>>>>> -               goto free_res;
>>>>>> +               kfree(res);
>>>>>> +               return -ENOMEM;
>>>>>>           }
>>>>>>             snprintf(feature_id_name, sizeof(feature_id_name),
>>>>>> "tpmi-
>>>>>> %s", name);
>>>>>> @@ -242,17 +242,8 @@ static int tpmi_create_device(struct
>>>>>> intel_tpmi_info *tpmi_info,
>>>>>>            * feature_vsec_dev memory is also freed as part of
>>>>>> device
>>>>>>            * delete.
>>>>>>            */
>>>>>> -       ret = intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev-
>>>>>>> auxdev.dev,
>>>>>> -                                feature_vsec_dev,
>>>>>> feature_id_name);
>>>>>> -       if (ret)
>>>>>> -               goto free_res;
>>>>>> -
>>>>>> -       return 0;
>>>>>> -
>>>>>> -free_res:
>>>>>> -       kfree(res);
>>>>>> -
>>>>>> -       return ret;
>>>>>> +       return intel_vsec_add_aux(vsec_dev->pcidev,
>>>>>> &vsec_dev-
>>>>>>> auxdev.dev,
>>>>>> +                                 feature_vsec_dev,
>>>>>> feature_id_name);
>>>>>>    }
>>>>>>      static int tpmi_create_devices(struct intel_tpmi_info
>>>>>> *tpmi_info)
Dongliang Mu March 20, 2023, 2:43 a.m. UTC | #9 
On 2023/3/17 18:27, srinivas pandruvada wrote:
> Hi Dongliang,
>
> ...
> ...
>
>
>> Hi Srinivas and Hans,
>>
>> How about folding these three patches into one patch and resend a v3
>> patch?
>>
>> This will get all people together and avoid the previous embarrassing
>> sitation.
> This is NOT an embarrassing situation.
> Thanks for finding and fixing the issue.
>
> Thanks,
> Srinivas
>
Hi Srinivas,

Any conclusion about this patch set?

>> Dongliang Mu
>>
>>>> Regards,
>>>>
>>>> Hans
>>>>
>>>>
>>>>
>>>>
>>>>> ---
>>>>>    drivers/platform/x86/intel/tpmi.c | 17 ++++-------------
>>>>>    1 file changed, 4 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/drivers/platform/x86/intel/tpmi.c
>>>>> b/drivers/platform/x86/intel/tpmi.c
>>>>> index c999732b0f1e..882fe5e4763f 100644
>>>>> --- a/drivers/platform/x86/intel/tpmi.c
>>>>> +++ b/drivers/platform/x86/intel/tpmi.c
>>>>> @@ -215,8 +215,8 @@ static int tpmi_create_device(struct
>>>>> intel_tpmi_info *tpmi_info,
>>>>>    
>>>>>           feature_vsec_dev = kzalloc(sizeof(*feature_vsec_dev),
>>>>> GFP_KERNEL);
>>>>>           if (!feature_vsec_dev) {
>>>>> -               ret = -ENOMEM;
>>>>> -               goto free_res;
>>>>> +               kfree(res);
>>>>> +               return -ENOMEM;
>>>>>           }
>>>>>    
>>>>>           snprintf(feature_id_name, sizeof(feature_id_name),
>>>>> "tpmi-
>>>>> %s", name);
>>>>> @@ -242,17 +242,8 @@ static int tpmi_create_device(struct
>>>>> intel_tpmi_info *tpmi_info,
>>>>>            * feature_vsec_dev memory is also freed as part of
>>>>> device
>>>>>            * delete.
>>>>>            */
>>>>> -       ret = intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev-
>>>>>> auxdev.dev,
>>>>> -                                feature_vsec_dev,
>>>>> feature_id_name);
>>>>> -       if (ret)
>>>>> -               goto free_res;
>>>>> -
>>>>> -       return 0;
>>>>> -
>>>>> -free_res:
>>>>> -       kfree(res);
>>>>> -
>>>>> -       return ret;
>>>>> +       return intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev-
>>>>>> auxdev.dev,
>>>>> +                                 feature_vsec_dev,
>>>>> feature_id_name);
>>>>>    }
>>>>>    
>>>>>    static int tpmi_create_devices(struct intel_tpmi_info
>>>>> *tpmi_info)
srinivas pandruvada March 20, 2023, 6:32 a.m. UTC | #10 
On Mon, 2023-03-20 at 10:43 +0800, Dongliang Mu wrote:
> 
> On 2023/3/17 18:27, srinivas pandruvada wrote:
> > Hi Dongliang,
> > 
> > ...
> > ...
> > 
> > 
> > > Hi Srinivas and Hans,
> > > 
> > > How about folding these three patches into one patch and resend a
> > > v3
> > > patch?
> > > 
> > > This will get all people together and avoid the previous
> > > embarrassing
> > > sitation.
> > This is NOT an embarrassing situation.
> > Thanks for finding and fixing the issue.
> > 
> > Thanks,
> > Srinivas
> > 
> Hi Srinivas,
> 
> Any conclusion about this patch set?

Hans can reorder patches as he suggested and apply.

Thanks,
Srinivas

> 
> > > Dongliang Mu
> > > 
> > > > > Regards,
> > > > > 
> > > > > Hans
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > > ---
> > > > > >    drivers/platform/x86/intel/tpmi.c | 17 ++++-------------
> > > > > >    1 file changed, 4 insertions(+), 13 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/platform/x86/intel/tpmi.c
> > > > > > b/drivers/platform/x86/intel/tpmi.c
> > > > > > index c999732b0f1e..882fe5e4763f 100644
> > > > > > --- a/drivers/platform/x86/intel/tpmi.c
> > > > > > +++ b/drivers/platform/x86/intel/tpmi.c
> > > > > > @@ -215,8 +215,8 @@ static int tpmi_create_device(struct
> > > > > > intel_tpmi_info *tpmi_info,
> > > > > >    
> > > > > >           feature_vsec_dev =
> > > > > > kzalloc(sizeof(*feature_vsec_dev),
> > > > > > GFP_KERNEL);
> > > > > >           if (!feature_vsec_dev) {
> > > > > > -               ret = -ENOMEM;
> > > > > > -               goto free_res;
> > > > > > +               kfree(res);
> > > > > > +               return -ENOMEM;
> > > > > >           }
> > > > > >    
> > > > > >           snprintf(feature_id_name,
> > > > > > sizeof(feature_id_name),
> > > > > > "tpmi-
> > > > > > %s", name);
> > > > > > @@ -242,17 +242,8 @@ static int tpmi_create_device(struct
> > > > > > intel_tpmi_info *tpmi_info,
> > > > > >            * feature_vsec_dev memory is also freed as part
> > > > > > of
> > > > > > device
> > > > > >            * delete.
> > > > > >            */
> > > > > > -       ret = intel_vsec_add_aux(vsec_dev->pcidev,
> > > > > > &vsec_dev-
> > > > > > > auxdev.dev,
> > > > > > -                                feature_vsec_dev,
> > > > > > feature_id_name);
> > > > > > -       if (ret)
> > > > > > -               goto free_res;
> > > > > > -
> > > > > > -       return 0;
> > > > > > -
> > > > > > -free_res:
> > > > > > -       kfree(res);
> > > > > > -
> > > > > > -       return ret;
> > > > > > +       return intel_vsec_add_aux(vsec_dev->pcidev,
> > > > > > &vsec_dev-
> > > > > > > auxdev.dev,
> > > > > > +                                 feature_vsec_dev,
> > > > > > feature_id_name);
> > > > > >    }
> > > > > >    
> > > > > >    static int tpmi_create_devices(struct intel_tpmi_info
> > > > > > *tpmi_info)
Hans de Goede March 20, 2023, 10:32 a.m. UTC | #11 
Hi,

On 3/9/23 05:01, Dongliang Mu wrote:
> The previous commit 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix
> double free reported by Smatch") incorrectly handle the deallocation of
> res variable. As shown in the comment, intel_vsec_add_aux handles all
> the deallocation of res and feature_vsec_dev. Therefore, kfree(res) can
> still cause double free if intel_vsec_add_aux returns error.
> 
> Fix this by adjusting the error handling part in tpmi_create_device,
> following the function intel_vsec_add_dev.
> 
> Fixes: 6a192c0cbf38 ("platform/x86/intel/tpmi: Fix double free reported by Smatch")
> Signed-off-by: Dongliang Mu <dzm91@hust.edu.cn>

Note this patch causes the following compiler warnings:

  CC [M]  drivers/platform/x86/intel/tpmi.o
drivers/platform/x86/intel/tpmi.c: In function ‘tpmi_create_device’:
drivers/platform/x86/intel/tpmi.c:206:13: warning: unused variable ‘ret’ [-Wunused-variable]
  206 |         int ret, i;
      |             ^~~

I have fixed this and squashed the fix into the patch while applying it,
so there is no need to send a new version:

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans



> ---
>  drivers/platform/x86/intel/tpmi.c | 17 ++++-------------
>  1 file changed, 4 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/tpmi.c b/drivers/platform/x86/intel/tpmi.c
> index c999732b0f1e..882fe5e4763f 100644
> --- a/drivers/platform/x86/intel/tpmi.c
> +++ b/drivers/platform/x86/intel/tpmi.c
> @@ -215,8 +215,8 @@ static int tpmi_create_device(struct intel_tpmi_info *tpmi_info,
>  
>  	feature_vsec_dev = kzalloc(sizeof(*feature_vsec_dev), GFP_KERNEL);
>  	if (!feature_vsec_dev) {
> -		ret = -ENOMEM;
> -		goto free_res;
> +		kfree(res);
> +		return -ENOMEM;
>  	}
>  
>  	snprintf(feature_id_name, sizeof(feature_id_name), "tpmi-%s", name);
> @@ -242,17 +242,8 @@ static int tpmi_create_device(struct intel_tpmi_info *tpmi_info,
>  	 * feature_vsec_dev memory is also freed as part of device
>  	 * delete.
>  	 */
> -	ret = intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev->auxdev.dev,
> -				 feature_vsec_dev, feature_id_name);
> -	if (ret)
> -		goto free_res;
> -
> -	return 0;
> -
> -free_res:
> -	kfree(res);
> -
> -	return ret;
> +	return intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev->auxdev.dev,
> +				  feature_vsec_dev, feature_id_name);
>  }
>  
>  static int tpmi_create_devices(struct intel_tpmi_info *tpmi_info)
diff mbox series

Patch

diff --git a/drivers/platform/x86/intel/tpmi.c b/drivers/platform/x86/intel/tpmi.c
index c999732b0f1e..882fe5e4763f 100644
--- a/drivers/platform/x86/intel/tpmi.c
+++ b/drivers/platform/x86/intel/tpmi.c
@@ -215,8 +215,8 @@  static int tpmi_create_device(struct intel_tpmi_info *tpmi_info,
 
 	feature_vsec_dev = kzalloc(sizeof(*feature_vsec_dev), GFP_KERNEL);
 	if (!feature_vsec_dev) {
-		ret = -ENOMEM;
-		goto free_res;
+		kfree(res);
+		return -ENOMEM;
 	}
 
 	snprintf(feature_id_name, sizeof(feature_id_name), "tpmi-%s", name);
@@ -242,17 +242,8 @@  static int tpmi_create_device(struct intel_tpmi_info *tpmi_info,
 	 * feature_vsec_dev memory is also freed as part of device
 	 * delete.
 	 */
-	ret = intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev->auxdev.dev,
-				 feature_vsec_dev, feature_id_name);
-	if (ret)
-		goto free_res;
-
-	return 0;
-
-free_res:
-	kfree(res);
-
-	return ret;
+	return intel_vsec_add_aux(vsec_dev->pcidev, &vsec_dev->auxdev.dev,
+				  feature_vsec_dev, feature_id_name);
 }
 
 static int tpmi_create_devices(struct intel_tpmi_info *tpmi_info)