[PATCH] drm/amd/amdgpu: default to zero number of states if not enabled

[PATCH] drm/amd/amdgpu: default to zero number of states if not enabled

[Colin King]

From: Colin Ian King <colin.king@canonical.com>

Currently, if adev->pp_enabled is false then the pp_stats_info data
is not read and hence a garbage number of states from the stack
is used to dump out the number of states. Given data.nums could be
any random value, this could easily lead to read outside the
data.states array.  Fix this by setting data.nums to zero if
adev->pp_enabled is false.

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
index accc908..808d788 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
@@ -195,6 +195,8 @@ static ssize_t amdgpu_get_pp_num_states(struct device *dev,
 
 	if (adev->pp_enabled)
 		amdgpu_dpm_get_pp_num_states(adev, &data);
+	else
+		data.nums = 0;
 
 	buf_len = snprintf(buf, PAGE_SIZE, "states: %d\n", data.nums);
 	for (i = 0; i < data.nums; i++)
-- 
2.9.3

Re: [PATCH] drm/amd/amdgpu: default to zero number of states if not enabled

[Alex Deucher]

On Thu, Oct 6, 2016 at 2:02 PM, Colin King <colin.king@canonical.com> wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Currently, if adev->pp_enabled is false then the pp_stats_info data
> is not read and hence a garbage number of states from the stack
> is used to dump out the number of states. Given data.nums could be
> any random value, this could easily lead to read outside the
> data.states array.  Fix this by setting data.nums to zero if
> adev->pp_enabled is false.

Are you actually seeing a problem?  The pp_num_states attribute only
gets added in the first place if pp_enabled is true.

Alex

>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> index accc908..808d788 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> @@ -195,6 +195,8 @@ static ssize_t amdgpu_get_pp_num_states(struct device *dev,
>
>         if (adev->pp_enabled)
>                 amdgpu_dpm_get_pp_num_states(adev, &data);
> +       else
> +               data.nums = 0;
>
>         buf_len = snprintf(buf, PAGE_SIZE, "states: %d\n", data.nums);
>         for (i = 0; i < data.nums; i++)
> --
> 2.9.3
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/amd/amdgpu: default to zero number of states if not enabled

[Colin Ian King]

On 06/10/16 19:32, Alex Deucher wrote:
> On Thu, Oct 6, 2016 at 2:02 PM, Colin King <colin.king@canonical.com> wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> Currently, if adev->pp_enabled is false then the pp_stats_info data
>> is not read and hence a garbage number of states from the stack
>> is used to dump out the number of states. Given data.nums could be
>> any random value, this could easily lead to read outside the
>> data.states array.  Fix this by setting data.nums to zero if
>> adev->pp_enabled is false.
> 
> Are you actually seeing a problem?  

Nope.

> The pp_num_states attribute only
> gets added in the first place if pp_enabled is true.

Does that mean that the check on adev->pp_enabled is redundant then?

> 
> Alex

> 
>>
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>> index accc908..808d788 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>> @@ -195,6 +195,8 @@ static ssize_t amdgpu_get_pp_num_states(struct device *dev,
>>
>>         if (adev->pp_enabled)
>>                 amdgpu_dpm_get_pp_num_states(adev, &data);
>> +       else
>> +               data.nums = 0;
>>
>>         buf_len = snprintf(buf, PAGE_SIZE, "states: %d\n", data.nums);
>>         for (i = 0; i < data.nums; i++)
>> --
>> 2.9.3
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH] drm/amd/amdgpu: default to zero number of states if not enabled

[Deucher, Alexander]

> -----Original Message-----
> From: Colin Ian King [mailto:colin.king@canonical.com]
> Sent: Thursday, October 06, 2016 3:04 PM
> To: Alex Deucher
> Cc: Deucher, Alexander; Koenig, Christian; David Airlie; Huang, JinHuiEric;
> Zhu, Rex; Zhou, Jammy; StDenis, Tom; Dan Carpenter; Maling list - DRI
> developers; LKML
> Subject: Re: [PATCH] drm/amd/amdgpu: default to zero number of states if
> not enabled
> 
> On 06/10/16 19:32, Alex Deucher wrote:
> > On Thu, Oct 6, 2016 at 2:02 PM, Colin King <colin.king@canonical.com>
> wrote:
> >> From: Colin Ian King <colin.king@canonical.com>
> >>
> >> Currently, if adev->pp_enabled is false then the pp_stats_info data
> >> is not read and hence a garbage number of states from the stack
> >> is used to dump out the number of states. Given data.nums could be
> >> any random value, this could easily lead to read outside the
> >> data.states array.  Fix this by setting data.nums to zero if
> >> adev->pp_enabled is false.
> >
> > Are you actually seeing a problem?
> 
> Nope.
> 
> > The pp_num_states attribute only
> > gets added in the first place if pp_enabled is true.
> 
> Does that mean that the check on adev->pp_enabled is redundant then?

Yes, I think so.

Alex

> 
> >
> > Alex
> 
> >
> >>
> >> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> >> ---
> >>  drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> >> index accc908..808d788 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> >> @@ -195,6 +195,8 @@ static ssize_t amdgpu_get_pp_num_states(struct
> device *dev,
> >>
> >>         if (adev->pp_enabled)
> >>                 amdgpu_dpm_get_pp_num_states(adev, &data);
> >> +       else
> >> +               data.nums = 0;
> >>
> >>         buf_len = snprintf(buf, PAGE_SIZE, "states: %d\n", data.nums);
> >>         for (i = 0; i < data.nums; i++)
> >> --
> >> 2.9.3
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/amd/amdgpu: default to zero number of states if not enabled

[Colin Ian King]

On 06/10/16 20:04, Deucher, Alexander wrote:
>> -----Original Message-----
>> From: Colin Ian King [mailto:colin.king@canonical.com]
>> Sent: Thursday, October 06, 2016 3:04 PM
>> To: Alex Deucher
>> Cc: Deucher, Alexander; Koenig, Christian; David Airlie; Huang, JinHuiEric;
>> Zhu, Rex; Zhou, Jammy; StDenis, Tom; Dan Carpenter; Maling list - DRI
>> developers; LKML
>> Subject: Re: [PATCH] drm/amd/amdgpu: default to zero number of states if
>> not enabled
>>
>> On 06/10/16 19:32, Alex Deucher wrote:
>>> On Thu, Oct 6, 2016 at 2:02 PM, Colin King <colin.king@canonical.com>
>> wrote:
>>>> From: Colin Ian King <colin.king@canonical.com>
>>>>
>>>> Currently, if adev->pp_enabled is false then the pp_stats_info data
>>>> is not read and hence a garbage number of states from the stack
>>>> is used to dump out the number of states. Given data.nums could be
>>>> any random value, this could easily lead to read outside the
>>>> data.states array.  Fix this by setting data.nums to zero if
>>>> adev->pp_enabled is false.
>>>
>>> Are you actually seeing a problem?
>>
>> Nope.
>>
>>> The pp_num_states attribute only
>>> gets added in the first place if pp_enabled is true.
>>
>> Does that mean that the check on adev->pp_enabled is redundant then?
> 
> Yes, I think so.

OK, in which case it's probably extraneous code that could be removed.
> 
> Alex
> 
>>
>>>
>>> Alex
>>
>>>
>>>>
>>>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>>>> ---
>>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>>>> index accc908..808d788 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>>>> @@ -195,6 +195,8 @@ static ssize_t amdgpu_get_pp_num_states(struct
>> device *dev,
>>>>
>>>>         if (adev->pp_enabled)
>>>>                 amdgpu_dpm_get_pp_num_states(adev, &data);
>>>> +       else
>>>> +               data.nums = 0;
>>>>
>>>>         buf_len = snprintf(buf, PAGE_SIZE, "states: %d\n", data.nums);
>>>>         for (i = 0; i < data.nums; i++)
>>>> --
>>>> 2.9.3
>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>