All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/amd/pm: fix double free in si_parse_power_table()
@ 2022-04-19 10:37 ` Keita Suzuki
  0 siblings, 0 replies; 6+ messages in thread
From: Keita Suzuki @ 2022-04-19 10:37 UTC (permalink / raw)
  Cc: keitasuzuki.park, Evan Quan, Alex Deucher, Christian König,
	Pan, Xinhui, David Airlie, Daniel Vetter, Lijo Lazar,
	Gustavo A. R. Silva, Hawking Zhang, Maíra Canal, amd-gfx,
	dri-devel, linux-kernel

In function si_parse_power_table(), array adev->pm.dpm.ps and its member
is allocated. If the allocation of each member fails, the array itself
is freed and returned with an error code. However, the array is later
freed again in si_dpm_fini() function which is called when the function
returns an error.

This leads to potential double free of the array adev->pm.dpm.ps, as
well as leak of its array members, since the members are not freed in
the allocation function and the array is not nulled when freed.
In addition adev->pm.dpm.num_ps, which keeps track of the allocated
array member, is not updated until the member allocation is
successfully finished, this could also lead to either use after free,
or uninitialized variable access in si_dpm_fini().

Fix this by postponing the free of the array until si_dpm_fini() and
increment adev->pm.dpm.num_ps everytime the array member is allocated.

Signed-off-by: Keita Suzuki <keitasuzuki.park@sslab.ics.keio.ac.jp>
---
 drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
index caae54487f9c..079888229485 100644
--- a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
+++ b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
@@ -7331,17 +7331,15 @@ static int si_parse_power_table(struct amdgpu_device *adev)
 	if (!adev->pm.dpm.ps)
 		return -ENOMEM;
 	power_state_offset = (u8 *)state_array->states;
-	for (i = 0; i < state_array->ucNumEntries; i++) {
+	for (adev->pm.dpm.num_ps = 0, i = 0; i < state_array->ucNumEntries; i++) {
 		u8 *idx;
 		power_state = (union pplib_power_state *)power_state_offset;
 		non_clock_array_index = power_state->v2.nonClockInfoIndex;
 		non_clock_info = (struct _ATOM_PPLIB_NONCLOCK_INFO *)
 			&non_clock_info_array->nonClockInfo[non_clock_array_index];
 		ps = kzalloc(sizeof(struct  si_ps), GFP_KERNEL);
-		if (ps == NULL) {
-			kfree(adev->pm.dpm.ps);
+		if (ps == NULL)
 			return -ENOMEM;
-		}
 		adev->pm.dpm.ps[i].ps_priv = ps;
 		si_parse_pplib_non_clock_info(adev, &adev->pm.dpm.ps[i],
 					      non_clock_info,
@@ -7363,8 +7361,8 @@ static int si_parse_power_table(struct amdgpu_device *adev)
 			k++;
 		}
 		power_state_offset += 2 + power_state->v2.ucNumDPMLevels;
+		adev->pm.dpm.num_ps++;
 	}
-	adev->pm.dpm.num_ps = state_array->ucNumEntries;
 
 	/* fill in the vce power states */
 	for (i = 0; i < adev->pm.dpm.num_of_vce_states; i++) {
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH] drm/amd/pm: fix double free in si_parse_power_table()
@ 2022-04-19 10:37 ` Keita Suzuki
  0 siblings, 0 replies; 6+ messages in thread
From: Keita Suzuki @ 2022-04-19 10:37 UTC (permalink / raw)
  Cc: Lijo Lazar, David Airlie, Maíra Canal, Pan, Xinhui,
	Gustavo A. R. Silva, amd-gfx, linux-kernel, dri-devel,
	Daniel Vetter, Alex Deucher, Evan Quan, Christian König,
	keitasuzuki.park, Hawking Zhang

In function si_parse_power_table(), array adev->pm.dpm.ps and its member
is allocated. If the allocation of each member fails, the array itself
is freed and returned with an error code. However, the array is later
freed again in si_dpm_fini() function which is called when the function
returns an error.

This leads to potential double free of the array adev->pm.dpm.ps, as
well as leak of its array members, since the members are not freed in
the allocation function and the array is not nulled when freed.
In addition adev->pm.dpm.num_ps, which keeps track of the allocated
array member, is not updated until the member allocation is
successfully finished, this could also lead to either use after free,
or uninitialized variable access in si_dpm_fini().

Fix this by postponing the free of the array until si_dpm_fini() and
increment adev->pm.dpm.num_ps everytime the array member is allocated.

Signed-off-by: Keita Suzuki <keitasuzuki.park@sslab.ics.keio.ac.jp>
---
 drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
index caae54487f9c..079888229485 100644
--- a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
+++ b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
@@ -7331,17 +7331,15 @@ static int si_parse_power_table(struct amdgpu_device *adev)
 	if (!adev->pm.dpm.ps)
 		return -ENOMEM;
 	power_state_offset = (u8 *)state_array->states;
-	for (i = 0; i < state_array->ucNumEntries; i++) {
+	for (adev->pm.dpm.num_ps = 0, i = 0; i < state_array->ucNumEntries; i++) {
 		u8 *idx;
 		power_state = (union pplib_power_state *)power_state_offset;
 		non_clock_array_index = power_state->v2.nonClockInfoIndex;
 		non_clock_info = (struct _ATOM_PPLIB_NONCLOCK_INFO *)
 			&non_clock_info_array->nonClockInfo[non_clock_array_index];
 		ps = kzalloc(sizeof(struct  si_ps), GFP_KERNEL);
-		if (ps == NULL) {
-			kfree(adev->pm.dpm.ps);
+		if (ps == NULL)
 			return -ENOMEM;
-		}
 		adev->pm.dpm.ps[i].ps_priv = ps;
 		si_parse_pplib_non_clock_info(adev, &adev->pm.dpm.ps[i],
 					      non_clock_info,
@@ -7363,8 +7361,8 @@ static int si_parse_power_table(struct amdgpu_device *adev)
 			k++;
 		}
 		power_state_offset += 2 + power_state->v2.ucNumDPMLevels;
+		adev->pm.dpm.num_ps++;
 	}
-	adev->pm.dpm.num_ps = state_array->ucNumEntries;
 
 	/* fill in the vce power states */
 	for (i = 0; i < adev->pm.dpm.num_of_vce_states; i++) {
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH] drm/amd/pm: fix double free in si_parse_power_table()
@ 2022-04-19 10:37 ` Keita Suzuki
  0 siblings, 0 replies; 6+ messages in thread
From: Keita Suzuki @ 2022-04-19 10:37 UTC (permalink / raw)
  Cc: Lijo Lazar, David Airlie, Maíra Canal, Pan, Xinhui,
	Gustavo A. R. Silva, amd-gfx, linux-kernel, dri-devel,
	Alex Deucher, Evan Quan, Christian König, keitasuzuki.park,
	Hawking Zhang

In function si_parse_power_table(), array adev->pm.dpm.ps and its member
is allocated. If the allocation of each member fails, the array itself
is freed and returned with an error code. However, the array is later
freed again in si_dpm_fini() function which is called when the function
returns an error.

This leads to potential double free of the array adev->pm.dpm.ps, as
well as leak of its array members, since the members are not freed in
the allocation function and the array is not nulled when freed.
In addition adev->pm.dpm.num_ps, which keeps track of the allocated
array member, is not updated until the member allocation is
successfully finished, this could also lead to either use after free,
or uninitialized variable access in si_dpm_fini().

Fix this by postponing the free of the array until si_dpm_fini() and
increment adev->pm.dpm.num_ps everytime the array member is allocated.

Signed-off-by: Keita Suzuki <keitasuzuki.park@sslab.ics.keio.ac.jp>
---
 drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
index caae54487f9c..079888229485 100644
--- a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
+++ b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
@@ -7331,17 +7331,15 @@ static int si_parse_power_table(struct amdgpu_device *adev)
 	if (!adev->pm.dpm.ps)
 		return -ENOMEM;
 	power_state_offset = (u8 *)state_array->states;
-	for (i = 0; i < state_array->ucNumEntries; i++) {
+	for (adev->pm.dpm.num_ps = 0, i = 0; i < state_array->ucNumEntries; i++) {
 		u8 *idx;
 		power_state = (union pplib_power_state *)power_state_offset;
 		non_clock_array_index = power_state->v2.nonClockInfoIndex;
 		non_clock_info = (struct _ATOM_PPLIB_NONCLOCK_INFO *)
 			&non_clock_info_array->nonClockInfo[non_clock_array_index];
 		ps = kzalloc(sizeof(struct  si_ps), GFP_KERNEL);
-		if (ps == NULL) {
-			kfree(adev->pm.dpm.ps);
+		if (ps == NULL)
 			return -ENOMEM;
-		}
 		adev->pm.dpm.ps[i].ps_priv = ps;
 		si_parse_pplib_non_clock_info(adev, &adev->pm.dpm.ps[i],
 					      non_clock_info,
@@ -7363,8 +7361,8 @@ static int si_parse_power_table(struct amdgpu_device *adev)
 			k++;
 		}
 		power_state_offset += 2 + power_state->v2.ucNumDPMLevels;
+		adev->pm.dpm.num_ps++;
 	}
-	adev->pm.dpm.num_ps = state_array->ucNumEntries;
 
 	/* fill in the vce power states */
 	for (i = 0; i < adev->pm.dpm.num_of_vce_states; i++) {
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/amd/pm: fix double free in si_parse_power_table()
  2022-04-19 10:37 ` Keita Suzuki
  (?)
@ 2022-04-19 14:38   ` Alex Deucher
  -1 siblings, 0 replies; 6+ messages in thread
From: Alex Deucher @ 2022-04-19 14:38 UTC (permalink / raw)
  To: Keita Suzuki
  Cc: Lijo Lazar, David Airlie, Maíra Canal, Pan, Xinhui,
	Gustavo A. R. Silva, amd-gfx list, LKML,
	Maling list - DRI developers, Daniel Vetter, Alex Deucher,
	Evan Quan, Christian König, Hawking Zhang

Applied.  Thanks!

On Tue, Apr 19, 2022 at 8:49 AM Keita Suzuki
<keitasuzuki.park@sslab.ics.keio.ac.jp> wrote:
>
> In function si_parse_power_table(), array adev->pm.dpm.ps and its member
> is allocated. If the allocation of each member fails, the array itself
> is freed and returned with an error code. However, the array is later
> freed again in si_dpm_fini() function which is called when the function
> returns an error.
>
> This leads to potential double free of the array adev->pm.dpm.ps, as
> well as leak of its array members, since the members are not freed in
> the allocation function and the array is not nulled when freed.
> In addition adev->pm.dpm.num_ps, which keeps track of the allocated
> array member, is not updated until the member allocation is
> successfully finished, this could also lead to either use after free,
> or uninitialized variable access in si_dpm_fini().
>
> Fix this by postponing the free of the array until si_dpm_fini() and
> increment adev->pm.dpm.num_ps everytime the array member is allocated.
>
> Signed-off-by: Keita Suzuki <keitasuzuki.park@sslab.ics.keio.ac.jp>
> ---
>  drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
> index caae54487f9c..079888229485 100644
> --- a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
> +++ b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
> @@ -7331,17 +7331,15 @@ static int si_parse_power_table(struct amdgpu_device *adev)
>         if (!adev->pm.dpm.ps)
>                 return -ENOMEM;
>         power_state_offset = (u8 *)state_array->states;
> -       for (i = 0; i < state_array->ucNumEntries; i++) {
> +       for (adev->pm.dpm.num_ps = 0, i = 0; i < state_array->ucNumEntries; i++) {
>                 u8 *idx;
>                 power_state = (union pplib_power_state *)power_state_offset;
>                 non_clock_array_index = power_state->v2.nonClockInfoIndex;
>                 non_clock_info = (struct _ATOM_PPLIB_NONCLOCK_INFO *)
>                         &non_clock_info_array->nonClockInfo[non_clock_array_index];
>                 ps = kzalloc(sizeof(struct  si_ps), GFP_KERNEL);
> -               if (ps == NULL) {
> -                       kfree(adev->pm.dpm.ps);
> +               if (ps == NULL)
>                         return -ENOMEM;
> -               }
>                 adev->pm.dpm.ps[i].ps_priv = ps;
>                 si_parse_pplib_non_clock_info(adev, &adev->pm.dpm.ps[i],
>                                               non_clock_info,
> @@ -7363,8 +7361,8 @@ static int si_parse_power_table(struct amdgpu_device *adev)
>                         k++;
>                 }
>                 power_state_offset += 2 + power_state->v2.ucNumDPMLevels;
> +               adev->pm.dpm.num_ps++;
>         }
> -       adev->pm.dpm.num_ps = state_array->ucNumEntries;
>
>         /* fill in the vce power states */
>         for (i = 0; i < adev->pm.dpm.num_of_vce_states; i++) {
> --
> 2.25.1
>

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/amd/pm: fix double free in si_parse_power_table()
@ 2022-04-19 14:38   ` Alex Deucher
  0 siblings, 0 replies; 6+ messages in thread
From: Alex Deucher @ 2022-04-19 14:38 UTC (permalink / raw)
  To: Keita Suzuki
  Cc: Pan, Xinhui, David Airlie, Maíra Canal, Lijo Lazar,
	Gustavo A. R. Silva, amd-gfx list, LKML,
	Maling list - DRI developers, Alex Deucher, Evan Quan,
	Christian König, Hawking Zhang

Applied.  Thanks!

On Tue, Apr 19, 2022 at 8:49 AM Keita Suzuki
<keitasuzuki.park@sslab.ics.keio.ac.jp> wrote:
>
> In function si_parse_power_table(), array adev->pm.dpm.ps and its member
> is allocated. If the allocation of each member fails, the array itself
> is freed and returned with an error code. However, the array is later
> freed again in si_dpm_fini() function which is called when the function
> returns an error.
>
> This leads to potential double free of the array adev->pm.dpm.ps, as
> well as leak of its array members, since the members are not freed in
> the allocation function and the array is not nulled when freed.
> In addition adev->pm.dpm.num_ps, which keeps track of the allocated
> array member, is not updated until the member allocation is
> successfully finished, this could also lead to either use after free,
> or uninitialized variable access in si_dpm_fini().
>
> Fix this by postponing the free of the array until si_dpm_fini() and
> increment adev->pm.dpm.num_ps everytime the array member is allocated.
>
> Signed-off-by: Keita Suzuki <keitasuzuki.park@sslab.ics.keio.ac.jp>
> ---
>  drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
> index caae54487f9c..079888229485 100644
> --- a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
> +++ b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
> @@ -7331,17 +7331,15 @@ static int si_parse_power_table(struct amdgpu_device *adev)
>         if (!adev->pm.dpm.ps)
>                 return -ENOMEM;
>         power_state_offset = (u8 *)state_array->states;
> -       for (i = 0; i < state_array->ucNumEntries; i++) {
> +       for (adev->pm.dpm.num_ps = 0, i = 0; i < state_array->ucNumEntries; i++) {
>                 u8 *idx;
>                 power_state = (union pplib_power_state *)power_state_offset;
>                 non_clock_array_index = power_state->v2.nonClockInfoIndex;
>                 non_clock_info = (struct _ATOM_PPLIB_NONCLOCK_INFO *)
>                         &non_clock_info_array->nonClockInfo[non_clock_array_index];
>                 ps = kzalloc(sizeof(struct  si_ps), GFP_KERNEL);
> -               if (ps == NULL) {
> -                       kfree(adev->pm.dpm.ps);
> +               if (ps == NULL)
>                         return -ENOMEM;
> -               }
>                 adev->pm.dpm.ps[i].ps_priv = ps;
>                 si_parse_pplib_non_clock_info(adev, &adev->pm.dpm.ps[i],
>                                               non_clock_info,
> @@ -7363,8 +7361,8 @@ static int si_parse_power_table(struct amdgpu_device *adev)
>                         k++;
>                 }
>                 power_state_offset += 2 + power_state->v2.ucNumDPMLevels;
> +               adev->pm.dpm.num_ps++;
>         }
> -       adev->pm.dpm.num_ps = state_array->ucNumEntries;
>
>         /* fill in the vce power states */
>         for (i = 0; i < adev->pm.dpm.num_of_vce_states; i++) {
> --
> 2.25.1
>

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/amd/pm: fix double free in si_parse_power_table()
@ 2022-04-19 14:38   ` Alex Deucher
  0 siblings, 0 replies; 6+ messages in thread
From: Alex Deucher @ 2022-04-19 14:38 UTC (permalink / raw)
  To: Keita Suzuki
  Cc: Pan, Xinhui, David Airlie, Maíra Canal, Lijo Lazar,
	Gustavo A. R. Silva, amd-gfx list, LKML,
	Maling list - DRI developers, Daniel Vetter, Alex Deucher,
	Evan Quan, Christian König, Hawking Zhang

Applied.  Thanks!

On Tue, Apr 19, 2022 at 8:49 AM Keita Suzuki
<keitasuzuki.park@sslab.ics.keio.ac.jp> wrote:
>
> In function si_parse_power_table(), array adev->pm.dpm.ps and its member
> is allocated. If the allocation of each member fails, the array itself
> is freed and returned with an error code. However, the array is later
> freed again in si_dpm_fini() function which is called when the function
> returns an error.
>
> This leads to potential double free of the array adev->pm.dpm.ps, as
> well as leak of its array members, since the members are not freed in
> the allocation function and the array is not nulled when freed.
> In addition adev->pm.dpm.num_ps, which keeps track of the allocated
> array member, is not updated until the member allocation is
> successfully finished, this could also lead to either use after free,
> or uninitialized variable access in si_dpm_fini().
>
> Fix this by postponing the free of the array until si_dpm_fini() and
> increment adev->pm.dpm.num_ps everytime the array member is allocated.
>
> Signed-off-by: Keita Suzuki <keitasuzuki.park@sslab.ics.keio.ac.jp>
> ---
>  drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
> index caae54487f9c..079888229485 100644
> --- a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
> +++ b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
> @@ -7331,17 +7331,15 @@ static int si_parse_power_table(struct amdgpu_device *adev)
>         if (!adev->pm.dpm.ps)
>                 return -ENOMEM;
>         power_state_offset = (u8 *)state_array->states;
> -       for (i = 0; i < state_array->ucNumEntries; i++) {
> +       for (adev->pm.dpm.num_ps = 0, i = 0; i < state_array->ucNumEntries; i++) {
>                 u8 *idx;
>                 power_state = (union pplib_power_state *)power_state_offset;
>                 non_clock_array_index = power_state->v2.nonClockInfoIndex;
>                 non_clock_info = (struct _ATOM_PPLIB_NONCLOCK_INFO *)
>                         &non_clock_info_array->nonClockInfo[non_clock_array_index];
>                 ps = kzalloc(sizeof(struct  si_ps), GFP_KERNEL);
> -               if (ps == NULL) {
> -                       kfree(adev->pm.dpm.ps);
> +               if (ps == NULL)
>                         return -ENOMEM;
> -               }
>                 adev->pm.dpm.ps[i].ps_priv = ps;
>                 si_parse_pplib_non_clock_info(adev, &adev->pm.dpm.ps[i],
>                                               non_clock_info,
> @@ -7363,8 +7361,8 @@ static int si_parse_power_table(struct amdgpu_device *adev)
>                         k++;
>                 }
>                 power_state_offset += 2 + power_state->v2.ucNumDPMLevels;
> +               adev->pm.dpm.num_ps++;
>         }
> -       adev->pm.dpm.num_ps = state_array->ucNumEntries;
>
>         /* fill in the vce power states */
>         for (i = 0; i < adev->pm.dpm.num_of_vce_states; i++) {
> --
> 2.25.1
>

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-04-19 16:58 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-19 10:37 [PATCH] drm/amd/pm: fix double free in si_parse_power_table() Keita Suzuki
2022-04-19 10:37 ` Keita Suzuki
2022-04-19 10:37 ` Keita Suzuki
2022-04-19 14:38 ` Alex Deucher
2022-04-19 14:38   ` Alex Deucher
2022-04-19 14:38   ` Alex Deucher

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.