drm/amdgpu: missing bounds check in amdgpu_set_pp_force_state()

PATCH Superseded Dan Carpenter 4 participants 8 messages Jun 16, 2016 – Jun 17, 2016

Subsystems (3)

DRM drivers Kernel janitors Radeon and amdgpu DRM drivers

lore

Share & tools

lore

Dan Carpenter

author

2016-06-16 06:41 UTC #0 lore raw

PATCH 1/1 drm/amdgpu: missing bounds check in amdgpu_set_pp_force_state()

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.

Fixes f3898ea12fc1 ('drm/amd/powerplay: add some sysfs interfaces for powerplay.')

Signed-off-by Dan Carpenter <[email protected]>

drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c +12 ~~-14~~

index 589b36e..ce9e97f 100644
@@ -275,25 +275,23 @@ static ssize_t amdgpu_set_pp_force_state(struct device *dev,
 
 	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 = kstrtol(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:

walter harms

2016-06-16 07:26 UTC #1 lore raw

Am 16.06.2016 08:41, schrieb Dan Carpenter:

Quoted 18 lines

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.
Fixes f3898ea12fc1 ('drm/amd/powerplay: add some sysfs interfaces for powerplay.')
Signed-off-by Dan Carpenter <[email protected]>
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c +0 -1
index 589b36e..ce9e97f 100644
@@ -275,25 +275,23 @@ static ssize_t amdgpu_set_pp_force_state(struct device *dev,
 
 	if (strlen(buf) == 1)
 		adev->pp_force_state_enabled = false;
-	else {

drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c static ssize_t amdgpu_set_pp_force_state(struct device *dev, · line 285

Quoted 10 lines

drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c +4 -2

-		ret = kstrtol(buf, 0, &idx);
+	else if (adev->pp_enabled) {
+		struct pp_states_info data;
 
-		if (ret) {
+		ret = kstrtol(buf, 0, &idx);
+		if (ret || idx >= ARRAY_SIZE(data.states)) {
 			count = -EINVAL;
 			goto fail;
 		}

i would also expect a check idx < 0, does it mean this can not happen ? otherwise maybe kstrtoul is a solution ?

re,
wh

Quoted 28 lines

drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c +8 ~~-12~~

-		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:
--

To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Dan Carpenter

author

2016-06-16 07:54 UTC #2 lore raw

Quoted 25 lines · walter harms

Am 16.06.2016 08:41, schrieb Dan Carpenter:
Quoted 22 lines
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.
Fixes f3898ea12fc1 ('drm/amd/powerplay: add some sysfs interfaces for powerplay.')
Signed-off-by Dan Carpenter <[email protected]>
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c +2 -2
index 589b36e..ce9e97f 100644
@@ -275,25 +275,23 @@ static ssize_t amdgpu_set_pp_force_state(struct device *dev,
 
 	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;
 

Quoted 10 lines

drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c static ssize_t amdgpu_set_pp_force_state(struct device *dev, · line 285
Quoted 6 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c +2 -1
-		if (ret) {
+		ret = kstrtol(buf, 0, &idx);
+		if (ret || idx >= ARRAY_SIZE(data.states)) {
 			count = -EINVAL;
 			goto fail;
 		}
i would also expect a check idx < 0, does it mean this can not happen ? otherwise maybe kstrtoul is a solution ?

The original code could underflow, but my code can't. ARRAY_SIZE() means the comparison is type promoted to size_t which is unsigned long.

regards,
dan carpenter

Christian König

maintainer

2016-06-16 07:58 UTC #3 lore raw

Am 16.06.2016 um 09:54 schrieb Dan Carpenter:

Quoted 34 lines · walter harms

Quoted 24 lines
Am 16.06.2016 08:41, schrieb Dan Carpenter:
Quoted 22 lines
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.
Fixes f3898ea12fc1 ('drm/amd/powerplay: add some sysfs interfaces for powerplay.')
Signed-off-by Dan Carpenter <[email protected]>
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c +2 -2
index 589b36e..ce9e97f 100644
@@ -275,25 +275,23 @@ static ssize_t amdgpu_set_pp_force_state(struct device *dev,
 
  	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;
 
Quoted 9 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c static ssize_t amdgpu_set_pp_force_state(struct device *dev, · line 285
Quoted 6 lines
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c +2 -1
-		if (ret) {
+		ret = kstrtol(buf, 0, &idx);
+		if (ret || idx >= ARRAY_SIZE(data.states)) {
  			count = -EINVAL;
  			goto fail;
  		}
i would also expect a check idx < 0, does it mean this can not happen ? otherwise maybe kstrtoul is a solution ?

Quoted 2 lines

The original code could underflow, but my code can't. ARRAY_SIZE() means the comparison is type promoted to size_t which is unsigned long.

That's probably true, but not very obvious (not that I understand much of the power related code anyway).

Using kstrtoul() in the first place would make it a bit less obscure and probably generate a nice error code when somebody really tries to use a negative index here.

Cheers,
Christian.

Quoted 3 lines

regards,
dan carpenter

Dan Carpenter

author

2016-06-16 08:09 UTC #4 lore raw

Sure, I'll resend. Unsigned is a little cleaner. There is no new error code/message or change in behavior though either way.

regards,
dan carpenter

Dan Carpenter

author

2016-06-16 08:30 UTC #5 lore raw

PATCH v2 1/1 drm/amdgpu: missing bounds check in amdgpu_set_pp_force_state()

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 <[email protected]>

---
v2: make idx unsigned

drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c +13 ~~-15~~

index 589b36e..0e13d80 100644
@@ -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:

Christian König

maintainer Acked-by on v2 1/1

2016-06-16 08:39 UTC #6 lore raw

Am 16.06.2016 um 10:30 schrieb Dan Carpenter:

Quoted 8 lines

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 <[email protected]>

Acked-by Christian König <[email protected]>.

Quoted 41 lines

---
v2: make idx unsigned

drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c +6 ~~-15~~

index 589b36e..0e13d80 100644
@@ -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);

drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c static ssize_t amdgpu_set_pp_force_state(struct device *dev, · line 297

Quoted 10 lines

drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c +7 -0

+		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:

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Alex Deucher

Applied

2016-06-17 16:34 UTC #7 lore raw

Quoted 8 lines · Christian König

Am 16.06.2016 um 10:30 schrieb Dan Carpenter:
Quoted 7 lines
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.

Quoted 7 lines

Quoted 3 lines
Fixes f3898ea12fc1 ('drm/amd/powerplay: add some sysfs interfaces for
powerplay.')
Signed-off-by Dan Carpenter <[email protected]>
Acked-by Christian König <[email protected]>.

Applied. thanks!

Alex

Quoted 51 lines

---
v2: make idx unsigned

drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c +13 ~~-13~~

index 589b36e..0e13d80 100644
@@ -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;
                }

Quoted 8 lines

Quoted 2 lines
        }
fail:
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html