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

[Thomas Zimmermann]

Hi

Am 18.09.26 um 06:16 schrieb Chen-Yu Tsai:
> On Thu, Sep 17, 2026 at 11:20 PM Thomas Zimmermann <tzimmermann@xxxxxxx> 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@xxxxxxx>
> > > Cc: <stable@xxxxxxxxxxxxxxx> # dependency for next patch
> > > Signed-off-by: Chen-Yu Tsai <wenst@xxxxxxxxxxxx>
> > > ---
> > > 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)

--
--
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)