All of lore.kernel.org
 help / color / mirror / Atom feed
* [v1 0/4] Fix a buffer overflow in drm/amdgpu/smu
@ 2025-02-07  6:44 Jiang Liu
  2025-02-07  6:44 ` [v1 1/4] drm/amdgpu: avoid buffer overflow attach in smu_sys_set_pp_table() Jiang Liu
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Jiang Liu @ 2025-02-07  6:44 UTC (permalink / raw)
  To: alexander.deucher, christian.koenig, airlied, simona,
	sunil.khatri, lijo.lazar, Hawking.Zhang, mario.limonciello,
	xiaogang.chen, Kent.Russell, shuox.liu, amd-gfx
  Cc: Jiang Liu

Fix several bugs in smu subsystem:
1) a buffer overflow bug in function smu_sys_set_pp_table()
2) tune logic of is_vcn_enabled()
3) enhance handling of gfx_off_entrycount in function smu_suspend()

Jiang Liu (4):
  drm/amdgpu: avoid buffer overflow attach in smu_sys_set_pp_table()
  drm/amdgpu: accumulate gfx_off_entrycount in smu_suspend()
  drm/amdgpu: treat VCN as enabled if either VCN or JPEC is enabled
  drm/amdgpu: minor code style enhancement for smu

 drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c       | 17 +++++++++--------
 .../drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c    |  2 +-
 2 files changed, 10 insertions(+), 9 deletions(-)

-- 
2.43.5


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [v1 1/4] drm/amdgpu: avoid buffer overflow attach in smu_sys_set_pp_table()
  2025-02-07  6:44 [v1 0/4] Fix a buffer overflow in drm/amdgpu/smu Jiang Liu
@ 2025-02-07  6:44 ` Jiang Liu
  2025-02-07  7:56   ` Lazar, Lijo
  2025-02-07  6:44 ` [v1 2/4] drm/amdgpu: accumulate gfx_off_entrycount in smu_suspend() Jiang Liu
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 12+ messages in thread
From: Jiang Liu @ 2025-02-07  6:44 UTC (permalink / raw)
  To: alexander.deucher, christian.koenig, airlied, simona,
	sunil.khatri, lijo.lazar, Hawking.Zhang, mario.limonciello,
	xiaogang.chen, Kent.Russell, shuox.liu, amd-gfx
  Cc: Jiang Liu

It malicious user provides a small pptable through sysfs and then
a bigger pptable, it may cause buffer overflow attack in function
smu_sys_set_pp_table().

Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>
---
 drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
index 8ca793c222ff..ed9dac00ebfb 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
@@ -612,7 +612,8 @@ static int smu_sys_set_pp_table(void *handle,
 		return -EIO;
 	}
 
-	if (!smu_table->hardcode_pptable) {
+	if (!smu_table->hardcode_pptable || smu_table->power_play_table_size < size) {
+		kfree(smu_table->hardcode_pptable);
 		smu_table->hardcode_pptable = kzalloc(size, GFP_KERNEL);
 		if (!smu_table->hardcode_pptable)
 			return -ENOMEM;
-- 
2.43.5


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [v1 2/4] drm/amdgpu: accumulate gfx_off_entrycount in smu_suspend()
  2025-02-07  6:44 [v1 0/4] Fix a buffer overflow in drm/amdgpu/smu Jiang Liu
  2025-02-07  6:44 ` [v1 1/4] drm/amdgpu: avoid buffer overflow attach in smu_sys_set_pp_table() Jiang Liu
@ 2025-02-07  6:44 ` Jiang Liu
  2025-02-07  8:04   ` Lazar, Lijo
  2025-02-07  6:44 ` [v1 3/4] drm/amdgpu: treat VCN as enabled if either VCN or JPEC is enabled Jiang Liu
  2025-02-07  6:44 ` [v1 4/4] drm/amdgpu: minor code style enhancement for smu Jiang Liu
  3 siblings, 1 reply; 12+ messages in thread
From: Jiang Liu @ 2025-02-07  6:44 UTC (permalink / raw)
  To: alexander.deucher, christian.koenig, airlied, simona,
	sunil.khatri, lijo.lazar, Hawking.Zhang, mario.limonciello,
	xiaogang.chen, Kent.Russell, shuox.liu, amd-gfx
  Cc: Jiang Liu

As pwfw resets entrycount when device is suspended, so we should
accmulate the gfx_off_entrycount value instead of save the last value
of it.

Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>
---
 drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
index ed9dac00ebfb..70a5ab649e5f 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
@@ -2134,12 +2134,12 @@ static int smu_suspend(struct amdgpu_ip_block *ip_block)
 	smu_set_gfx_cgpg(smu, false);
 
 	/*
-	 * pwfw resets entrycount when device is suspended, so we save the
-	 * last value to be used when we resume to keep it consistent
+	 * pwfw resets entrycount when device is suspended, so we accumulate
+	 * the `gfx_off_entrycount` value.
 	 */
 	ret = smu_get_entrycount_gfxoff(smu, &count);
 	if (!ret)
-		adev->gfx.gfx_off_entrycount = count;
+		adev->gfx.gfx_off_entrycount += count;
 
 	/* clear this on suspend so it will get reprogrammed on resume */
 	smu->workload_mask = 0;
-- 
2.43.5


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [v1 3/4] drm/amdgpu: treat VCN as enabled if either VCN or JPEC is enabled
  2025-02-07  6:44 [v1 0/4] Fix a buffer overflow in drm/amdgpu/smu Jiang Liu
  2025-02-07  6:44 ` [v1 1/4] drm/amdgpu: avoid buffer overflow attach in smu_sys_set_pp_table() Jiang Liu
  2025-02-07  6:44 ` [v1 2/4] drm/amdgpu: accumulate gfx_off_entrycount in smu_suspend() Jiang Liu
@ 2025-02-07  6:44 ` Jiang Liu
  2025-02-07  6:44 ` [v1 4/4] drm/amdgpu: minor code style enhancement for smu Jiang Liu
  3 siblings, 0 replies; 12+ messages in thread
From: Jiang Liu @ 2025-02-07  6:44 UTC (permalink / raw)
  To: alexander.deucher, christian.koenig, airlied, simona,
	sunil.khatri, lijo.lazar, Hawking.Zhang, mario.limonciello,
	xiaogang.chen, Kent.Russell, shuox.liu, amd-gfx
  Cc: Jiang Liu

Function is_vcn_enabled() returns false if either the VCN or JPEG ip
block is disabled, which sounds unreasonable. It should returns true
when either VCN and JPEG is enabled.

Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>
---
 drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
index 70a5ab649e5f..08b42c7a4ad1 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
@@ -230,11 +230,11 @@ static bool is_vcn_enabled(struct amdgpu_device *adev)
 	for (i = 0; i < adev->num_ip_blocks; i++) {
 		if ((adev->ip_blocks[i].version->type == AMD_IP_BLOCK_TYPE_VCN ||
 			adev->ip_blocks[i].version->type == AMD_IP_BLOCK_TYPE_JPEG) &&
-			!adev->ip_blocks[i].status.valid)
-			return false;
+			adev->ip_blocks[i].status.valid)
+			return true;
 	}
 
-	return true;
+	return false;
 }
 
 static int smu_dpm_set_vcn_enable(struct smu_context *smu,
-- 
2.43.5


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [v1 4/4] drm/amdgpu: minor code style enhancement for smu
  2025-02-07  6:44 [v1 0/4] Fix a buffer overflow in drm/amdgpu/smu Jiang Liu
                   ` (2 preceding siblings ...)
  2025-02-07  6:44 ` [v1 3/4] drm/amdgpu: treat VCN as enabled if either VCN or JPEC is enabled Jiang Liu
@ 2025-02-07  6:44 ` Jiang Liu
  2025-02-07 10:37   ` Lazar, Lijo
  3 siblings, 1 reply; 12+ messages in thread
From: Jiang Liu @ 2025-02-07  6:44 UTC (permalink / raw)
  To: alexander.deucher, christian.koenig, airlied, simona,
	sunil.khatri, lijo.lazar, Hawking.Zhang, mario.limonciello,
	xiaogang.chen, Kent.Russell, shuox.liu, amd-gfx
  Cc: Jiang Liu

Minor code style enhancement for smu.

Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>
---
 drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c            | 2 +-
 drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
index 08b42c7a4ad1..51e7bd4d3444 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
@@ -2153,7 +2153,7 @@ static int smu_resume(struct amdgpu_ip_block *ip_block)
 	struct amdgpu_device *adev = ip_block->adev;
 	struct smu_context *smu = adev->powerplay.pp_handle;
 
-	if (amdgpu_sriov_vf(adev)&& !amdgpu_sriov_is_pp_one_vf(adev))
+	if (amdgpu_sriov_vf(adev) && !amdgpu_sriov_is_pp_one_vf(adev))
 		return 0;
 
 	if (!smu->pm_enabled)
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c
index da7bd9227afe..da55d66099fb 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c
@@ -3524,8 +3524,8 @@ static const struct pptable_funcs smu_v13_0_6_ppt_funcs = {
 	.init_power = smu_v13_0_init_power,
 	.fini_power = smu_v13_0_fini_power,
 	.check_fw_status = smu_v13_0_6_check_fw_status,
-	/* pptable related */
 	.check_fw_version = smu_v13_0_6_check_fw_version,
+	/* pptable related */
 	.set_driver_table_location = smu_v13_0_set_driver_table_location,
 	.set_tool_table_location = smu_v13_0_set_tool_table_location,
 	.notify_memory_pool_location = smu_v13_0_notify_memory_pool_location,
-- 
2.43.5


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [v1 1/4] drm/amdgpu: avoid buffer overflow attach in smu_sys_set_pp_table()
  2025-02-07  6:44 ` [v1 1/4] drm/amdgpu: avoid buffer overflow attach in smu_sys_set_pp_table() Jiang Liu
@ 2025-02-07  7:56   ` Lazar, Lijo
  2025-02-07 18:17     ` Alex Deucher
  0 siblings, 1 reply; 12+ messages in thread
From: Lazar, Lijo @ 2025-02-07  7:56 UTC (permalink / raw)
  To: Jiang Liu, alexander.deucher, christian.koenig, airlied, simona,
	sunil.khatri, Hawking.Zhang, mario.limonciello, xiaogang.chen,
	Kent.Russell, shuox.liu, amd-gfx



On 2/7/2025 12:14 PM, Jiang Liu wrote:
> It malicious user provides a small pptable through sysfs and then
> a bigger pptable, it may cause buffer overflow attack in function
> smu_sys_set_pp_table().
> 
> Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>

	Reviewed-by: Lijo Lazar <lijo.lazar@amd.com>

Thanks,
Lijo

> ---
>  drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> index 8ca793c222ff..ed9dac00ebfb 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> @@ -612,7 +612,8 @@ static int smu_sys_set_pp_table(void *handle,
>  		return -EIO;
>  	}
>  
> -	if (!smu_table->hardcode_pptable) {
> +	if (!smu_table->hardcode_pptable || smu_table->power_play_table_size < size) {
> +		kfree(smu_table->hardcode_pptable);
>  		smu_table->hardcode_pptable = kzalloc(size, GFP_KERNEL);
>  		if (!smu_table->hardcode_pptable)
>  			return -ENOMEM;


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [v1 2/4] drm/amdgpu: accumulate gfx_off_entrycount in smu_suspend()
  2025-02-07  6:44 ` [v1 2/4] drm/amdgpu: accumulate gfx_off_entrycount in smu_suspend() Jiang Liu
@ 2025-02-07  8:04   ` Lazar, Lijo
  2025-02-07  8:30     ` Gerry Liu
  0 siblings, 1 reply; 12+ messages in thread
From: Lazar, Lijo @ 2025-02-07  8:04 UTC (permalink / raw)
  To: Jiang Liu, alexander.deucher, christian.koenig, airlied, simona,
	sunil.khatri, Hawking.Zhang, mario.limonciello, xiaogang.chen,
	Kent.Russell, shuox.liu, amd-gfx



On 2/7/2025 12:14 PM, Jiang Liu wrote:
> As pwfw resets entrycount when device is suspended, so we should
> accmulate the gfx_off_entrycount value instead of save the last value
> of it.
> 
> Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>
> ---
>  drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> index ed9dac00ebfb..70a5ab649e5f 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> @@ -2134,12 +2134,12 @@ static int smu_suspend(struct amdgpu_ip_block *ip_block)
>  	smu_set_gfx_cgpg(smu, false);
>  
>  	/*
> -	 * pwfw resets entrycount when device is suspended, so we save the
> -	 * last value to be used when we resume to keep it consistent
> +	 * pwfw resets entrycount when device is suspended, so we accumulate
> +	 * the `gfx_off_entrycount` value.
>  	 */
>  	ret = smu_get_entrycount_gfxoff(smu, &count);
>  	if (!ret)
> -		adev->gfx.gfx_off_entrycount = count;
> +		adev->gfx.gfx_off_entrycount += count;

This is slightly misleading - only Vangogh implements
get_gfx_off_entrycount and its implementation,
vangogh_get_gfxoff_entrycount, is already doing something like this -

*entrycount = value + adev->gfx.gfx_off_entrycount;

Thanks,
Lijo

>  
>  	/* clear this on suspend so it will get reprogrammed on resume */
>  	smu->workload_mask = 0;


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [v1 2/4] drm/amdgpu: accumulate gfx_off_entrycount in smu_suspend()
  2025-02-07  8:04   ` Lazar, Lijo
@ 2025-02-07  8:30     ` Gerry Liu
  2025-02-07  8:34       ` Lazar, Lijo
  0 siblings, 1 reply; 12+ messages in thread
From: Gerry Liu @ 2025-02-07  8:30 UTC (permalink / raw)
  To: Lazar, Lijo
  Cc: Deucher, Alexander, Koenig, Christian, airlied, simona,
	sunil.khatri, Hawking.Zhang, mario.limonciello, xiaogang.chen,
	Kent.Russell, shuox.liu, amd-gfx



> 2025年2月7日 16:04,Lazar, Lijo <lijo.lazar@amd.com> 写道:
> 
> 
> 
> On 2/7/2025 12:14 PM, Jiang Liu wrote:
>> As pwfw resets entrycount when device is suspended, so we should
>> accmulate the gfx_off_entrycount value instead of save the last value
>> of it.
>> 
>> Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>
>> ---
>> drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>> index ed9dac00ebfb..70a5ab649e5f 100644
>> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>> @@ -2134,12 +2134,12 @@ static int smu_suspend(struct amdgpu_ip_block *ip_block)
>> 	smu_set_gfx_cgpg(smu, false);
>> 
>> 	/*
>> -	 * pwfw resets entrycount when device is suspended, so we save the
>> -	 * last value to be used when we resume to keep it consistent
>> +	 * pwfw resets entrycount when device is suspended, so we accumulate
>> +	 * the `gfx_off_entrycount` value.
>> 	 */
>> 	ret = smu_get_entrycount_gfxoff(smu, &count);
>> 	if (!ret)
>> -		adev->gfx.gfx_off_entrycount = count;
>> +		adev->gfx.gfx_off_entrycount += count;
> 
> This is slightly misleading - only Vangogh implements
> get_gfx_off_entrycount and its implementation,
> vangogh_get_gfxoff_entrycount, is already doing something like this -
> 
> *entrycount = value + adev->gfx.gfx_off_entrycount;

Because we have not restored the hardware counter by writing back the value on resume, adev->gfx.gfx_off_entrycount only remembers the latest value of latest suspend/resume cycle. And history information is lost, so changed it to accumulate all values.

> 
> Thanks,
> Lijo
> 
>> 
>> 	/* clear this on suspend so it will get reprogrammed on resume */
>> 	smu->workload_mask = 0;


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [v1 2/4] drm/amdgpu: accumulate gfx_off_entrycount in smu_suspend()
  2025-02-07  8:30     ` Gerry Liu
@ 2025-02-07  8:34       ` Lazar, Lijo
  2025-02-07  8:42         ` Gerry Liu
  0 siblings, 1 reply; 12+ messages in thread
From: Lazar, Lijo @ 2025-02-07  8:34 UTC (permalink / raw)
  To: Gerry Liu
  Cc: Deucher, Alexander, Koenig, Christian, airlied, simona,
	sunil.khatri, Hawking.Zhang, mario.limonciello, xiaogang.chen,
	Kent.Russell, shuox.liu, amd-gfx



On 2/7/2025 2:00 PM, Gerry Liu wrote:
> 
> 
>> 2025年2月7日 16:04,Lazar, Lijo <lijo.lazar@amd.com> 写道:
>>
>>
>>
>> On 2/7/2025 12:14 PM, Jiang Liu wrote:
>>> As pwfw resets entrycount when device is suspended, so we should
>>> accmulate the gfx_off_entrycount value instead of save the last value
>>> of it.
>>>
>>> Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>
>>> ---
>>> drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>> index ed9dac00ebfb..70a5ab649e5f 100644
>>> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>> @@ -2134,12 +2134,12 @@ static int smu_suspend(struct amdgpu_ip_block *ip_block)
>>> 	smu_set_gfx_cgpg(smu, false);
>>>
>>> 	/*
>>> -	 * pwfw resets entrycount when device is suspended, so we save the
>>> -	 * last value to be used when we resume to keep it consistent
>>> +	 * pwfw resets entrycount when device is suspended, so we accumulate
>>> +	 * the `gfx_off_entrycount` value.
>>> 	 */
>>> 	ret = smu_get_entrycount_gfxoff(smu, &count);
>>> 	if (!ret)
>>> -		adev->gfx.gfx_off_entrycount = count;
>>> +		adev->gfx.gfx_off_entrycount += count;
>>
>> This is slightly misleading - only Vangogh implements
>> get_gfx_off_entrycount and its implementation,
>> vangogh_get_gfxoff_entrycount, is already doing something like this -
>>
>> *entrycount = value + adev->gfx.gfx_off_entrycount;
> 
> Because we have not restored the hardware counter by writing back the value on resume, adev->gfx.gfx_off_entrycount only remembers the latest value of latest suspend/resume cycle. And history information is lost, so changed it to accumulate all values.

What I meant is that accumulation is already done in the implementation
side - check implementation of vangogh_get_gfxoff_entrycount.

Thanks,
Lijo

> 
>>
>> Thanks,
>> Lijo
>>
>>>
>>> 	/* clear this on suspend so it will get reprogrammed on resume */
>>> 	smu->workload_mask = 0;
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [v1 2/4] drm/amdgpu: accumulate gfx_off_entrycount in smu_suspend()
  2025-02-07  8:34       ` Lazar, Lijo
@ 2025-02-07  8:42         ` Gerry Liu
  0 siblings, 0 replies; 12+ messages in thread
From: Gerry Liu @ 2025-02-07  8:42 UTC (permalink / raw)
  To: Lazar, Lijo
  Cc: Deucher, Alexander, Koenig, Christian, airlied, simona,
	sunil.khatri, Hawking.Zhang, mario.limonciello, xiaogang.chen,
	Kent.Russell, shuox.liu, amd-gfx



> 2025年2月7日 16:34,Lazar, Lijo <lijo.lazar@amd.com> 写道:
> 
> 
> 
> On 2/7/2025 2:00 PM, Gerry Liu wrote:
>> 
>> 
>>> 2025年2月7日 16:04,Lazar, Lijo <lijo.lazar@amd.com> 写道:
>>> 
>>> 
>>> 
>>> On 2/7/2025 12:14 PM, Jiang Liu wrote:
>>>> As pwfw resets entrycount when device is suspended, so we should
>>>> accmulate the gfx_off_entrycount value instead of save the last value
>>>> of it.
>>>> 
>>>> Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>
>>>> ---
>>>> drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 6 +++---
>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>> 
>>>> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>>> index ed9dac00ebfb..70a5ab649e5f 100644
>>>> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>>> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
>>>> @@ -2134,12 +2134,12 @@ static int smu_suspend(struct amdgpu_ip_block *ip_block)
>>>> 	smu_set_gfx_cgpg(smu, false);
>>>> 
>>>> 	/*
>>>> -	 * pwfw resets entrycount when device is suspended, so we save the
>>>> -	 * last value to be used when we resume to keep it consistent
>>>> +	 * pwfw resets entrycount when device is suspended, so we accumulate
>>>> +	 * the `gfx_off_entrycount` value.
>>>> 	 */
>>>> 	ret = smu_get_entrycount_gfxoff(smu, &count);
>>>> 	if (!ret)
>>>> -		adev->gfx.gfx_off_entrycount = count;
>>>> +		adev->gfx.gfx_off_entrycount += count;
>>> 
>>> This is slightly misleading - only Vangogh implements
>>> get_gfx_off_entrycount and its implementation,
>>> vangogh_get_gfxoff_entrycount, is already doing something like this -
>>> 
>>> *entrycount = value + adev->gfx.gfx_off_entrycount;
>> 
>> Because we have not restored the hardware counter by writing back the value on resume, adev->gfx.gfx_off_entrycount only remembers the latest value of latest suspend/resume cycle. And history information is lost, so changed it to accumulate all values.
> 
> What I meant is that accumulation is already done in the implementation
> side - check implementation of vangogh_get_gfxoff_entrycount.
Aha, you are right, smu_suspend() calls smu_get_entrycount_gfxoff(), it already accumulates along the way.
Will drop it.

> 
> Thanks,
> Lijo
> 
>> 
>>> 
>>> Thanks,
>>> Lijo
>>> 
>>>> 
>>>> 	/* clear this on suspend so it will get reprogrammed on resume */
>>>> 	smu->workload_mask = 0;


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [v1 4/4] drm/amdgpu: minor code style enhancement for smu
  2025-02-07  6:44 ` [v1 4/4] drm/amdgpu: minor code style enhancement for smu Jiang Liu
@ 2025-02-07 10:37   ` Lazar, Lijo
  0 siblings, 0 replies; 12+ messages in thread
From: Lazar, Lijo @ 2025-02-07 10:37 UTC (permalink / raw)
  To: Jiang Liu, alexander.deucher, christian.koenig, airlied, simona,
	sunil.khatri, Hawking.Zhang, mario.limonciello, xiaogang.chen,
	Kent.Russell, shuox.liu, amd-gfx



On 2/7/2025 12:14 PM, Jiang Liu wrote:
> Minor code style enhancement for smu.
> 
> Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>
> ---
>  drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c            | 2 +-
>  drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> index 08b42c7a4ad1..51e7bd4d3444 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> @@ -2153,7 +2153,7 @@ static int smu_resume(struct amdgpu_ip_block *ip_block)
>  	struct amdgpu_device *adev = ip_block->adev;
>  	struct smu_context *smu = adev->powerplay.pp_handle;
>  
> -	if (amdgpu_sriov_vf(adev)&& !amdgpu_sriov_is_pp_one_vf(adev))
> +	if (amdgpu_sriov_vf(adev) && !amdgpu_sriov_is_pp_one_vf(adev))

This is already taken care in another patch. Rest of the patch is fine.

Thanks,
Lijo

>  		return 0;
>  
>  	if (!smu->pm_enabled)
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c
> index da7bd9227afe..da55d66099fb 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_6_ppt.c
> @@ -3524,8 +3524,8 @@ static const struct pptable_funcs smu_v13_0_6_ppt_funcs = {
>  	.init_power = smu_v13_0_init_power,
>  	.fini_power = smu_v13_0_fini_power,
>  	.check_fw_status = smu_v13_0_6_check_fw_status,
> -	/* pptable related */
>  	.check_fw_version = smu_v13_0_6_check_fw_version,
> +	/* pptable related */
>  	.set_driver_table_location = smu_v13_0_set_driver_table_location,
>  	.set_tool_table_location = smu_v13_0_set_tool_table_location,
>  	.notify_memory_pool_location = smu_v13_0_notify_memory_pool_location,


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [v1 1/4] drm/amdgpu: avoid buffer overflow attach in smu_sys_set_pp_table()
  2025-02-07  7:56   ` Lazar, Lijo
@ 2025-02-07 18:17     ` Alex Deucher
  0 siblings, 0 replies; 12+ messages in thread
From: Alex Deucher @ 2025-02-07 18:17 UTC (permalink / raw)
  To: Lazar, Lijo
  Cc: Jiang Liu, alexander.deucher, christian.koenig, airlied, simona,
	sunil.khatri, Hawking.Zhang, mario.limonciello, xiaogang.chen,
	Kent.Russell, shuox.liu, amd-gfx

Applied.  Thanks!

On Fri, Feb 7, 2025 at 3:02 AM Lazar, Lijo <lijo.lazar@amd.com> wrote:
>
>
>
> On 2/7/2025 12:14 PM, Jiang Liu wrote:
> > It malicious user provides a small pptable through sysfs and then
> > a bigger pptable, it may cause buffer overflow attack in function
> > smu_sys_set_pp_table().
> >
> > Signed-off-by: Jiang Liu <gerry@linux.alibaba.com>
>
>         Reviewed-by: Lijo Lazar <lijo.lazar@amd.com>
>
> Thanks,
> Lijo
>
> > ---
> >  drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > index 8ca793c222ff..ed9dac00ebfb 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > @@ -612,7 +612,8 @@ static int smu_sys_set_pp_table(void *handle,
> >               return -EIO;
> >       }
> >
> > -     if (!smu_table->hardcode_pptable) {
> > +     if (!smu_table->hardcode_pptable || smu_table->power_play_table_size < size) {
> > +             kfree(smu_table->hardcode_pptable);
> >               smu_table->hardcode_pptable = kzalloc(size, GFP_KERNEL);
> >               if (!smu_table->hardcode_pptable)
> >                       return -ENOMEM;
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2025-02-07 18:19 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-07  6:44 [v1 0/4] Fix a buffer overflow in drm/amdgpu/smu Jiang Liu
2025-02-07  6:44 ` [v1 1/4] drm/amdgpu: avoid buffer overflow attach in smu_sys_set_pp_table() Jiang Liu
2025-02-07  7:56   ` Lazar, Lijo
2025-02-07 18:17     ` Alex Deucher
2025-02-07  6:44 ` [v1 2/4] drm/amdgpu: accumulate gfx_off_entrycount in smu_suspend() Jiang Liu
2025-02-07  8:04   ` Lazar, Lijo
2025-02-07  8:30     ` Gerry Liu
2025-02-07  8:34       ` Lazar, Lijo
2025-02-07  8:42         ` Gerry Liu
2025-02-07  6:44 ` [v1 3/4] drm/amdgpu: treat VCN as enabled if either VCN or JPEC is enabled Jiang Liu
2025-02-07  6:44 ` [v1 4/4] drm/amdgpu: minor code style enhancement for smu Jiang Liu
2025-02-07 10:37   ` Lazar, Lijo

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.