| Message ID | 20240517144946.289615-1-harshit.m.mogalapalli@oracle.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Hans de Goede |
| Headers | show |
| Series | [v2] platform/x86: ISST: fix use-after-free in tpmi_sst_dev_remove() | expand |
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); > }
… > 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.
>> … >>> 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.
>>>>>> … >>>>>>> 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
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.
>>> 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
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); > }
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); }
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(-)