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

[Dongliang Mu <dzm91@hust.edu.cn>]

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)