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 <[email protected]>
Reviewed-by: Hans de Goede <[email protected]>
---
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
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 <[email protected]>
> Reviewed-by: Hans de Goede <[email protected]>
Acked-by: Srinivas Pandruvada <[email protected]>
> ---
> 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);
> }
…
> 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
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
>
>> …
>>> 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
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
>>>> …
>>>>> 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
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.
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 <[email protected]>
> Reviewed-by: Hans de Goede <[email protected]>
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);
> }