Nouveau - Mar 2023 - [PATCH 0/2] drm/nouveau: avoid usage of list iterator after loop

This patch set includes two instances where the list iterator variable
'pstate' is implicitly assumed to be valid after the iterator loop.
While in pratice that is most likely the case (if
'pstatei'/'args->v0.state' is <= the elements in
clk->states), we should
explicitly only allow 'pstate' to always point to correct
'nvkm_pstate'
structs.

That allows catching potential bugs with BUG_ON(!pstate) that otherwise
would be completely undetectable.

It also helps the greater mission to hopefully move the list iterator
variable into the iterating macro directly [1].

Link:
https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w
at mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <jkl820.git at gmail.com>
---
Jakob Koschel (2):
      drm/nouveau/device: avoid usage of list iterator after loop
      drm/nouveau/clk: avoid usage of list iterator after loop

 drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c | 9 ++++++---
 drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c    | 9 ++++++---
 2 files changed, 12 insertions(+), 6 deletions(-)
---
base-commit: c0927a7a5391f7d8e593e5e50ead7505a23cadf9
change-id: 20230301-drm-nouveau-avoid-iter-after-loop-4bff97166efa

Best regards,
-- 
Jakob Koschel <jkl820.git at gmail.com>

If potentially no valid element is found, 'pstate' would contain an
invalid pointer past the iterator loop. To ensure 'pstate' is always
valid, we only set it if the correct element was found. That allows
adding a BUG_ON in case the code works incorrectly, exposing currently
undetectable potential bugs.

Additionally, Linus proposed to avoid any use of the list iterator
variable after the loop, in the attempt to move the list iterator
variable declaration into the marcro to avoid any potential misuse after
the loop [1].

Link:
https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w
at mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <jkl820.git at gmail.com>
---
 drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c
b/drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c
index ce774579c89d..7c9dd91e98ee 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c
@@ -72,7 +72,7 @@ nvkm_control_mthd_pstate_attr(struct nvkm_control *ctrl, void
*data, u32 size)
 	} *args = data;
 	struct nvkm_clk *clk = ctrl->device->clk;
 	const struct nvkm_domain *domain;
-	struct nvkm_pstate *pstate;
+	struct nvkm_pstate *pstate = NULL, *iter;
 	struct nvkm_cstate *cstate;
 	int i = 0, j = -1;
 	u32 lo, hi;
@@ -103,11 +103,14 @@ nvkm_control_mthd_pstate_attr(struct nvkm_control *ctrl,
void *data, u32 size)
 		return -EINVAL;
 
 	if (args->v0.state != NVIF_CONTROL_PSTATE_ATTR_V0_STATE_CURRENT) {
-		list_for_each_entry(pstate, &clk->states, head) {
-			if (i++ == args->v0.state)
+		list_for_each_entry(iter, &clk->states, head) {
+			if (i++ == args->v0.state) {
+				pstate = iter;
 				break;
+			}
 		}
 
+		BUG_ON(!pstate);
 		lo = pstate->base.domain[domain->name];
 		hi = lo;
 		list_for_each_entry(cstate, &pstate->list, head) {

-- 
2.34.1

If potentially no valid element is found, 'pstate' would contain an
invalid pointer past the iterator loop. To ensure 'pstate' is always
valid, we only set it if the correct element was found. That allows
adding a BUG_ON in case the code works incorrectly, exposing currently
undetectable potential bugs.

Additionally, Linus proposed to avoid any use of the list iterator
variable after the loop, in the attempt to move the list iterator
variable declaration into the marcro to avoid any potential misuse after
the loop [1].

Link:
https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w
at mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <jkl820.git at gmail.com>
---
 drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c
index da07a2fbef06..871127dfe1d7 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c
@@ -269,14 +269,17 @@ nvkm_pstate_prog(struct nvkm_clk *clk, int pstatei)
 	struct nvkm_subdev *subdev = &clk->subdev;
 	struct nvkm_fb *fb = subdev->device->fb;
 	struct nvkm_pci *pci = subdev->device->pci;
-	struct nvkm_pstate *pstate;
+	struct nvkm_pstate *pstate = NULL, *iter;
 	int ret, idx = 0;
 
-	list_for_each_entry(pstate, &clk->states, head) {
-		if (idx++ == pstatei)
+	list_for_each_entry(iter, &clk->states, head) {
+		if (idx++ == pstatei) {
+			pstate = iter;
 			break;
+		}
 	}
 
+	BUG_ON(!pstate);
 	nvkm_debug(subdev, "setting performance state %d\n", pstatei);
 	clk->pstate = pstatei;
 

-- 
2.34.1

Reviewed-by: Lyude Paul <lyude at redhat.com>

Will push upstream in just a moment

On Wed, 2023-03-01 at 18:25 +0100, Jakob Koschel wrote:
> This patch set includes two instances where the list iterator variable
> 'pstate' is implicitly assumed to be valid after the iterator loop.
> While in pratice that is most likely the case (if
> 'pstatei'/'args->v0.state' is <= the elements in
clk->states), we should
> explicitly only allow 'pstate' to always point to correct
'nvkm_pstate'
> structs.
> 
> That allows catching potential bugs with BUG_ON(!pstate) that otherwise
> would be completely undetectable.
> 
> It also helps the greater mission to hopefully move the list iterator
> variable into the iterating macro directly [1].
> 
> Link:
https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w
at mail.gmail.com/ [1]
> Signed-off-by: Jakob Koschel <jkl820.git at gmail.com>
> ---
> Jakob Koschel (2):
>       drm/nouveau/device: avoid usage of list iterator after loop
>       drm/nouveau/clk: avoid usage of list iterator after loop
> 
>  drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c | 9 ++++++---
>  drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c    | 9 ++++++---
>  2 files changed, 12 insertions(+), 6 deletions(-)
> ---
> base-commit: c0927a7a5391f7d8e593e5e50ead7505a23cadf9
> change-id: 20230301-drm-nouveau-avoid-iter-after-loop-4bff97166efa
> 
> Best regards,
-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

On Wed, 2023-03-01 at 18:25 +0100, Jakob Koschel wrote:
> If potentially no valid element is found, 'pstate' would contain an
> invalid pointer past the iterator loop. To ensure 'pstate' is
always
> valid, we only set it if the correct element was found. That allows
> adding a BUG_ON in case the code works incorrectly, exposing currently
> undetectable potential bugs.
> 
> Additionally, Linus proposed to avoid any use of the list iterator
> variable after the loop, in the attempt to move the list iterator
> variable declaration into the marcro to avoid any potential misuse after
> the loop [1].
> 
> Link:
https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w
at mail.gmail.com/ [1]
> Signed-off-by: Jakob Koschel <jkl820.git at gmail.com>
> ---
>  drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c
b/drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c
> index ce774579c89d..7c9dd91e98ee 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c
> @@ -72,7 +72,7 @@ nvkm_control_mthd_pstate_attr(struct nvkm_control *ctrl,
void *data, u32 size)
>  	} *args = data;
>  	struct nvkm_clk *clk = ctrl->device->clk;
>  	const struct nvkm_domain *domain;
> -	struct nvkm_pstate *pstate;
> +	struct nvkm_pstate *pstate = NULL, *iter;
>  	struct nvkm_cstate *cstate;
>  	int i = 0, j = -1;
>  	u32 lo, hi;
> @@ -103,11 +103,14 @@ nvkm_control_mthd_pstate_attr(struct nvkm_control
*ctrl, void *data, u32 size)
>  		return -EINVAL;
>  
>  	if (args->v0.state != NVIF_CONTROL_PSTATE_ATTR_V0_STATE_CURRENT) {
> -		list_for_each_entry(pstate, &clk->states, head) {
> -			if (i++ == args->v0.state)
> +		list_for_each_entry(iter, &clk->states, head) {
> +			if (i++ == args->v0.state) {
> +				pstate = iter;
>  				break;
> +			}
>  		}
>  
> +		BUG_ON(!pstate);
Let's replace this with

		if (WARN_ON_ONCE(!pstate))
			return -EINVAL;
>  		lo = pstate->base.domain[domain->name];
>  		hi = lo;
>  		list_for_each_entry(cstate, &pstate->list, head) {
> 
-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

On Wed, 2023-03-01 at 18:25 +0100, Jakob Koschel wrote:
> +		}
> ?	}
> ?
> +	BUG_ON(!pstate);
> ?	nvkm_debug(subdev, "setting performance state %d\n", pstatei);
> ?	clk->pstate = pstatei;
We should probably also replace this with

	if (WARN_ON(!pstate)
		return -EINVAL;

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat