[PATCH] drm/panthor: snapshot firmware interface counts before loops

[PATCH] drm/panthor: snapshot firmware interface counts before loops

[Osama Abdelkader]

The firmware exposes the global group count and per-group stream count in
the shared control interface. These values are validated before being used
as loop bounds, but the memory is shared with the MCU firmware and can be
changed after validation.

Read each count once with READ_ONCE() and use the validated snapshot as the
loop bound. This keeps the loop bounds consistent with the validation and
prevents the compiler from reloading a firmware-controlled count during
iteration.

Fixes: 2718d91816ee ("drm/panthor: Add the FW logical block")
Cc: stable@vger.kernel.org
Signed-off-by: Osama Abdelkader <osama.abdelkader@gmail.com>
---
 drivers/gpu/drm/panthor/panthor_fw.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
index e2fcbd639c3c..6e6da98d795e 100644
--- a/drivers/gpu/drm/panthor/panthor_fw.c
+++ b/drivers/gpu/drm/panthor/panthor_fw.c
@@ -959,6 +959,7 @@ static int panthor_init_csg_iface(struct panthor_device *ptdev,
 	u64 shared_section_sz = panthor_kernel_bo_size(ptdev->fw->shared_section->mem);
 	u64 iface_offset = CSF_GROUP_CONTROL_OFFSET +
 			   ((u64)csg_idx * glb_iface->control->group_stride);
+	u32 stream_num;
 	unsigned int i;
 
 	if (iface_offset > shared_section_sz ||
@@ -972,8 +973,8 @@ static int panthor_init_csg_iface(struct panthor_device *ptdev,
 	csg_iface->output = iface_fw_to_cpu_addr(ptdev, csg_iface->control->output_va,
 						 sizeof(*csg_iface->output));
 
-	if (csg_iface->control->stream_num < MIN_CS_PER_CSG ||
-	    csg_iface->control->stream_num > MAX_CS_PER_CSG)
+	stream_num = READ_ONCE(csg_iface->control->stream_num);
+	if (stream_num < MIN_CS_PER_CSG || stream_num > MAX_CS_PER_CSG)
 		return -EINVAL;
 
 	if (!csg_iface->input || !csg_iface->output) {
@@ -990,7 +991,7 @@ static int panthor_init_csg_iface(struct panthor_device *ptdev,
 		}
 	}
 
-	for (i = 0; i < csg_iface->control->stream_num; i++) {
+	for (i = 0; i < stream_num; i++) {
 		int ret = panthor_init_cs_iface(ptdev, csg_idx, i);
 
 		if (ret)
@@ -1015,6 +1016,7 @@ static int panthor_fw_init_ifaces(struct panthor_device *ptdev)
 {
 	struct panthor_fw_global_iface *glb_iface = &ptdev->fw->iface.global;
 	u64 shared_section_sz = panthor_kernel_bo_size(ptdev->fw->shared_section->mem);
+	u32 group_num;
 	unsigned int i;
 
 	if (!ptdev->fw->shared_section->mem->kmap)
@@ -1034,17 +1036,17 @@ static int panthor_fw_init_ifaces(struct panthor_device *ptdev)
 		return -EINVAL;
 	}
 
-	if (glb_iface->control->group_num > MAX_CSGS ||
-	    glb_iface->control->group_num < MIN_CSGS) {
+	group_num = READ_ONCE(glb_iface->control->group_num);
+	if (group_num > MAX_CSGS || group_num < MIN_CSGS) {
 		drm_err(&ptdev->base, "Invalid number of control groups");
 		return -EINVAL;
 	}
 
-	for (i = 0; i < glb_iface->control->group_num; i++) {
+	for (i = 0; i < group_num; i++) {
 		int ret = panthor_init_csg_iface(ptdev, i);
 
 		if (ret)
 			return ret;
 	}
 
 	drm_info(&ptdev->base, "CSF FW using interface v%d.%d.%d, Features %#x Instrumentation features %#x",
-- 
2.43.0

Re: [PATCH] drm/panthor: snapshot firmware interface counts before loops

[Liviu Dudau]

On Mon, Jul 20, 2026 at 03:55:18PM +0200, Osama Abdelkader wrote:
> The firmware exposes the global group count and per-group stream count in
> the shared control interface. These values are validated before being used
> as loop bounds, but the memory is shared with the MCU firmware and can be
> changed after validation.

Again, you're making statements that need to be proven here. How is MCU
firmware going to change the content of the memory shared with the driver?

> 
> Read each count once with READ_ONCE() and use the validated snapshot as the
> loop bound. This keeps the loop bounds consistent with the validation and
> prevents the compiler from reloading a firmware-controlled count during
> iteration.

As an optimisation I'm fine with it, so:

Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>

Best regards,
Liviu

> 
> Fixes: 2718d91816ee ("drm/panthor: Add the FW logical block")
> Cc: stable@vger.kernel.org
> Signed-off-by: Osama Abdelkader <osama.abdelkader@gmail.com>
> ---
>  drivers/gpu/drm/panthor/panthor_fw.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> index e2fcbd639c3c..6e6da98d795e 100644
> --- a/drivers/gpu/drm/panthor/panthor_fw.c
> +++ b/drivers/gpu/drm/panthor/panthor_fw.c
> @@ -959,6 +959,7 @@ static int panthor_init_csg_iface(struct panthor_device *ptdev,
>  	u64 shared_section_sz = panthor_kernel_bo_size(ptdev->fw->shared_section->mem);
>  	u64 iface_offset = CSF_GROUP_CONTROL_OFFSET +
>  			   ((u64)csg_idx * glb_iface->control->group_stride);
> +	u32 stream_num;
>  	unsigned int i;
>  
>  	if (iface_offset > shared_section_sz ||
> @@ -972,8 +973,8 @@ static int panthor_init_csg_iface(struct panthor_device *ptdev,
>  	csg_iface->output = iface_fw_to_cpu_addr(ptdev, csg_iface->control->output_va,
>  						 sizeof(*csg_iface->output));
>  
> -	if (csg_iface->control->stream_num < MIN_CS_PER_CSG ||
> -	    csg_iface->control->stream_num > MAX_CS_PER_CSG)
> +	stream_num = READ_ONCE(csg_iface->control->stream_num);
> +	if (stream_num < MIN_CS_PER_CSG || stream_num > MAX_CS_PER_CSG)
>  		return -EINVAL;
>  
>  	if (!csg_iface->input || !csg_iface->output) {
> @@ -990,7 +991,7 @@ static int panthor_init_csg_iface(struct panthor_device *ptdev,
>  		}
>  	}
>  
> -	for (i = 0; i < csg_iface->control->stream_num; i++) {
> +	for (i = 0; i < stream_num; i++) {
>  		int ret = panthor_init_cs_iface(ptdev, csg_idx, i);
>  
>  		if (ret)
> @@ -1015,6 +1016,7 @@ static int panthor_fw_init_ifaces(struct panthor_device *ptdev)
>  {
>  	struct panthor_fw_global_iface *glb_iface = &ptdev->fw->iface.global;
>  	u64 shared_section_sz = panthor_kernel_bo_size(ptdev->fw->shared_section->mem);
> +	u32 group_num;
>  	unsigned int i;
>  
>  	if (!ptdev->fw->shared_section->mem->kmap)
> @@ -1034,17 +1036,17 @@ static int panthor_fw_init_ifaces(struct panthor_device *ptdev)
>  		return -EINVAL;
>  	}
>  
> -	if (glb_iface->control->group_num > MAX_CSGS ||
> -	    glb_iface->control->group_num < MIN_CSGS) {
> +	group_num = READ_ONCE(glb_iface->control->group_num);
> +	if (group_num > MAX_CSGS || group_num < MIN_CSGS) {
>  		drm_err(&ptdev->base, "Invalid number of control groups");
>  		return -EINVAL;
>  	}
>  
> -	for (i = 0; i < glb_iface->control->group_num; i++) {
> +	for (i = 0; i < group_num; i++) {
>  		int ret = panthor_init_csg_iface(ptdev, i);
>  
>  		if (ret)
>  			return ret;
>  	}
>  
>  	drm_info(&ptdev->base, "CSF FW using interface v%d.%d.%d, Features %#x Instrumentation features %#x",
> -- 
> 2.43.0
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH] drm/panthor: snapshot firmware interface counts before loops

[Osama Abdelkader]

On Tue, Jul 28, 2026 at 04:17:14PM +0100, Liviu Dudau wrote:
> On Mon, Jul 20, 2026 at 03:55:18PM +0200, Osama Abdelkader wrote:
> > The firmware exposes the global group count and per-group stream count in
> > the shared control interface. These values are validated before being used
> > as loop bounds, but the memory is shared with the MCU firmware and can be
> > changed after validation.
> 
> Again, you're making statements that need to be proven here. How is MCU
> firmware going to change the content of the memory shared with the driver?
> 

Thanks for the review. yes, this was flagged by Sashiko while reviewing the 
previous patch, but I agree the commit message shouldn't present it as a proven
firmware mutation bug.

The useful part here is just making the validated count the same value used
as the loop bound. since It's an optimisation, should I send v2 without Fixes
and Cc stable tags?

> > 
> > Read each count once with READ_ONCE() and use the validated snapshot as the
> > loop bound. This keeps the loop bounds consistent with the validation and
> > prevents the compiler from reloading a firmware-controlled count during
> > iteration.
> 
> As an optimisation I'm fine with it, so:
> 
> Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
> 
> Best regards,
> Liviu
> 

Best regards,
Osama
> > 
> > Fixes: 2718d91816ee ("drm/panthor: Add the FW logical block")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Osama Abdelkader <osama.abdelkader@gmail.com>
> > ---
> >  drivers/gpu/drm/panthor/panthor_fw.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> > index e2fcbd639c3c..6e6da98d795e 100644
> > --- a/drivers/gpu/drm/panthor/panthor_fw.c
> > +++ b/drivers/gpu/drm/panthor/panthor_fw.c
> > @@ -959,6 +959,7 @@ static int panthor_init_csg_iface(struct panthor_device *ptdev,
> >  	u64 shared_section_sz = panthor_kernel_bo_size(ptdev->fw->shared_section->mem);
> >  	u64 iface_offset = CSF_GROUP_CONTROL_OFFSET +
> >  			   ((u64)csg_idx * glb_iface->control->group_stride);
> > +	u32 stream_num;
> >  	unsigned int i;
> >  
> >  	if (iface_offset > shared_section_sz ||
> > @@ -972,8 +973,8 @@ static int panthor_init_csg_iface(struct panthor_device *ptdev,
> >  	csg_iface->output = iface_fw_to_cpu_addr(ptdev, csg_iface->control->output_va,
> >  						 sizeof(*csg_iface->output));
> >  
> > -	if (csg_iface->control->stream_num < MIN_CS_PER_CSG ||
> > -	    csg_iface->control->stream_num > MAX_CS_PER_CSG)
> > +	stream_num = READ_ONCE(csg_iface->control->stream_num);
> > +	if (stream_num < MIN_CS_PER_CSG || stream_num > MAX_CS_PER_CSG)
> >  		return -EINVAL;
> >  
> >  	if (!csg_iface->input || !csg_iface->output) {
> > @@ -990,7 +991,7 @@ static int panthor_init_csg_iface(struct panthor_device *ptdev,
> >  		}
> >  	}
> >  
> > -	for (i = 0; i < csg_iface->control->stream_num; i++) {
> > +	for (i = 0; i < stream_num; i++) {
> >  		int ret = panthor_init_cs_iface(ptdev, csg_idx, i);
> >  
> >  		if (ret)
> > @@ -1015,6 +1016,7 @@ static int panthor_fw_init_ifaces(struct panthor_device *ptdev)
> >  {
> >  	struct panthor_fw_global_iface *glb_iface = &ptdev->fw->iface.global;
> >  	u64 shared_section_sz = panthor_kernel_bo_size(ptdev->fw->shared_section->mem);
> > +	u32 group_num;
> >  	unsigned int i;
> >  
> >  	if (!ptdev->fw->shared_section->mem->kmap)
> > @@ -1034,17 +1036,17 @@ static int panthor_fw_init_ifaces(struct panthor_device *ptdev)
> >  		return -EINVAL;
> >  	}
> >  
> > -	if (glb_iface->control->group_num > MAX_CSGS ||
> > -	    glb_iface->control->group_num < MIN_CSGS) {
> > +	group_num = READ_ONCE(glb_iface->control->group_num);
> > +	if (group_num > MAX_CSGS || group_num < MIN_CSGS) {
> >  		drm_err(&ptdev->base, "Invalid number of control groups");
> >  		return -EINVAL;
> >  	}
> >  
> > -	for (i = 0; i < glb_iface->control->group_num; i++) {
> > +	for (i = 0; i < group_num; i++) {
> >  		int ret = panthor_init_csg_iface(ptdev, i);
> >  
> >  		if (ret)
> >  			return ret;
> >  	}
> >  
> >  	drm_info(&ptdev->base, "CSF FW using interface v%d.%d.%d, Features %#x Instrumentation features %#x",
> > -- 
> > 2.43.0
> > 
> 
> -- 
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯