| Message ID | 20240514092656.3462832-1-harshit.m.mogalapalli@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | platform/x86: ISST: fix use-after-free in tpmi_sst_dev_remove() | expand |
Hi, On 5/14/24 11:26 AM, 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> This looks obviously correct to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Still would be nice if one of the Intel maintainers for this can ack it. Either way I'll merge this for the first pdx86 fixes pull-request for 6.10 (when I get around to merging the first round of fixes). Regards, Hans > --- > 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); > }
> In tpmi_sst_dev_remove(), tpmi_sst is dereferenced after being freed. Fix this by reordering the kfree() post the dereference.
I suggest to take preferred line lengths better into account
also for such a change description.
Thus the second sentence should be put into a subsequent line.
How do you think about the following wording approach?
Move a kfree() call behind an assignment statement in the affected if branch.
Regards,
Markus
On Tue, 2024-05-14 at 02:26 -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> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > --- > 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); > }
On Fri, 2024-05-17 at 07:31 -0700, srinivas pandruvada wrote: > On Tue, 2024-05-14 at 02:26 -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> > Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > I realized the line length is 127 chars in commit description. Please fix that and run checkpatch.pl before posting again. Thanks, Srinivas > > --- > > 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); > > } >
On 17/05/24 20:03, srinivas pandruvada wrote: > On Fri, 2024-05-17 at 07:31 -0700, srinivas pandruvada wrote: >> On Tue, 2024-05-14 at 02:26 -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> >> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> >> > I realized the line length is 127 chars in commit description. > Please fix that and run checkpatch.pl before posting again. > Thanks for the ACK. Sorry I usually run checkpatch.pl, but somehow missed noticing the error. Will send it with the commit message fixup. Thanks, Harshit > Thanks, > Srinivas > >>> --- >>> 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> --- 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(-)