[PATCH] drm/amdgpu: Fix out-of-bounds write warning

[PATCH] drm/amdgpu: Fix out-of-bounds write warning

[Ma Jun]

Check the ring type value to fix the out-of-bounds
write warning

Signed-off-by: Ma Jun <Jun.Ma2@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
index 06f0a6534a94..1e0b5bb47bc9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
@@ -353,6 +353,11 @@ int amdgpu_ring_init(struct amdgpu_device *adev, struct amdgpu_ring *ring,
 	ring->hw_prio = hw_prio;
 
 	if (!ring->no_scheduler) {
+		if (ring->funcs->type >= AMDGPU_HW_IP_NUM) {
+			dev_warn(adev->dev, "ring type %d has no scheduler\n", ring->funcs->type);
+			return 0;
+		}
+
 		hw_ip = ring->funcs->type;
 		num_sched = &adev->gpu_sched[hw_ip][hw_prio].num_scheds;
 		adev->gpu_sched[hw_ip][hw_prio].sched[(*num_sched)++] =
-- 
2.34.1

[PATCH v2 1/2] drm/amdgpu: Fix uninitialized variable warning in amdgpu_afmt_acr

[Ma Jun]

Assign value to clock to fix the warning below:
"Using uninitialized value res. Field res.clock is uninitialized"

Signed-off-by: Ma Jun <Jun.Ma2@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_afmt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_afmt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_afmt.c
index a4d65973bf7c..80771b1480ff 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_afmt.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_afmt.c
@@ -100,6 +100,7 @@ struct amdgpu_afmt_acr amdgpu_afmt_acr(uint32_t clock)
 	amdgpu_afmt_calc_cts(clock, &res.cts_32khz, &res.n_32khz, 32000);
 	amdgpu_afmt_calc_cts(clock, &res.cts_44_1khz, &res.n_44_1khz, 44100);
 	amdgpu_afmt_calc_cts(clock, &res.cts_48khz, &res.n_48khz, 48000);
+	res.clock = clock;
 
 	return res;
 }
-- 
2.34.1

[PATCH v2 2/2] drm/amdgpu: Fix the uninitialized variable warning

[Ma Jun]

Initialize the phy_id to 0 to fix the warning of
"Using uninitialized value phy_id"

Signed-off-by: Ma Jun <Jun.Ma2@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
index 8ed0e073656f..53d85fafd8ab 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
@@ -95,7 +95,7 @@ static ssize_t amdgpu_securedisplay_debugfs_write(struct file *f, const char __u
 	struct psp_context *psp = &adev->psp;
 	struct ta_securedisplay_cmd *securedisplay_cmd;
 	struct drm_device *dev = adev_to_drm(adev);
-	uint32_t phy_id;
+	uint32_t phy_id = 0;
 	uint32_t op;
 	char str[64];
 	int ret;
@@ -135,6 +135,10 @@ static ssize_t amdgpu_securedisplay_debugfs_write(struct file *f, const char __u
 		mutex_unlock(&psp->securedisplay_context.mutex);
 		break;
 	case 2:
+		if (size < 3) {
+			dev_err(adev->dev, "Invalid input: %s\n", str);
+			return -EINVAL;
+		}
 		mutex_lock(&psp->securedisplay_context.mutex);
 		psp_prep_securedisplay_cmd_buf(psp, &securedisplay_cmd,
 			TA_SECUREDISPLAY_COMMAND__SEND_ROI_CRC);
-- 
2.34.1

Re: [PATCH v2 2/2] drm/amdgpu: Fix the uninitialized variable warning

[Lazar, Lijo]

On 4/25/2024 3:30 PM, Ma Jun wrote:
> Initialize the phy_id to 0 to fix the warning of
> "Using uninitialized value phy_id"
> 
> Signed-off-by: Ma Jun <Jun.Ma2@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
> index 8ed0e073656f..53d85fafd8ab 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
> @@ -95,7 +95,7 @@ static ssize_t amdgpu_securedisplay_debugfs_write(struct file *f, const char __u
>  	struct psp_context *psp = &adev->psp;
>  	struct ta_securedisplay_cmd *securedisplay_cmd;
>  	struct drm_device *dev = adev_to_drm(adev);
> -	uint32_t phy_id;
> +	uint32_t phy_id = 0;
>  	uint32_t op;
>  	char str[64];
>  	int ret;
> @@ -135,6 +135,10 @@ static ssize_t amdgpu_securedisplay_debugfs_write(struct file *f, const char __u
>  		mutex_unlock(&psp->securedisplay_context.mutex);
>  		break;
>  	case 2:
> +		if (size < 3) {
> +			dev_err(adev->dev, "Invalid input: %s\n", str);
> +			return -EINVAL;
> +		}

Better is to check the return of sscanf to see if phy_id value is
successfully scanned. Otherwise, return error.

Thanks,
Lijo

>  		mutex_lock(&psp->securedisplay_context.mutex);
>  		psp_prep_securedisplay_cmd_buf(psp, &securedisplay_cmd,
>  			TA_SECUREDISPLAY_COMMAND__SEND_ROI_CRC);

Re: [PATCH] drm/amdgpu: Fix out-of-bounds write warning

[Christian König]

Am 25.04.24 um 12:00 schrieb Ma Jun:
> Check the ring type value to fix the out-of-bounds
> write warning
>
> Signed-off-by: Ma Jun <Jun.Ma2@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> index 06f0a6534a94..1e0b5bb47bc9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> @@ -353,6 +353,11 @@ int amdgpu_ring_init(struct amdgpu_device *adev, struct amdgpu_ring *ring,
>   	ring->hw_prio = hw_prio;
>   
>   	if (!ring->no_scheduler) {
> +		if (ring->funcs->type >= AMDGPU_HW_IP_NUM) {
> +			dev_warn(adev->dev, "ring type %d has no scheduler\n", ring->funcs->type);
> +			return 0;
> +		}
> +

That check should probably be at the beginning of the function since 
trying to initialize a ring with an invalid type should be rejected 
immediately.

Regards,
Christian.

>   		hw_ip = ring->funcs->type;
>   		num_sched = &adev->gpu_sched[hw_ip][hw_prio].num_scheds;
>   		adev->gpu_sched[hw_ip][hw_prio].sched[(*num_sched)++] =

Re: [PATCH v2 2/2] drm/amdgpu: Fix the uninitialized variable warning

[Alex Deucher]

On Thu, Apr 25, 2024 at 6:17 AM Ma Jun <Jun.Ma2@amd.com> wrote:
>
> Initialize the phy_id to 0 to fix the warning of
> "Using uninitialized value phy_id"
>
> Signed-off-by: Ma Jun <Jun.Ma2@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
> index 8ed0e073656f..53d85fafd8ab 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
> @@ -95,7 +95,7 @@ static ssize_t amdgpu_securedisplay_debugfs_write(struct file *f, const char __u
>         struct psp_context *psp = &adev->psp;
>         struct ta_securedisplay_cmd *securedisplay_cmd;
>         struct drm_device *dev = adev_to_drm(adev);
> -       uint32_t phy_id;
> +       uint32_t phy_id = 0;

You can drop this hunk now that you added the check below.

Alex

>         uint32_t op;
>         char str[64];
>         int ret;
> @@ -135,6 +135,10 @@ static ssize_t amdgpu_securedisplay_debugfs_write(struct file *f, const char __u
>                 mutex_unlock(&psp->securedisplay_context.mutex);
>                 break;
>         case 2:
> +               if (size < 3) {
> +                       dev_err(adev->dev, "Invalid input: %s\n", str);
> +                       return -EINVAL;
> +               }
>                 mutex_lock(&psp->securedisplay_context.mutex);
>                 psp_prep_securedisplay_cmd_buf(psp, &securedisplay_cmd,
>                         TA_SECUREDISPLAY_COMMAND__SEND_ROI_CRC);
> --
> 2.34.1
>

Re: [PATCH v2 1/2] drm/amdgpu: Fix uninitialized variable warning in amdgpu_afmt_acr

[Alex Deucher]

On Thu, Apr 25, 2024 at 6:07 AM Ma Jun <Jun.Ma2@amd.com> wrote:
>
> Assign value to clock to fix the warning below:
> "Using uninitialized value res. Field res.clock is uninitialized"
>
> Signed-off-by: Ma Jun <Jun.Ma2@amd.com>

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_afmt.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_afmt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_afmt.c
> index a4d65973bf7c..80771b1480ff 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_afmt.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_afmt.c
> @@ -100,6 +100,7 @@ struct amdgpu_afmt_acr amdgpu_afmt_acr(uint32_t clock)
>         amdgpu_afmt_calc_cts(clock, &res.cts_32khz, &res.n_32khz, 32000);
>         amdgpu_afmt_calc_cts(clock, &res.cts_44_1khz, &res.n_44_1khz, 44100);
>         amdgpu_afmt_calc_cts(clock, &res.cts_48khz, &res.n_48khz, 48000);
> +       res.clock = clock;
>
>         return res;
>  }
> --
> 2.34.1
>

Re: [PATCH v2 2/2] drm/amdgpu: Fix the uninitialized variable warning

[Ma, Jun]

On 4/25/2024 6:10 PM, Lazar, Lijo wrote:
> 
> 
> On 4/25/2024 3:30 PM, Ma Jun wrote:
>> Initialize the phy_id to 0 to fix the warning of
>> "Using uninitialized value phy_id"
>>
>> Signed-off-by: Ma Jun <Jun.Ma2@amd.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
>> index 8ed0e073656f..53d85fafd8ab 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
>> @@ -95,7 +95,7 @@ static ssize_t amdgpu_securedisplay_debugfs_write(struct file *f, const char __u
>>  	struct psp_context *psp = &adev->psp;
>>  	struct ta_securedisplay_cmd *securedisplay_cmd;
>>  	struct drm_device *dev = adev_to_drm(adev);
>> -	uint32_t phy_id;
>> +	uint32_t phy_id = 0;
>>  	uint32_t op;
>>  	char str[64];
>>  	int ret;
>> @@ -135,6 +135,10 @@ static ssize_t amdgpu_securedisplay_debugfs_write(struct file *f, const char __u
>>  		mutex_unlock(&psp->securedisplay_context.mutex);
>>  		break;
>>  	case 2:
>> +		if (size < 3) {
>> +			dev_err(adev->dev, "Invalid input: %s\n", str);
>> +			return -EINVAL;
>> +		}
> 
> Better is to check the return of sscanf to see if phy_id value is
> successfully scanned. Otherwise, return error.

Thanks, will fix in v3

Regards,
Ma Jun
> 
> Thanks,
> Lijo
> 
>>  		mutex_lock(&psp->securedisplay_context.mutex);
>>  		psp_prep_securedisplay_cmd_buf(psp, &securedisplay_cmd,
>>  			TA_SECUREDISPLAY_COMMAND__SEND_ROI_CRC);

Re: [PATCH] drm/amdgpu: Fix out-of-bounds write warning

[Ma, Jun]

On 4/25/2024 8:39 PM, Christian König wrote:
> 
> 
> Am 25.04.24 um 12:00 schrieb Ma Jun:
>> Check the ring type value to fix the out-of-bounds
>> write warning
>>
>> Signed-off-by: Ma Jun <Jun.Ma2@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>> index 06f0a6534a94..1e0b5bb47bc9 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>> @@ -353,6 +353,11 @@ int amdgpu_ring_init(struct amdgpu_device *adev, struct amdgpu_ring *ring,
>>   	ring->hw_prio = hw_prio;
>>   
>>   	if (!ring->no_scheduler) {
>> +		if (ring->funcs->type >= AMDGPU_HW_IP_NUM) {
>> +			dev_warn(adev->dev, "ring type %d has no scheduler\n", ring->funcs->type);
>> +			return 0;
>> +		}
>> +
> 
> That check should probably be at the beginning of the function since 
> trying to initialize a ring with an invalid type should be rejected 
> immediately.
> 
This check is used to skip the gpu_sched setting for the rings
which don't have scheduler, such as KIQ, MES, UMSCH_MM. 
Without this check, there could be an potential out-of-bounds writing
when ring->no__scheduler is not set correctly.

Regards,
Ma Jun

> Regards,
> Christian.
> 
>>   		hw_ip = ring->funcs->type;
>>   		num_sched = &adev->gpu_sched[hw_ip][hw_prio].num_scheds;
>>   		adev->gpu_sched[hw_ip][hw_prio].sched[(*num_sched)++] =
> 

Re: [PATCH] drm/amdgpu: Fix out-of-bounds write warning

[Christian König]

Am 26.04.24 um 05:24 schrieb Ma, Jun:
> On 4/25/2024 8:39 PM, Christian König wrote:
>> Am 25.04.24 um 12:00 schrieb Ma Jun:
>>> Check the ring type value to fix the out-of-bounds
>>> write warning
>>>
>>> Signed-off-by: Ma Jun <Jun.Ma2@amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 5 +++++
>>>    1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>> index 06f0a6534a94..1e0b5bb47bc9 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>> @@ -353,6 +353,11 @@ int amdgpu_ring_init(struct amdgpu_device *adev, struct amdgpu_ring *ring,
>>>    	ring->hw_prio = hw_prio;
>>>    
>>>    	if (!ring->no_scheduler) {
>>> +		if (ring->funcs->type >= AMDGPU_HW_IP_NUM) {
>>> +			dev_warn(adev->dev, "ring type %d has no scheduler\n", ring->funcs->type);
>>> +			return 0;
>>> +		}
>>> +
>> That check should probably be at the beginning of the function since
>> trying to initialize a ring with an invalid type should be rejected
>> immediately.
>>
> This check is used to skip the gpu_sched setting for the rings
> which don't have scheduler, such as KIQ, MES, UMSCH_MM.
> Without this check, there could be an potential out-of-bounds writing
> when ring->no__scheduler is not set correctly.

Ah! In this case that that is not really clean.

Probably best approach is to change the "if (!ring->no_scheduler)" instead.

Maybe even move this code here into amdgpu_ctx.c:

                 hw_ip = ring->funcs->type;
                 num_sched = &adev->gpu_sched[hw_ip][hw_prio].num_scheds;
adev->gpu_sched[hw_ip][hw_prio].sched[(*num_sched)++] =
                         &ring->sched;

And then add something like hw_ip <= AMDGPU_HW_IP_NUM.

Background is that it is perfectly valid to have a scheduler for the MES 
for example.

Regards,
Christian.

>
> Regards,
> Ma Jun
>
>> Regards,
>> Christian.
>>
>>>    		hw_ip = ring->funcs->type;
>>>    		num_sched = &adev->gpu_sched[hw_ip][hw_prio].num_scheds;
>>>    		adev->gpu_sched[hw_ip][hw_prio].sched[(*num_sched)++] =