[PATCH v2] platform/x86: ISST: fix use-after-free in tpmi_sst_dev_remove()

[PATCH v2] platform/x86: ISST: fix use-after-free in tpmi_sst_dev_remove()

[Harshit Mogalapalli]

In tpmi_sst_dev_remove(), tpmi_sst is dereferenced after being freed.
Fix this by reordering the kfree() post the dereference.

Fixes: 9d1d36268f3d ("platform/x86: ISST: Support partitioned systems")
Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---
v1->v2: Add R.B from Hans and fix commit message wrapping to 75 chars.
This is found by smatch and only compile tested.
---
 drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c b/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
index 7bac7841ff0a..7fa360073f6e 100644
--- a/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
+++ b/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
@@ -1610,8 +1610,8 @@ void tpmi_sst_dev_remove(struct auxiliary_device *auxdev)
 	tpmi_sst->partition_mask_current &= ~BIT(plat_info->partition);
 	/* Free the package instance when the all partitions are removed */
 	if (!tpmi_sst->partition_mask_current) {
-		kfree(tpmi_sst);
 		isst_common.sst_inst[tpmi_sst->package_id] = NULL;
+		kfree(tpmi_sst);
 	}
 	mutex_unlock(&isst_tpmi_dev_lock);
 }
-- 
2.39.3

Re: [PATCH v2] platform/x86: ISST: fix use-after-free in tpmi_sst_dev_remove()

[srinivas pandruvada]

On Fri, 2024-05-17 at 07:49 -0700, Harshit Mogalapalli wrote:
> In tpmi_sst_dev_remove(), tpmi_sst is dereferenced after being freed.
> Fix this by reordering the kfree() post the dereference.
> 
> Fixes: 9d1d36268f3d ("platform/x86: ISST: Support partitioned
> systems")
> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

> ---
> v1->v2: Add R.B from Hans and fix commit message wrapping to 75
> chars.
> This is found by smatch and only compile tested.
> ---
>  drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git
> a/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
> b/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
> index 7bac7841ff0a..7fa360073f6e 100644
> --- a/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
> +++ b/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
> @@ -1610,8 +1610,8 @@ void tpmi_sst_dev_remove(struct
> auxiliary_device *auxdev)
>         tpmi_sst->partition_mask_current &= ~BIT(plat_info-
> >partition);
>         /* Free the package instance when the all partitions are
> removed */
>         if (!tpmi_sst->partition_mask_current) {
> -               kfree(tpmi_sst);
>                 isst_common.sst_inst[tpmi_sst->package_id] = NULL;
> +               kfree(tpmi_sst);
>         }
>         mutex_unlock(&isst_tpmi_dev_lock);
>  }

Re: [PATCH v2] platform/x86: ISST: fix use-after-free in tpmi_sst_dev_remove()

[Markus Elfring]

…
> Fix this by reordering the kfree() post the dereference.

Would a wording approach (like the following) be a bit nicer?

   Move a kfree() call behind an assignment statement in the affected if branch.


…
> ---
> v1->v2: Add R.B from Hans and fix commit message wrapping to 75 chars.
> This is found by smatch and only compile tested.

* Can it occasionally be nicer to use an enumeration also for
  version descriptions?

* Is it helpful to separate additional comments by blank lines?


> ---
>  drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c | 2 +-

How do you think about to omit a repeated marker line here?

Regards,
Markus

Re: [PATCH v2] platform/x86: ISST: fix use-after-free in tpmi_sst_dev_remove()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 988 bytes --]

On Sat, 18 May 2024, Markus Elfring wrote:

> …
> > Fix this by reordering the kfree() post the dereference.
> 
> Would a wording approach (like the following) be a bit nicer?
> 
>    Move a kfree() call behind an assignment statement in the affected if branch.

No, the suggested wording would make it less precise ("post the 
dereference" -> "behind an assignment") and also tries to tell pointless 
things about the location in the codei that is visible in the patch itself.

-- 
 i.

> > v1->v2: Add R.B from Hans and fix commit message wrapping to 75 chars.
> > This is found by smatch and only compile tested.
> 
> * Can it occasionally be nicer to use an enumeration also for
>   version descriptions?
> 
> * Is it helpful to separate additional comments by blank lines?
> 
> 
> > ---
> >  drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c | 2 +-
> 
> How do you think about to omit a repeated marker line here?
> 
> Regards,
> Markus
> 

Re: [v2] platform/x86: ISST: fix use-after-free in tpmi_sst_dev_remove()

[Markus Elfring]

>> …
>>> Fix this by reordering the kfree() post the dereference.
>>
>> Would a wording approach (like the following) be a bit nicer?
>>
>>    Move a kfree() call behind an assignment statement in the affected if branch.
>
> No, the suggested wording would make it less precise ("post the
> dereference" -> "behind an assignment") and also tries to tell pointless
> things about the location in the codei that is visible in the patch itself.

Would you eventually like another wording variant a bit more?

    Thus move a kfree() call behind a dereference of an invalid pointer.


Regards,
Markus

Re: [v2] platform/x86: ISST: fix use-after-free in tpmi_sst_dev_remove()

[Hans de Goede]

Hi Markus,

On 5/20/24 12:56 PM, Markus Elfring wrote:
>>> …
>>>> Fix this by reordering the kfree() post the dereference.
>>>
>>> Would a wording approach (like the following) be a bit nicer?
>>>
>>>    Move a kfree() call behind an assignment statement in the affected if branch.
>>
>> No, the suggested wording would make it less precise ("post the
>> dereference" -> "behind an assignment") and also tries to tell pointless
>> things about the location in the codei that is visible in the patch itself.
> 
> Would you eventually like another wording variant a bit more?
> 
>     Thus move a kfree() call behind a dereference of an invalid pointer.

The original wording of the commit message really is fine as is,
I see no need for Harshit to send a new version and I plan to
merge this as is.

Regards,

Hans

Re: [v2] platform/x86: ISST: fix use-after-free in tpmi_sst_dev_remove()

[Markus Elfring]

>>>> …
>>>>> Fix this by reordering the kfree() post the dereference.
…
> The original wording of the commit message really is fine as is,
> I see no need for Harshit to send a new version and I plan to
> merge this as is.

Are there opportunities remaining to improve the discussed wording?

1. https://en.wiktionary.org/wiki/post#Etymology_1

2. https://en.wiktionary.org/wiki/reorder

3. Function call indication?
   https://elixir.bootlin.com/linux/v6.9.1/source/mm/slub.c#L4371

4. Rephrasing of “Fix this by …”?

5. https://en.wikipedia.org/wiki/Dangling_pointer#Cause_of_dangling_pointers

6. https://wiki.sei.cmu.edu/confluence/display/c/MEM30-C.+Do+not+access+freed+memory#MEM30C.Donotaccessfreedmemory-AutomatedDetection

7. https://en.wikipedia.org/wiki/Code_sanitizer#KernelAddressSanitizer


Regards,
Markus

Re: [v2] platform/x86: ISST: fix use-after-free in tpmi_sst_dev_remove()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1076 bytes --]

On Tue, 21 May 2024, Markus Elfring wrote:

> >>>> …
> >>>>> Fix this by reordering the kfree() post the dereference.
> …
> > The original wording of the commit message really is fine as is,
> > I see no need for Harshit to send a new version and I plan to
> > merge this as is.
> 
> Are there opportunities remaining to improve the discussed wording?
> 
> 1. https://en.wiktionary.org/wiki/post#Etymology_1
> 
> 2. https://en.wiktionary.org/wiki/reorder
> 
> 3. Function call indication?
>    https://elixir.bootlin.com/linux/v6.9.1/source/mm/slub.c#L4371
> 
> 4. Rephrasing of “Fix this by …”?
> 
> 5. https://en.wikipedia.org/wiki/Dangling_pointer#Cause_of_dangling_pointers
> 
> 6. https://wiki.sei.cmu.edu/confluence/display/c/MEM30-C.+Do+not+access+freed+memory#MEM30C.Donotaccessfreedmemory-AutomatedDetection
> 
> 7. https://en.wikipedia.org/wiki/Code_sanitizer#KernelAddressSanitizer

We'll not waste our time in wordsmithing commit messages to perfection, 
the current one is good enough as was stated to you already.

-- 
 i.

Re: [v2] platform/x86: ISST: fix use-after-free in tpmi_sst_dev_remove()

[Markus Elfring]

>>>>>> …
>>>>>>> Fix this by reordering the kfree() post the dereference.
…
> We'll not waste our time in wordsmithing commit messages to perfection,

I hope that you would also like to avoid typos in change descriptions.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9#n45


> the current one is good enough as was stated to you already.

I dared to present some wording concerns according to the running patch review.

Regards,
Markus

Re: [v2] platform/x86: ISST: fix use-after-free in tpmi_sst_dev_remove()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 865 bytes --]

On Tue, 21 May 2024, Markus Elfring wrote:

> >>>>>> …
> >>>>>>> Fix this by reordering the kfree() post the dereference.
> …
> > We'll not waste our time in wordsmithing commit messages to perfection,
> 
> I hope that you would also like to avoid typos in change descriptions.

Now you start derailing this with off-topic references to hypothetical 
"typos".

> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9#n45
> 
> 
> > the current one is good enough as was stated to you already.
> 
> I dared to present some wording concerns according to the running patch review.

No, your latest replies were not anymore about wording concerns but 
complaining about process.

Your wording concerns were already replied but after that you kept on 
arguing.

-- 
 i.

Re: [v2] platform/x86: ISST: fix use-after-free in tpmi_sst_dev_remove()

[Markus Elfring]

>>> We'll not waste our time in wordsmithing commit messages to perfection,
>>
>> I hope that you would also like to avoid typos in change descriptions.
>
> Now you start derailing this with off-topic references to hypothetical "typos".

Which alternative term would you prefer for the word combination “the kfree() post”
(for example) instead?

Regards,
Markus

Re: [PATCH v2] platform/x86: ISST: fix use-after-free in tpmi_sst_dev_remove()

[Hans de Goede]

Hi,

On 5/17/24 4:49 PM, Harshit Mogalapalli wrote:
> In tpmi_sst_dev_remove(), tpmi_sst is dereferenced after being freed.
> Fix this by reordering the kfree() post the dereference.
> 
> Fixes: 9d1d36268f3d ("platform/x86: ISST: Support partitioned systems")
> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>

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.

I will include this patch in my next fixes pull-req to Linus
for the current kernel development cycle.

Regards,

Hans




> ---
> v1->v2: Add R.B from Hans and fix commit message wrapping to 75 chars.
> This is found by smatch and only compile tested.
> ---
>  drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c b/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
> index 7bac7841ff0a..7fa360073f6e 100644
> --- a/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
> +++ b/drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
> @@ -1610,8 +1610,8 @@ void tpmi_sst_dev_remove(struct auxiliary_device *auxdev)
>  	tpmi_sst->partition_mask_current &= ~BIT(plat_info->partition);
>  	/* Free the package instance when the all partitions are removed */
>  	if (!tpmi_sst->partition_mask_current) {
> -		kfree(tpmi_sst);
>  		isst_common.sst_inst[tpmi_sst->package_id] = NULL;
> +		kfree(tpmi_sst);
>  	}
>  	mutex_unlock(&isst_tpmi_dev_lock);
>  }