[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!  /
>   ---------------
>     ¯\_(ツ)_/¯

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

[Steven Price]

On 29/07/2026 13:00, Osama Abdelkader wrote:
> 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?

I think the issue here is that Sashiko is thinking the firmware isn't
trusted. And there's some truth to that - we do perform validation of
the firmware binary and sanity check some of the values it writes. But
this is more as a robustness check rather than a real trust boundary.
The firmware changing values under the kernel's feet is more than just a
robustness bug, it's only really likely to happen with an actually
malicious firmware.

But the actual change is reasonable, as Liviu says it could even be a
(very minor) optimisation. So if you could send a v2 with a reworded
commit message then feel free to add:

Reviewed-by: Steven Price <steven.price@arm.com>

Thanks,
Steve

>>>
>>> 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!  /
>>   ---------------
>>     ¯\_(ツ)_/¯

[PATCH v2] drm/panthor: use local variables for firmware interface counts

[Osama Abdelkader]

panthor_fw_init_ifaces() validates the firmware interface group count
before iterating over the CSG interfaces. panthor_init_csg_iface() does
the same for the per-group stream count before iterating over the CS
interfaces.

Store those validated counts in local variables and use the locals as the
loop bounds. This avoids reading the same control interface fields twice
and makes it explicit that the loops use the values that were just
validated.

Signed-off-by: Osama Abdelkader <osama.abdelkader@gmail.com>
Reviewed-by: Steven Price <steven.price@arm.com>
---
v2:
- Reword as an optimization/cleanup instead of a firmware trust-boundary fix.
- Drop the Fixes and stable tags.

 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 v2] drm/panthor: use local variables for firmware interface counts

[Boris Brezillon]

On Mon,  3 Aug 2026 14:48:06 +0200
Osama Abdelkader <osama.abdelkader@gmail.com> wrote:

> panthor_fw_init_ifaces() validates the firmware interface group count
> before iterating over the CSG interfaces. panthor_init_csg_iface() does
> the same for the per-group stream count before iterating over the CS
> interfaces.
> 
> Store those validated counts in local variables and use the locals as the
> loop bounds. This avoids reading the same control interface fields twice
> and makes it explicit that the loops use the values that were just
> validated.
> 
> Signed-off-by: Osama Abdelkader <osama.abdelkader@gmail.com>
> Reviewed-by: Steven Price <steven.price@arm.com>
> ---
> v2:
> - Reword as an optimization/cleanup instead of a firmware trust-boundary fix.
> - Drop the Fixes and stable tags.
> 
>  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);

We need a comment to explain the READ_ONCE(), otherwise new readers
(and given my inability to remember things, I consider myself a new
reader after 2 weeks :-)) will keep wondering why we're forcing the
compiler to read the memory only once. IIUC, that's here to protect
against self-modifying control sections, so maybe say that. Or if we
consider that the FW is trusted/sure, drop the READ_ONCE()...

> +	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",

[PATCH v3] drm/panthor: use local variables for firmware interface counts

[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.

Read each count once with READ_ONCE() and store it in a local variable.
This avoids reading the same control section field twice and makes it
explicit that the loop uses the same value that passed validation
to protect against self-modifying control sections.

Signed-off-by: Osama Abdelkader <osama.abdelkader@gmail.com>
Reviewed-by: Steven Price <steven.price@arm.com>
---
v3:
- Add comments explaining the READ_ONCE() usage.

 drivers/gpu/drm/panthor/panthor_fw.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
index 8411c468bdac..b2ccb81f280c 100644
--- a/drivers/gpu/drm/panthor/panthor_fw.c
+++ b/drivers/gpu/drm/panthor/panthor_fw.c
@@ -954,6 +954,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 ||
@@ -967,8 +968,13 @@ 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)
+	/*
+	 * To protect against self-modifying control sections
+	 * take a single snapshot from the control section so validation and
+	 * iteration use the same value.
+	 */
+	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) {
@@ -986,7 +992,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)
@@ -1010,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)
@@ -1035,13 +1042,18 @@ 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) {
+	/*
+	 * To protect against self-modifying control sections
+	 * take a single snapshot from the control section so validation and
+	 * iteration use the same value.
+	 */
+	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)
-- 
2.43.0

Re: [PATCH v3] drm/panthor: use local variables for firmware interface counts

[Liviu Dudau]

On Mon, Aug 03, 2026 at 04:11:49PM +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.
> 
> Read each count once with READ_ONCE() and store it in a local variable.
> This avoids reading the same control section field twice and makes it
> explicit that the loop uses the same value that passed validation
> to protect against self-modifying control sections.
> 
> Signed-off-by: Osama Abdelkader <osama.abdelkader@gmail.com>
> Reviewed-by: Steven Price <steven.price@arm.com>

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

Best regards,
Liviu

> ---
> v3:
> - Add comments explaining the READ_ONCE() usage.
> 
>  drivers/gpu/drm/panthor/panthor_fw.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> index 8411c468bdac..b2ccb81f280c 100644
> --- a/drivers/gpu/drm/panthor/panthor_fw.c
> +++ b/drivers/gpu/drm/panthor/panthor_fw.c
> @@ -954,6 +954,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 ||
> @@ -967,8 +968,13 @@ 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)
> +	/*
> +	 * To protect against self-modifying control sections
> +	 * take a single snapshot from the control section so validation and
> +	 * iteration use the same value.
> +	 */
> +	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) {
> @@ -986,7 +992,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)
> @@ -1010,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)
> @@ -1035,13 +1042,18 @@ 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) {
> +	/*
> +	 * To protect against self-modifying control sections
> +	 * take a single snapshot from the control section so validation and
> +	 * iteration use the same value.
> +	 */
> +	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)
> -- 
> 2.43.0
> 

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