diff mbox

[v2] drm/amdgpu: missing bounds check in amdgpu_set_pp_force_state()

Message ID 20160616083023.GA7046@mwanda (mailing list archive)
State New, archived
Headers show

Commit Message

Dan Carpenter June 16, 2016, 8:30 a.m. UTC
There is no limit on high "idx" can go.  It should be less than
ARRAY_SIZE(data.states) which is 16.

The "data" variable wasn't declared in that scope so I shifted the code
around a bit to make it work.  Also I made "idx" unsigned.

Fixes: f3898ea12fc1 ('drm/amd/powerplay: add some sysfs interfaces for powerplay.')
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: make idx unsigned

Comments

Christian König June 16, 2016, 8:39 a.m. UTC | #1
Am 16.06.2016 um 10:30 schrieb Dan Carpenter:
> There is no limit on high "idx" can go.  It should be less than
> ARRAY_SIZE(data.states) which is 16.
>
> The "data" variable wasn't declared in that scope so I shifted the code
> around a bit to make it work.  Also I made "idx" unsigned.
>
> Fixes: f3898ea12fc1 ('drm/amd/powerplay: add some sysfs interfaces for powerplay.')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Acked-by: Christian König <christian.koenig@amd.com>.

> ---
> v2: make idx unsigned
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> index 589b36e..0e13d80 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> @@ -270,30 +270,28 @@ static ssize_t amdgpu_set_pp_force_state(struct device *dev,
>   	struct drm_device *ddev = dev_get_drvdata(dev);
>   	struct amdgpu_device *adev = ddev->dev_private;
>   	enum amd_pm_state_type state = 0;
> -	long idx;
> +	unsigned long idx;
>   	int ret;
>   
>   	if (strlen(buf) == 1)
>   		adev->pp_force_state_enabled = false;
> -	else {
> -		ret = kstrtol(buf, 0, &idx);
> +	else if (adev->pp_enabled) {
> +		struct pp_states_info data;
>   
> -		if (ret) {
> +		ret = kstrtoul(buf, 0, &idx);
> +		if (ret || idx >= ARRAY_SIZE(data.states)) {
>   			count = -EINVAL;
>   			goto fail;
>   		}
>   
> -		if (adev->pp_enabled) {
> -			struct pp_states_info data;
> -			amdgpu_dpm_get_pp_num_states(adev, &data);
> -			state = data.states[idx];
> -			/* only set user selected power states */
> -			if (state != POWER_STATE_TYPE_INTERNAL_BOOT &&
> -				state != POWER_STATE_TYPE_DEFAULT) {
> -				amdgpu_dpm_dispatch_task(adev,
> -						AMD_PP_EVENT_ENABLE_USER_STATE, &state, NULL);
> -				adev->pp_force_state_enabled = true;
> -			}
> +		amdgpu_dpm_get_pp_num_states(adev, &data);
> +		state = data.states[idx];
> +		/* only set user selected power states */
> +		if (state != POWER_STATE_TYPE_INTERNAL_BOOT &&
> +		    state != POWER_STATE_TYPE_DEFAULT) {
> +			amdgpu_dpm_dispatch_task(adev,
> +					AMD_PP_EVENT_ENABLE_USER_STATE, &state, NULL);
> +			adev->pp_force_state_enabled = true;
>   		}
>   	}
>   fail:
Alex Deucher June 17, 2016, 4:34 p.m. UTC | #2
On Thu, Jun 16, 2016 at 4:39 AM, Christian König
<christian.koenig@amd.com> wrote:
> Am 16.06.2016 um 10:30 schrieb Dan Carpenter:
>>
>> There is no limit on high "idx" can go.  It should be less than
>> ARRAY_SIZE(data.states) which is 16.
>>
>> The "data" variable wasn't declared in that scope so I shifted the code
>> around a bit to make it work.  Also I made "idx" unsigned.
>>
>> Fixes: f3898ea12fc1 ('drm/amd/powerplay: add some sysfs interfaces for
>> powerplay.')
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
>
> Acked-by: Christian König <christian.koenig@amd.com>.
>

Applied.  thanks!

Alex

>
>> ---
>> v2: make idx unsigned
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>> index 589b36e..0e13d80 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
>> @@ -270,30 +270,28 @@ static ssize_t amdgpu_set_pp_force_state(struct
>> device *dev,
>>         struct drm_device *ddev = dev_get_drvdata(dev);
>>         struct amdgpu_device *adev = ddev->dev_private;
>>         enum amd_pm_state_type state = 0;
>> -       long idx;
>> +       unsigned long idx;
>>         int ret;
>>         if (strlen(buf) == 1)
>>                 adev->pp_force_state_enabled = false;
>> -       else {
>> -               ret = kstrtol(buf, 0, &idx);
>> +       else if (adev->pp_enabled) {
>> +               struct pp_states_info data;
>>   -             if (ret) {
>> +               ret = kstrtoul(buf, 0, &idx);
>> +               if (ret || idx >= ARRAY_SIZE(data.states)) {
>>                         count = -EINVAL;
>>                         goto fail;
>>                 }
>>   -             if (adev->pp_enabled) {
>> -                       struct pp_states_info data;
>> -                       amdgpu_dpm_get_pp_num_states(adev, &data);
>> -                       state = data.states[idx];
>> -                       /* only set user selected power states */
>> -                       if (state != POWER_STATE_TYPE_INTERNAL_BOOT &&
>> -                               state != POWER_STATE_TYPE_DEFAULT) {
>> -                               amdgpu_dpm_dispatch_task(adev,
>> -
>> AMD_PP_EVENT_ENABLE_USER_STATE, &state, NULL);
>> -                               adev->pp_force_state_enabled = true;
>> -                       }
>> +               amdgpu_dpm_get_pp_num_states(adev, &data);
>> +               state = data.states[idx];
>> +               /* only set user selected power states */
>> +               if (state != POWER_STATE_TYPE_INTERNAL_BOOT &&
>> +                   state != POWER_STATE_TYPE_DEFAULT) {
>> +                       amdgpu_dpm_dispatch_task(adev,
>> +                                       AMD_PP_EVENT_ENABLE_USER_STATE,
>> &state, NULL);
>> +                       adev->pp_force_state_enabled = true;
>>                 }
>>         }
>>   fail:
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff mbox

Patch

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
index 589b36e..0e13d80 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
@@ -270,30 +270,28 @@  static ssize_t amdgpu_set_pp_force_state(struct device *dev,
 	struct drm_device *ddev = dev_get_drvdata(dev);
 	struct amdgpu_device *adev = ddev->dev_private;
 	enum amd_pm_state_type state = 0;
-	long idx;
+	unsigned long idx;
 	int ret;
 
 	if (strlen(buf) == 1)
 		adev->pp_force_state_enabled = false;
-	else {
-		ret = kstrtol(buf, 0, &idx);
+	else if (adev->pp_enabled) {
+		struct pp_states_info data;
 
-		if (ret) {
+		ret = kstrtoul(buf, 0, &idx);
+		if (ret || idx >= ARRAY_SIZE(data.states)) {
 			count = -EINVAL;
 			goto fail;
 		}
 
-		if (adev->pp_enabled) {
-			struct pp_states_info data;
-			amdgpu_dpm_get_pp_num_states(adev, &data);
-			state = data.states[idx];
-			/* only set user selected power states */
-			if (state != POWER_STATE_TYPE_INTERNAL_BOOT &&
-				state != POWER_STATE_TYPE_DEFAULT) {
-				amdgpu_dpm_dispatch_task(adev,
-						AMD_PP_EVENT_ENABLE_USER_STATE, &state, NULL);
-				adev->pp_force_state_enabled = true;
-			}
+		amdgpu_dpm_get_pp_num_states(adev, &data);
+		state = data.states[idx];
+		/* only set user selected power states */
+		if (state != POWER_STATE_TYPE_INTERNAL_BOOT &&
+		    state != POWER_STATE_TYPE_DEFAULT) {
+			amdgpu_dpm_dispatch_task(adev,
+					AMD_PP_EVENT_ENABLE_USER_STATE, &state, NULL);
+			adev->pp_force_state_enabled = true;
 		}
 	}
 fail: