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 <[email protected]>
---
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)
--
2.39.2
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 <[email protected]>
> ---
Yeah. These patches are right. The earlier fix still has a double
free. Devres stuff is confusing...
Reviewed-by: Dan Carpenter <[email protected]>
regards,
dan carpenter
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 <[email protected]>
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)
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 <[email protected]>
Acked-by: Srinivas Pandruvada <[email protected]>
>
> 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)
>
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 <[email protected]>
> Acked-by: Srinivas Pandruvada <[email protected]>
>
>> 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)
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 <[email protected]>
>> Acked-by: Srinivas Pandruvada <[email protected]>
>>
>>> 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)
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)
>
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)
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)
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)
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)
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 <[email protected]>
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)