Re: [PATCH] drm/panthor: fix firmware control interface bounds checks

[Osama Abdelkader]

On Tue, Jul 28, 2026 at 04:12:53PM +0100, Liviu Dudau wrote:
> On Mon, Jul 20, 2026 at 03:44:35PM +0200, Osama Abdelkader wrote:
> > panthor_init_cs_iface() and panthor_init_csg_iface() validate firmware
> > control interface offsets with 32-bit arithmetic and the size of the host
> > wrapper structures. The offsets are derived from firmware-provided strides,
> > so the arithmetic can wrap before the bounds check, and the host wrapper
> > size is not the size of the firmware control interface being mapped.
> 
> How can the offsets wrap with the firmware-provided strides? It's not like
> the firmware provides arbitrarily large strides.
>

Thanks for the review. Yes, It's unlikely to happen but good to have,
these patches actually address issues reported by sashiko while reviewing
the first two patches regarding firmware sections with oversized data and
truncated firmware.

> > 
> > Use 64-bit arithmetic for the computed offsets and validate against the
> > actual firmware control interface structure sizes with subtraction-based
> > bounds checks. Also validate that the shared section is large enough for
> > the global control interface before using it.
> > 
> > Fixes: 2718d91816ee ("drm/panthor: Add the FW logical block")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Osama Abdelkader <osama.abdelkader@xxxxxxxxx>
> > ---
> >  drivers/gpu/drm/panthor/panthor_fw.c | 18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> > index eee2bc7e8541..e2fcbd639c3c 100644
> > --- a/drivers/gpu/drm/panthor/panthor_fw.c
> > +++ b/drivers/gpu/drm/panthor/panthor_fw.c
> > @@ -897,15 +897,16 @@ static int panthor_init_cs_iface(struct panthor_device *ptdev,
> >  	struct panthor_fw_csg_iface *csg_iface = panthor_fw_get_csg_iface(ptdev, csg_idx);
> >  	struct panthor_fw_cs_iface *cs_iface = &ptdev->fw->iface.streams[csg_idx][cs_idx];
> >  	u64 shared_section_sz = panthor_kernel_bo_size(ptdev->fw->shared_section->mem);
> > -	u32 iface_offset = CSF_GROUP_CONTROL_OFFSET +
> > -			   (csg_idx * glb_iface->control->group_stride) +
> > +	u64 iface_offset = CSF_GROUP_CONTROL_OFFSET +
> > +			   ((u64)csg_idx * glb_iface->control->group_stride) +
> >  			   CSF_STREAM_CONTROL_OFFSET +
> > -			   (cs_idx * csg_iface->control->stream_stride);
> > +			   ((u64)cs_idx * csg_iface->control->stream_stride);
> >  	struct panthor_fw_cs_iface *first_cs_iface =
> >  		panthor_fw_get_cs_iface(ptdev, 0, 0);
> >  
> > -	if (iface_offset + sizeof(*cs_iface) >= shared_section_sz)
> > +	if (iface_offset > shared_section_sz ||
> > +	    sizeof(*cs_iface->control) > shared_section_sz - iface_offset)
> >  		return -EINVAL;
> >  
> >  	spin_lock_init(&cs_iface->lock);
> >  	cs_iface->control = ptdev->fw->shared_section->mem->kmap + iface_offset;
> > @@ -955,11 +956,13 @@ static int panthor_init_csg_iface(struct panthor_device *ptdev,
> >  	struct panthor_fw_global_iface *glb_iface = panthor_fw_get_glb_iface(ptdev);
> >  	struct panthor_fw_csg_iface *csg_iface = &ptdev->fw->iface.groups[csg_idx];
> >  	u64 shared_section_sz = panthor_kernel_bo_size(ptdev->fw->shared_section->mem);
> > -	u32 iface_offset = CSF_GROUP_CONTROL_OFFSET + (csg_idx * glb_iface->control->group_stride);
> > +	u64 iface_offset = CSF_GROUP_CONTROL_OFFSET +
> > +			   ((u64)csg_idx * glb_iface->control->group_stride);
> >  	unsigned int i;
> >  
> > -	if (iface_offset + sizeof(*csg_iface) >= shared_section_sz)
> > +	if (iface_offset > shared_section_sz ||
> > +	    sizeof(*csg_iface->control) > shared_section_sz - iface_offset)
> >  		return -EINVAL;
> >  
> >  	spin_lock_init(&csg_iface->lock);
> >  	csg_iface->control = ptdev->fw->shared_section->mem->kmap + iface_offset;
> > @@ -1011,12 +1014,16 @@ static u32 panthor_get_instr_features(struct panthor_device *ptdev)
> >  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);
> >  	unsigned int i;
> >  
> >  	if (!ptdev->fw->shared_section->mem->kmap)
> >  		return -EINVAL;
> >  
> > +	if (sizeof(*glb_iface->control) > shared_section_sz)
> > +		return -EINVAL;
> > +
> >  	spin_lock_init(&glb_iface->lock);
> >  	glb_iface->control = ptdev->fw->shared_section->mem->kmap;
> >  
> >  	if (!glb_iface->control->version) {
> > -- 
> > 2.43.0
> > 
> 
> I'm OK with the general content of the patch, so:
> 
> Reviewed-by: Liviu Dudau <liviu.dudau@xxxxxxx>
> 
> Best regards,
> Liviu
> 
> 
> -- 
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯

Thank you.
Best regards,
Osama