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

[Thomas Zimmermann <tzimmermann@suse.de>]

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

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