[RFT,v2,1/5] drm: Split framebuffer pixel offset calculation from drm_fb_dma_get_gem_addr()

Currently drm_fb_dma_get_gem_addr() calculates the offset into the
framebuffer memory for the framebuffer's unclipped source coordinates,
adds that to the framebuffer's backing storage, and returns the result.

We are about to add a variant that uses the clipped source coordinates,
so there is already some reuse of code. However, calculating the data
offset for a given pixel is not specific to the DMA FB helpers. The
offset is only related to the framebuffer.

Split out the offset calculation into a new framebuffer helper so that
non-DMA users can also reuse the same code.

Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: <stable@vger.kernel.org> # dependency for next patch
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
Changes since v1:
- New patch
---
 drivers/gpu/drm/drm_fb_dma_helper.c | 28 ++----------------
 drivers/gpu/drm/drm_framebuffer.c   | 45 +++++++++++++++++++++++++++++
 include/drm/drm_framebuffer.h       |  3 ++
 3 files changed, 51 insertions(+), 25 deletions(-)
  

Hi

Am 16.09.26 um 05:33 schrieb Chen-Yu Tsai:
> Currently drm_fb_dma_get_gem_addr() calculates the offset into the
> framebuffer memory for the framebuffer's unclipped source coordinates,
> adds that to the framebuffer's backing storage, and returns the result.
>
> We are about to add a variant that uses the clipped source coordinates,
> so there is already some reuse of code. However, calculating the data
> offset for a given pixel is not specific to the DMA FB helpers. The
> offset is only related to the framebuffer.
>
> Split out the offset calculation into a new framebuffer helper so that
> non-DMA users can also reuse the same code.
>
> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: <stable@vger.kernel.org> # dependency for next patch
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
> Changes since v1:
> - New patch
> ---
>   drivers/gpu/drm/drm_fb_dma_helper.c | 28 ++----------------
>   drivers/gpu/drm/drm_framebuffer.c   | 45 +++++++++++++++++++++++++++++
>   include/drm/drm_framebuffer.h       |  3 ++
>   3 files changed, 51 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_dma_helper.c b/drivers/gpu/drm/drm_fb_dma_helper.c
> index fd71969d2fb1..ab0f37d8a5ff 100644
> --- a/drivers/gpu/drm/drm_fb_dma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_dma_helper.c
> @@ -75,36 +75,14 @@ dma_addr_t drm_fb_dma_get_gem_addr(struct drm_framebuffer *fb,
>   				   unsigned int plane)
>   {
>   	struct drm_gem_dma_object *obj;
> -	dma_addr_t dma_addr;
> -	u8 h_div = 1, v_div = 1;
> -	u32 block_w = drm_format_info_block_width(fb->format, plane);
> -	u32 block_h = drm_format_info_block_height(fb->format, plane);
> -	u32 block_size = fb->format->char_per_block[plane];
> -	u32 sample_x;
> -	u32 sample_y;
> -	u32 block_start_y;
> -	u32 num_hblocks;
>   
>   	obj = drm_fb_dma_get_gem_obj(fb, plane);
>   	if (!obj)
>   		return 0;
>   
> -	dma_addr = obj->dma_addr + fb->offsets[plane];
> -
> -	if (plane > 0) {
> -		h_div = fb->format->hsub;
> -		v_div = fb->format->vsub;
> -	}
> -
> -	sample_x = (state->src_x >> 16) / h_div;
> -	sample_y = (state->src_y >> 16) / v_div;
> -	block_start_y = (sample_y / block_h) * block_h;
> -	num_hblocks = sample_x / block_w;
> -
> -	dma_addr += fb->pitches[plane] * block_start_y;
> -	dma_addr += block_size * num_hblocks;
> -
> -	return dma_addr;
> +	return obj->dma_addr + drm_framebuffer_get_block_offset(fb, plane,
> +								state->src_x >> 16,
> +								state->src_y >> 16);
>   }
>   EXPORT_SYMBOL_GPL(drm_fb_dma_get_gem_addr);
>   
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index d32aceb6ca9b..9e1231162047 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -1208,6 +1208,51 @@ void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,
>   	}
>   }
>   
> +/**
> + * drm_framebuffer_get_block_offset() - Get offset to start of pixel block for
> + * the given framebuffer and coordinates.
> + * @fb: The framebuffer
> + * @plane: Which plane
> + * @x: x coordinate for pixel
> + * @y: y coordinate for pixel
> + *
> + * This function will usually be called from the PLANE callback functions,
> + * or from one of the helpers that calculates the framebuffer's DMA address.
> + *
> + * Return: offset from start of framebuffer to start of pixel block
> + */
> +u32 drm_framebuffer_get_block_offset(struct drm_framebuffer *fb, unsigned int plane,
> +				     unsigned int x, unsigned int y)

Better use u64 as return type.

> +{
> +	u8 h_div = 1, v_div = 1;
> +	u32 block_w = drm_format_info_block_width(fb->format, plane);
> +	u32 block_h = drm_format_info_block_height(fb->format, plane);
> +	u32 block_size = fb->format->char_per_block[plane];
> +	u32 sample_x;
> +	u32 sample_y;
> +	u32 block_start_y;
> +	u32 num_hblocks;
> +	u32 offset;
> +
> +	offset = fb->offsets[plane];
> +
> +	if (plane > 0) {
> +		h_div = fb->format->hsub;
> +		v_div = fb->format->vsub;
> +	}
> +
> +	sample_x = x / h_div;
> +	sample_y = y / v_div;
> +	block_start_y = (sample_y / block_h) * block_h;
> +	num_hblocks = sample_x / block_w;
> +
> +	offset += fb->pitches[plane] * block_start_y;
> +	offset += block_size * num_hblocks;

User space controls the values in fb->offsets and fb->pitches.  I'm not 
sure how well they have been validated already at this point. Did you 
investigate this?

Best regards
Thomas


> +
> +	return offset;
> +}
> +EXPORT_SYMBOL(drm_framebuffer_get_block_offset);
> +
>   #ifdef CONFIG_DEBUG_FS
>   static int drm_framebuffer_info(struct seq_file *m, void *data)
>   {
> diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
> index 38b24fc8978d..c07aea1cc59f 100644
> --- a/include/drm/drm_framebuffer.h
> +++ b/include/drm/drm_framebuffer.h
> @@ -220,6 +220,9 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb);
>   void drm_framebuffer_cleanup(struct drm_framebuffer *fb);
>   void drm_framebuffer_unregister_private(struct drm_framebuffer *fb);
>   
> +u32 drm_framebuffer_get_block_offset(struct drm_framebuffer *fb, unsigned int plane,
> +				     unsigned int x, unsigned int y);
> +
>   /**
>    * drm_framebuffer_get - acquire a framebuffer reference
>    * @fb: DRM framebuffer
  

On Thu, Sep 17, 2026 at 11:20 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 16.09.26 um 05:33 schrieb Chen-Yu Tsai:
> > Currently drm_fb_dma_get_gem_addr() calculates the offset into the
> > framebuffer memory for the framebuffer's unclipped source coordinates,
> > adds that to the framebuffer's backing storage, and returns the result.
> >
> > We are about to add a variant that uses the clipped source coordinates,
> > so there is already some reuse of code. However, calculating the data
> > offset for a given pixel is not specific to the DMA FB helpers. The
> > offset is only related to the framebuffer.
> >
> > Split out the offset calculation into a new framebuffer helper so that
> > non-DMA users can also reuse the same code.
> >
> > Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: <stable@vger.kernel.org> # dependency for next patch
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> > Changes since v1:
> > - New patch
> > ---
> >   drivers/gpu/drm/drm_fb_dma_helper.c | 28 ++----------------
> >   drivers/gpu/drm/drm_framebuffer.c   | 45 +++++++++++++++++++++++++++++
> >   include/drm/drm_framebuffer.h       |  3 ++
> >   3 files changed, 51 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_fb_dma_helper.c b/drivers/gpu/drm/drm_fb_dma_helper.c
> > index fd71969d2fb1..ab0f37d8a5ff 100644
> > --- a/drivers/gpu/drm/drm_fb_dma_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_dma_helper.c
> > @@ -75,36 +75,14 @@ dma_addr_t drm_fb_dma_get_gem_addr(struct drm_framebuffer *fb,
> >                                  unsigned int plane)
> >   {
> >       struct drm_gem_dma_object *obj;
> > -     dma_addr_t dma_addr;
> > -     u8 h_div = 1, v_div = 1;
> > -     u32 block_w = drm_format_info_block_width(fb->format, plane);
> > -     u32 block_h = drm_format_info_block_height(fb->format, plane);
> > -     u32 block_size = fb->format->char_per_block[plane];
> > -     u32 sample_x;
> > -     u32 sample_y;
> > -     u32 block_start_y;
> > -     u32 num_hblocks;
> >
> >       obj = drm_fb_dma_get_gem_obj(fb, plane);
> >       if (!obj)
> >               return 0;
> >
> > -     dma_addr = obj->dma_addr + fb->offsets[plane];
> > -
> > -     if (plane > 0) {
> > -             h_div = fb->format->hsub;
> > -             v_div = fb->format->vsub;
> > -     }
> > -
> > -     sample_x = (state->src_x >> 16) / h_div;
> > -     sample_y = (state->src_y >> 16) / v_div;
> > -     block_start_y = (sample_y / block_h) * block_h;
> > -     num_hblocks = sample_x / block_w;
> > -
> > -     dma_addr += fb->pitches[plane] * block_start_y;
> > -     dma_addr += block_size * num_hblocks;
> > -
> > -     return dma_addr;
> > +     return obj->dma_addr + drm_framebuffer_get_block_offset(fb, plane,
> > +                                                             state->src_x >> 16,
> > +                                                             state->src_y >> 16);
> >   }
> >   EXPORT_SYMBOL_GPL(drm_fb_dma_get_gem_addr);
> >
> > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> > index d32aceb6ca9b..9e1231162047 100644
> > --- a/drivers/gpu/drm/drm_framebuffer.c
> > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > @@ -1208,6 +1208,51 @@ void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,
> >       }
> >   }
> >
> > +/**
> > + * drm_framebuffer_get_block_offset() - Get offset to start of pixel block for
> > + * the given framebuffer and coordinates.
> > + * @fb: The framebuffer
> > + * @plane: Which plane
> > + * @x: x coordinate for pixel
> > + * @y: y coordinate for pixel
> > + *
> > + * This function will usually be called from the PLANE callback functions,
> > + * or from one of the helpers that calculates the framebuffer's DMA address.
> > + *
> > + * Return: offset from start of framebuffer to start of pixel block
> > + */
> > +u32 drm_framebuffer_get_block_offset(struct drm_framebuffer *fb, unsigned int plane,
> > +                                  unsigned int x, unsigned int y)
>
> Better use u64 as return type.

To avoid overflow? Not sure who would use crazy large framebuffers, but
doesn't hurt to play it safe.

> > +{
> > +     u8 h_div = 1, v_div = 1;
> > +     u32 block_w = drm_format_info_block_width(fb->format, plane);
> > +     u32 block_h = drm_format_info_block_height(fb->format, plane);
> > +     u32 block_size = fb->format->char_per_block[plane];
> > +     u32 sample_x;
> > +     u32 sample_y;
> > +     u32 block_start_y;
> > +     u32 num_hblocks;
> > +     u32 offset;
> > +
> > +     offset = fb->offsets[plane];
> > +
> > +     if (plane > 0) {
> > +             h_div = fb->format->hsub;
> > +             v_div = fb->format->vsub;
> > +     }
> > +
> > +     sample_x = x / h_div;
> > +     sample_y = y / v_div;
> > +     block_start_y = (sample_y / block_h) * block_h;
> > +     num_hblocks = sample_x / block_w;
> > +
> > +     offset += fb->pitches[plane] * block_start_y;
> > +     offset += block_size * num_hblocks;
>
> User space controls the values in fb->offsets and fb->pitches.  I'm not
> sure how well they have been validated already at this point. Did you
> investigate this?

It wouldn't be worse than before, since this changes is purely code movement.

There are minimal sanity checks done by drm_internal_framebuffer_create()
in framebuffer_check(), such as offset overflow or pitch size too small,
but that's about it. It would be up to individual drivers to perform more
checks that match their hardware limitations.

What sort of issues are you thinking about?


ChenYu

> Best regards
> Thomas
>
>
> > +
> > +     return offset;
> > +}
> > +EXPORT_SYMBOL(drm_framebuffer_get_block_offset);
> > +
> >   #ifdef CONFIG_DEBUG_FS
> >   static int drm_framebuffer_info(struct seq_file *m, void *data)
> >   {
> > diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
> > index 38b24fc8978d..c07aea1cc59f 100644
> > --- a/include/drm/drm_framebuffer.h
> > +++ b/include/drm/drm_framebuffer.h
> > @@ -220,6 +220,9 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb);
> >   void drm_framebuffer_cleanup(struct drm_framebuffer *fb);
> >   void drm_framebuffer_unregister_private(struct drm_framebuffer *fb);
> >
> > +u32 drm_framebuffer_get_block_offset(struct drm_framebuffer *fb, unsigned int plane,
> > +                                  unsigned int x, unsigned int y);
> > +
> >   /**
> >    * drm_framebuffer_get - acquire a framebuffer reference
> >    * @fb: DRM framebuffer
>
> --
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
> GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)
>
>
  

Hi

Am 18.09.26 um 06:16 schrieb Chen-Yu Tsai:
> On Thu, Sep 17, 2026 at 11:20 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Hi
>>
>> Am 16.09.26 um 05:33 schrieb Chen-Yu Tsai:
>>> Currently drm_fb_dma_get_gem_addr() calculates the offset into the
>>> framebuffer memory for the framebuffer's unclipped source coordinates,
>>> adds that to the framebuffer's backing storage, and returns the result.
>>>
>>> We are about to add a variant that uses the clipped source coordinates,
>>> so there is already some reuse of code. However, calculating the data
>>> offset for a given pixel is not specific to the DMA FB helpers. The
>>> offset is only related to the framebuffer.
>>>
>>> Split out the offset calculation into a new framebuffer helper so that
>>> non-DMA users can also reuse the same code.
>>>
>>> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> Cc: <stable@vger.kernel.org> # dependency for next patch
>>> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
>>> ---
>>> Changes since v1:
>>> - New patch
>>> ---
>>>    drivers/gpu/drm/drm_fb_dma_helper.c | 28 ++----------------
>>>    drivers/gpu/drm/drm_framebuffer.c   | 45 +++++++++++++++++++++++++++++
>>>    include/drm/drm_framebuffer.h       |  3 ++
>>>    3 files changed, 51 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_fb_dma_helper.c b/drivers/gpu/drm/drm_fb_dma_helper.c
>>> index fd71969d2fb1..ab0f37d8a5ff 100644
>>> --- a/drivers/gpu/drm/drm_fb_dma_helper.c
>>> +++ b/drivers/gpu/drm/drm_fb_dma_helper.c
>>> @@ -75,36 +75,14 @@ dma_addr_t drm_fb_dma_get_gem_addr(struct drm_framebuffer *fb,
>>>                                   unsigned int plane)
>>>    {
>>>        struct drm_gem_dma_object *obj;
>>> -     dma_addr_t dma_addr;
>>> -     u8 h_div = 1, v_div = 1;
>>> -     u32 block_w = drm_format_info_block_width(fb->format, plane);
>>> -     u32 block_h = drm_format_info_block_height(fb->format, plane);
>>> -     u32 block_size = fb->format->char_per_block[plane];
>>> -     u32 sample_x;
>>> -     u32 sample_y;
>>> -     u32 block_start_y;
>>> -     u32 num_hblocks;
>>>
>>>        obj = drm_fb_dma_get_gem_obj(fb, plane);
>>>        if (!obj)
>>>                return 0;
>>>
>>> -     dma_addr = obj->dma_addr + fb->offsets[plane];
>>> -
>>> -     if (plane > 0) {
>>> -             h_div = fb->format->hsub;
>>> -             v_div = fb->format->vsub;
>>> -     }
>>> -
>>> -     sample_x = (state->src_x >> 16) / h_div;
>>> -     sample_y = (state->src_y >> 16) / v_div;
>>> -     block_start_y = (sample_y / block_h) * block_h;
>>> -     num_hblocks = sample_x / block_w;
>>> -
>>> -     dma_addr += fb->pitches[plane] * block_start_y;
>>> -     dma_addr += block_size * num_hblocks;
>>> -
>>> -     return dma_addr;
>>> +     return obj->dma_addr + drm_framebuffer_get_block_offset(fb, plane,
>>> +                                                             state->src_x >> 16,
>>> +                                                             state->src_y >> 16);
>>>    }
>>>    EXPORT_SYMBOL_GPL(drm_fb_dma_get_gem_addr);
>>>
>>> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
>>> index d32aceb6ca9b..9e1231162047 100644
>>> --- a/drivers/gpu/drm/drm_framebuffer.c
>>> +++ b/drivers/gpu/drm/drm_framebuffer.c
>>> @@ -1208,6 +1208,51 @@ void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,
>>>        }
>>>    }
>>>
>>> +/**
>>> + * drm_framebuffer_get_block_offset() - Get offset to start of pixel block for
>>> + * the given framebuffer and coordinates.
>>> + * @fb: The framebuffer
>>> + * @plane: Which plane
>>> + * @x: x coordinate for pixel
>>> + * @y: y coordinate for pixel
>>> + *
>>> + * This function will usually be called from the PLANE callback functions,
>>> + * or from one of the helpers that calculates the framebuffer's DMA address.
>>> + *
>>> + * Return: offset from start of framebuffer to start of pixel block
>>> + */
>>> +u32 drm_framebuffer_get_block_offset(struct drm_framebuffer *fb, unsigned int plane,
>>> +                                  unsigned int x, unsigned int y)
>> Better use u64 as return type.
> To avoid overflow? Not sure who would use crazy large framebuffers, but
> doesn't hurt to play it safe.

I'd be worried about a malicious user space that tries to access OOB.

Apart from that, we use u64 for other framebuffer-related sizes like 
pitch calculations or dma addresses. Using u64 here would keep that 
consistent.


>
>>> +{
>>> +     u8 h_div = 1, v_div = 1;
>>> +     u32 block_w = drm_format_info_block_width(fb->format, plane);
>>> +     u32 block_h = drm_format_info_block_height(fb->format, plane);
>>> +     u32 block_size = fb->format->char_per_block[plane];
>>> +     u32 sample_x;
>>> +     u32 sample_y;
>>> +     u32 block_start_y;
>>> +     u32 num_hblocks;
>>> +     u32 offset;
>>> +
>>> +     offset = fb->offsets[plane];
>>> +
>>> +     if (plane > 0) {
>>> +             h_div = fb->format->hsub;
>>> +             v_div = fb->format->vsub;
>>> +     }
>>> +
>>> +     sample_x = x / h_div;
>>> +     sample_y = y / v_div;
>>> +     block_start_y = (sample_y / block_h) * block_h;
>>> +     num_hblocks = sample_x / block_w;
>>> +
>>> +     offset += fb->pitches[plane] * block_start_y;
>>> +     offset += block_size * num_hblocks;
>> User space controls the values in fb->offsets and fb->pitches.  I'm not
>> sure how well they have been validated already at this point. Did you
>> investigate this?
> It wouldn't be worse than before, since this changes is purely code movement.
>
> There are minimal sanity checks done by drm_internal_framebuffer_create()
> in framebuffer_check(), such as offset overflow or pitch size too small,
> but that's about it. It would be up to individual drivers to perform more
> checks that match their hardware limitations.

Right, makes sense. Looking through the framebuffer validation, a 
buffer-size check could be done in framebuffer_check().  But that's 
another patch series.


>
> What sort of issues are you thinking about?

Again, I'm thinking of malicious user space that crafts these values to 
force an OOB access.

Best regards
Thomas


>
>
> ChenYu
>
>> Best regards
>> Thomas
>>
>>
>>> +
>>> +     return offset;
>>> +}
>>> +EXPORT_SYMBOL(drm_framebuffer_get_block_offset);
>>> +
>>>    #ifdef CONFIG_DEBUG_FS
>>>    static int drm_framebuffer_info(struct seq_file *m, void *data)
>>>    {
>>> diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
>>> index 38b24fc8978d..c07aea1cc59f 100644
>>> --- a/include/drm/drm_framebuffer.h
>>> +++ b/include/drm/drm_framebuffer.h
>>> @@ -220,6 +220,9 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb);
>>>    void drm_framebuffer_cleanup(struct drm_framebuffer *fb);
>>>    void drm_framebuffer_unregister_private(struct drm_framebuffer *fb);
>>>
>>> +u32 drm_framebuffer_get_block_offset(struct drm_framebuffer *fb, unsigned int plane,
>>> +                                  unsigned int x, unsigned int y);
>>> +
>>>    /**
>>>     * drm_framebuffer_get - acquire a framebuffer reference
>>>     * @fb: DRM framebuffer
>> --
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
>> GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)
>>
>>
  

On Fri, Sep 18, 2026 at 2:41 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 18.09.26 um 06:16 schrieb Chen-Yu Tsai:
> > On Thu, Sep 17, 2026 at 11:20 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >> Hi
> >>
> >> Am 16.09.26 um 05:33 schrieb Chen-Yu Tsai:
> >>> Currently drm_fb_dma_get_gem_addr() calculates the offset into the
> >>> framebuffer memory for the framebuffer's unclipped source coordinates,
> >>> adds that to the framebuffer's backing storage, and returns the result.
> >>>
> >>> We are about to add a variant that uses the clipped source coordinates,
> >>> so there is already some reuse of code. However, calculating the data
> >>> offset for a given pixel is not specific to the DMA FB helpers. The
> >>> offset is only related to the framebuffer.
> >>>
> >>> Split out the offset calculation into a new framebuffer helper so that
> >>> non-DMA users can also reuse the same code.
> >>>
> >>> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> >>> Cc: <stable@vger.kernel.org> # dependency for next patch
> >>> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> >>> ---
> >>> Changes since v1:
> >>> - New patch
> >>> ---
> >>>    drivers/gpu/drm/drm_fb_dma_helper.c | 28 ++----------------
> >>>    drivers/gpu/drm/drm_framebuffer.c   | 45 +++++++++++++++++++++++++++++
> >>>    include/drm/drm_framebuffer.h       |  3 ++
> >>>    3 files changed, 51 insertions(+), 25 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_fb_dma_helper.c b/drivers/gpu/drm/drm_fb_dma_helper.c
> >>> index fd71969d2fb1..ab0f37d8a5ff 100644
> >>> --- a/drivers/gpu/drm/drm_fb_dma_helper.c
> >>> +++ b/drivers/gpu/drm/drm_fb_dma_helper.c
> >>> @@ -75,36 +75,14 @@ dma_addr_t drm_fb_dma_get_gem_addr(struct drm_framebuffer *fb,
> >>>                                   unsigned int plane)
> >>>    {
> >>>        struct drm_gem_dma_object *obj;
> >>> -     dma_addr_t dma_addr;
> >>> -     u8 h_div = 1, v_div = 1;
> >>> -     u32 block_w = drm_format_info_block_width(fb->format, plane);
> >>> -     u32 block_h = drm_format_info_block_height(fb->format, plane);
> >>> -     u32 block_size = fb->format->char_per_block[plane];
> >>> -     u32 sample_x;
> >>> -     u32 sample_y;
> >>> -     u32 block_start_y;
> >>> -     u32 num_hblocks;
> >>>
> >>>        obj = drm_fb_dma_get_gem_obj(fb, plane);
> >>>        if (!obj)
> >>>                return 0;
> >>>
> >>> -     dma_addr = obj->dma_addr + fb->offsets[plane];
> >>> -
> >>> -     if (plane > 0) {
> >>> -             h_div = fb->format->hsub;
> >>> -             v_div = fb->format->vsub;
> >>> -     }
> >>> -
> >>> -     sample_x = (state->src_x >> 16) / h_div;
> >>> -     sample_y = (state->src_y >> 16) / v_div;
> >>> -     block_start_y = (sample_y / block_h) * block_h;
> >>> -     num_hblocks = sample_x / block_w;
> >>> -
> >>> -     dma_addr += fb->pitches[plane] * block_start_y;
> >>> -     dma_addr += block_size * num_hblocks;
> >>> -
> >>> -     return dma_addr;
> >>> +     return obj->dma_addr + drm_framebuffer_get_block_offset(fb, plane,
> >>> +                                                             state->src_x >> 16,
> >>> +                                                             state->src_y >> 16);
> >>>    }
> >>>    EXPORT_SYMBOL_GPL(drm_fb_dma_get_gem_addr);
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> >>> index d32aceb6ca9b..9e1231162047 100644
> >>> --- a/drivers/gpu/drm/drm_framebuffer.c
> >>> +++ b/drivers/gpu/drm/drm_framebuffer.c
> >>> @@ -1208,6 +1208,51 @@ void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,
> >>>        }
> >>>    }
> >>>
> >>> +/**
> >>> + * drm_framebuffer_get_block_offset() - Get offset to start of pixel block for
> >>> + * the given framebuffer and coordinates.
> >>> + * @fb: The framebuffer
> >>> + * @plane: Which plane
> >>> + * @x: x coordinate for pixel
> >>> + * @y: y coordinate for pixel
> >>> + *
> >>> + * This function will usually be called from the PLANE callback functions,
> >>> + * or from one of the helpers that calculates the framebuffer's DMA address.
> >>> + *
> >>> + * Return: offset from start of framebuffer to start of pixel block
> >>> + */
> >>> +u32 drm_framebuffer_get_block_offset(struct drm_framebuffer *fb, unsigned int plane,
> >>> +                                  unsigned int x, unsigned int y)
> >> Better use u64 as return type.
> > To avoid overflow? Not sure who would use crazy large framebuffers, but
> > doesn't hurt to play it safe.
>
> I'd be worried about a malicious user space that tries to access OOB.
>
> Apart from that, we use u64 for other framebuffer-related sizes like
> pitch calculations or dma addresses. Using u64 here would keep that
> consistent.

Indeed. It seemed weird that the helper originally used u32. Maybe it
was carried over from CMA on ARMv7, which predominantly only had
32-bit address space data busses?

>
> >
> >>> +{
> >>> +     u8 h_div = 1, v_div = 1;
> >>> +     u32 block_w = drm_format_info_block_width(fb->format, plane);
> >>> +     u32 block_h = drm_format_info_block_height(fb->format, plane);
> >>> +     u32 block_size = fb->format->char_per_block[plane];
> >>> +     u32 sample_x;
> >>> +     u32 sample_y;
> >>> +     u32 block_start_y;
> >>> +     u32 num_hblocks;
> >>> +     u32 offset;
> >>> +
> >>> +     offset = fb->offsets[plane];
> >>> +
> >>> +     if (plane > 0) {
> >>> +             h_div = fb->format->hsub;
> >>> +             v_div = fb->format->vsub;
> >>> +     }
> >>> +
> >>> +     sample_x = x / h_div;
> >>> +     sample_y = y / v_div;
> >>> +     block_start_y = (sample_y / block_h) * block_h;
> >>> +     num_hblocks = sample_x / block_w;
> >>> +
> >>> +     offset += fb->pitches[plane] * block_start_y;
> >>> +     offset += block_size * num_hblocks;
> >> User space controls the values in fb->offsets and fb->pitches.  I'm not
> >> sure how well they have been validated already at this point. Did you
> >> investigate this?
> > It wouldn't be worse than before, since this changes is purely code movement.
> >
> > There are minimal sanity checks done by drm_internal_framebuffer_create()
> > in framebuffer_check(), such as offset overflow or pitch size too small,
> > but that's about it. It would be up to individual drivers to perform more
> > checks that match their hardware limitations.
>
> Right, makes sense. Looking through the framebuffer validation, a
> buffer-size check could be done in framebuffer_check().  But that's
> another patch series.

That's further covered by drm_gem_fb_init_with_funcs(), which
drm_gem_fb_create*() goes into. I didn't check all the drivers that
implemented their own .fb_create callback though.

- rockchip uses the GEM FB helpers
- MSM reimplements the GEM FB helpers, but does have proper size checks
- nouveau has size checks
- omap has size checks

> >
> > What sort of issues are you thinking about?
>
> Again, I'm thinking of malicious user space that crafts these values to
> force an OOB access.

I think we're covered.


Thanks
ChenYu

> Best regards
> Thomas
>
>
> >
> >
> > ChenYu
> >
> >> Best regards
> >> Thomas
> >>
> >>
> >>> +
> >>> +     return offset;
> >>> +}
> >>> +EXPORT_SYMBOL(drm_framebuffer_get_block_offset);
> >>> +
> >>>    #ifdef CONFIG_DEBUG_FS
> >>>    static int drm_framebuffer_info(struct seq_file *m, void *data)
> >>>    {
> >>> diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
> >>> index 38b24fc8978d..c07aea1cc59f 100644
> >>> --- a/include/drm/drm_framebuffer.h
> >>> +++ b/include/drm/drm_framebuffer.h
> >>> @@ -220,6 +220,9 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb);
> >>>    void drm_framebuffer_cleanup(struct drm_framebuffer *fb);
> >>>    void drm_framebuffer_unregister_private(struct drm_framebuffer *fb);
> >>>
> >>> +u32 drm_framebuffer_get_block_offset(struct drm_framebuffer *fb, unsigned int plane,
> >>> +                                  unsigned int x, unsigned int y);
> >>> +
> >>>    /**
> >>>     * drm_framebuffer_get - acquire a framebuffer reference
> >>>     * @fb: DRM framebuffer
> >> --
> >> --
> >> Thomas Zimmermann
> >> Graphics Driver Developer
> >> SUSE Software Solutions Germany GmbH
> >> Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
> >> GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)
> >>
> >>
>
> --
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
> GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)
>
>
  

On Fri, Sep 18, 2026 at 3:06 PM Chen-Yu Tsai <wenst@chromium.org> wrote:
>
> On Fri, Sep 18, 2026 at 2:41 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >
> > Hi
> >
> > Am 18.09.26 um 06:16 schrieb Chen-Yu Tsai:
> > > On Thu, Sep 17, 2026 at 11:20 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> > >> Hi
> > >>
> > >> Am 16.09.26 um 05:33 schrieb Chen-Yu Tsai:
> > >>> Currently drm_fb_dma_get_gem_addr() calculates the offset into the
> > >>> framebuffer memory for the framebuffer's unclipped source coordinates,
> > >>> adds that to the framebuffer's backing storage, and returns the result.
> > >>>
> > >>> We are about to add a variant that uses the clipped source coordinates,
> > >>> so there is already some reuse of code. However, calculating the data
> > >>> offset for a given pixel is not specific to the DMA FB helpers. The
> > >>> offset is only related to the framebuffer.
> > >>>
> > >>> Split out the offset calculation into a new framebuffer helper so that
> > >>> non-DMA users can also reuse the same code.
> > >>>
> > >>> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> > >>> Cc: <stable@vger.kernel.org> # dependency for next patch
> > >>> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > >>> ---
> > >>> Changes since v1:
> > >>> - New patch
> > >>> ---
> > >>>    drivers/gpu/drm/drm_fb_dma_helper.c | 28 ++----------------
> > >>>    drivers/gpu/drm/drm_framebuffer.c   | 45 +++++++++++++++++++++++++++++
> > >>>    include/drm/drm_framebuffer.h       |  3 ++
> > >>>    3 files changed, 51 insertions(+), 25 deletions(-)
> > >>>
> > >>> diff --git a/drivers/gpu/drm/drm_fb_dma_helper.c b/drivers/gpu/drm/drm_fb_dma_helper.c
> > >>> index fd71969d2fb1..ab0f37d8a5ff 100644
> > >>> --- a/drivers/gpu/drm/drm_fb_dma_helper.c
> > >>> +++ b/drivers/gpu/drm/drm_fb_dma_helper.c
> > >>> @@ -75,36 +75,14 @@ dma_addr_t drm_fb_dma_get_gem_addr(struct drm_framebuffer *fb,
> > >>>                                   unsigned int plane)
> > >>>    {
> > >>>        struct drm_gem_dma_object *obj;
> > >>> -     dma_addr_t dma_addr;
> > >>> -     u8 h_div = 1, v_div = 1;
> > >>> -     u32 block_w = drm_format_info_block_width(fb->format, plane);
> > >>> -     u32 block_h = drm_format_info_block_height(fb->format, plane);
> > >>> -     u32 block_size = fb->format->char_per_block[plane];
> > >>> -     u32 sample_x;
> > >>> -     u32 sample_y;
> > >>> -     u32 block_start_y;
> > >>> -     u32 num_hblocks;
> > >>>
> > >>>        obj = drm_fb_dma_get_gem_obj(fb, plane);
> > >>>        if (!obj)
> > >>>                return 0;
> > >>>
> > >>> -     dma_addr = obj->dma_addr + fb->offsets[plane];
> > >>> -
> > >>> -     if (plane > 0) {
> > >>> -             h_div = fb->format->hsub;
> > >>> -             v_div = fb->format->vsub;
> > >>> -     }
> > >>> -
> > >>> -     sample_x = (state->src_x >> 16) / h_div;
> > >>> -     sample_y = (state->src_y >> 16) / v_div;
> > >>> -     block_start_y = (sample_y / block_h) * block_h;
> > >>> -     num_hblocks = sample_x / block_w;
> > >>> -
> > >>> -     dma_addr += fb->pitches[plane] * block_start_y;
> > >>> -     dma_addr += block_size * num_hblocks;
> > >>> -
> > >>> -     return dma_addr;
> > >>> +     return obj->dma_addr + drm_framebuffer_get_block_offset(fb, plane,
> > >>> +                                                             state->src_x >> 16,
> > >>> +                                                             state->src_y >> 16);
> > >>>    }
> > >>>    EXPORT_SYMBOL_GPL(drm_fb_dma_get_gem_addr);
> > >>>
> > >>> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> > >>> index d32aceb6ca9b..9e1231162047 100644
> > >>> --- a/drivers/gpu/drm/drm_framebuffer.c
> > >>> +++ b/drivers/gpu/drm/drm_framebuffer.c
> > >>> @@ -1208,6 +1208,51 @@ void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent,
> > >>>        }
> > >>>    }
> > >>>
> > >>> +/**
> > >>> + * drm_framebuffer_get_block_offset() - Get offset to start of pixel block for
> > >>> + * the given framebuffer and coordinates.
> > >>> + * @fb: The framebuffer
> > >>> + * @plane: Which plane
> > >>> + * @x: x coordinate for pixel
> > >>> + * @y: y coordinate for pixel
> > >>> + *
> > >>> + * This function will usually be called from the PLANE callback functions,
> > >>> + * or from one of the helpers that calculates the framebuffer's DMA address.
> > >>> + *
> > >>> + * Return: offset from start of framebuffer to start of pixel block
> > >>> + */
> > >>> +u32 drm_framebuffer_get_block_offset(struct drm_framebuffer *fb, unsigned int plane,
> > >>> +                                  unsigned int x, unsigned int y)
> > >> Better use u64 as return type.
> > > To avoid overflow? Not sure who would use crazy large framebuffers, but
> > > doesn't hurt to play it safe.
> >
> > I'd be worried about a malicious user space that tries to access OOB.
> >
> > Apart from that, we use u64 for other framebuffer-related sizes like
> > pitch calculations or dma addresses. Using u64 here would keep that
> > consistent.
>
> Indeed. It seemed weird that the helper originally used u32. Maybe it
> was carried over from CMA on ARMv7, which predominantly only had
> 32-bit address space data busses?
>
> >
> > >
> > >>> +{
> > >>> +     u8 h_div = 1, v_div = 1;
> > >>> +     u32 block_w = drm_format_info_block_width(fb->format, plane);
> > >>> +     u32 block_h = drm_format_info_block_height(fb->format, plane);
> > >>> +     u32 block_size = fb->format->char_per_block[plane];
> > >>> +     u32 sample_x;
> > >>> +     u32 sample_y;
> > >>> +     u32 block_start_y;
> > >>> +     u32 num_hblocks;
> > >>> +     u32 offset;
> > >>> +
> > >>> +     offset = fb->offsets[plane];
> > >>> +
> > >>> +     if (plane > 0) {
> > >>> +             h_div = fb->format->hsub;
> > >>> +             v_div = fb->format->vsub;
> > >>> +     }
> > >>> +
> > >>> +     sample_x = x / h_div;
> > >>> +     sample_y = y / v_div;
> > >>> +     block_start_y = (sample_y / block_h) * block_h;
> > >>> +     num_hblocks = sample_x / block_w;
> > >>> +
> > >>> +     offset += fb->pitches[plane] * block_start_y;
> > >>> +     offset += block_size * num_hblocks;
> > >> User space controls the values in fb->offsets and fb->pitches.  I'm not
> > >> sure how well they have been validated already at this point. Did you
> > >> investigate this?
> > > It wouldn't be worse than before, since this changes is purely code movement.
> > >
> > > There are minimal sanity checks done by drm_internal_framebuffer_create()
> > > in framebuffer_check(), such as offset overflow or pitch size too small,
> > > but that's about it. It would be up to individual drivers to perform more
> > > checks that match their hardware limitations.
> >
> > Right, makes sense. Looking through the framebuffer validation, a
> > buffer-size check could be done in framebuffer_check().  But that's
> > another patch series.
>
> That's further covered by drm_gem_fb_init_with_funcs(), which
> drm_gem_fb_create*() goes into. I didn't check all the drivers that
> implemented their own .fb_create callback though.
>
> - rockchip uses the GEM FB helpers
> - MSM reimplements the GEM FB helpers, but does have proper size checks
> - nouveau has size checks
> - omap has size checks

Side note: it seems that the drivers that reimplement
drm_gem_fb_init_with_funcs() do so because they need to do additional
checks on the (sub-classed) GEM objects. Perhaps exporting drm_gem_fb_init()
or deconstructing drm_gem_fb_init_with_funcs() could allow more of them
to use common helpers for things like size checks.

> > >
> > > What sort of issues are you thinking about?
> >
> > Again, I'm thinking of malicious user space that crafts these values to
> > force an OOB access.
>
> I think we're covered.
>
>
> Thanks
> ChenYu
>
> > Best regards
> > Thomas
> >
> >
> > >
> > >
> > > ChenYu
> > >
> > >> Best regards
> > >> Thomas
> > >>
> > >>
> > >>> +
> > >>> +     return offset;
> > >>> +}
> > >>> +EXPORT_SYMBOL(drm_framebuffer_get_block_offset);
> > >>> +
> > >>>    #ifdef CONFIG_DEBUG_FS
> > >>>    static int drm_framebuffer_info(struct seq_file *m, void *data)
> > >>>    {
> > >>> diff --git a/include/drm/drm_framebuffer.h b/include/drm/drm_framebuffer.h
> > >>> index 38b24fc8978d..c07aea1cc59f 100644
> > >>> --- a/include/drm/drm_framebuffer.h
> > >>> +++ b/include/drm/drm_framebuffer.h
> > >>> @@ -220,6 +220,9 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb);
> > >>>    void drm_framebuffer_cleanup(struct drm_framebuffer *fb);
> > >>>    void drm_framebuffer_unregister_private(struct drm_framebuffer *fb);
> > >>>
> > >>> +u32 drm_framebuffer_get_block_offset(struct drm_framebuffer *fb, unsigned int plane,
> > >>> +                                  unsigned int x, unsigned int y);
> > >>> +
> > >>>    /**
> > >>>     * drm_framebuffer_get - acquire a framebuffer reference
> > >>>     * @fb: DRM framebuffer
> > >> --
> > >> --
> > >> Thomas Zimmermann
> > >> Graphics Driver Developer
> > >> SUSE Software Solutions Germany GmbH
> > >> Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
> > >> GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)
> > >>
> > >>
> >
> > --
> > --
> > Thomas Zimmermann
> > Graphics Driver Developer
> > SUSE Software Solutions Germany GmbH
> > Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
> > GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)
> >
> >