Re: [PATCH v4 6/6] drm/hibmc: Use gem-shmem with shadow-plane helpers for memory management

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Missing bounds check on framebuffer size and pitch allows an out-of-bounds write into the memory mapped VRAM.
- [High] Sleep in atomic context due to manual call of `drm_gem_fb_begin_cpu_access()` inside plane atomic update.
--

commit dadf851669fd23173dce5357fbb44cac3db06b77
Author: Thomas Zimmermann <tzimmermann@suse.de>

drm/hibmc: Use gem-shmem with shadow-plane helpers for memory management

Replace the gem-vram memory manager with gem-shmem. Makes the driver more
robust and enables dma-buf sharing with other hardware.

[ ... ]

> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
> index fe73b365e5474..b4ab53db1c087 100644
> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c
> @@ -100,22 +102,35 @@ static int hibmc_plane_atomic_check(struct drm_plane *plane,
>  static void hibmc_plane_atomic_update(struct drm_plane *plane,
>  				      struct drm_atomic_commit *state)
>  {
> +	struct hibmc_drm_private *priv = to_hibmc_drm_private(plane->dev);
>  	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state, plane);
> +	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(new_state);
>  	struct drm_framebuffer *fb = new_state->fb;
> +	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state, plane);
> +	u32 gpu_addr = 0;
>  	u32 reg;
> -	s64 gpu_addr = 0;
>  	u32 line_l;
> -	struct hibmc_drm_private *priv = to_hibmc_drm_private(plane->dev);
> -	struct drm_gem_vram_object *gbo;
>  
>  	if (!fb)
>  		return;
>  
> -	gbo = drm_gem_vram_of_gem(fb->obj[0]);
> +	if (drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE) == 0) {

[Severity: High]
Is it safe to call drm_gem_fb_begin_cpu_access() here?

This function can sleep (for example, by acquiring dma_resv_lock for
imported dma-bufs), but hibmc_plane_atomic_update() runs in atomic context
where sleeping is forbidden.

Also, since the driver uses DRM_GEM_SHADOW_PLANE_HELPER_FUNCS which
automatically registers .begin_fb_access = drm_gem_begin_shadow_fb_access,
doesn't the DRM core already handle buffer mapping and synchronization
outside the atomic section?

> +		struct drm_rect damage;
> +		struct drm_atomic_helper_damage_iter iter;
> +
> +		drm_atomic_helper_damage_iter_init(&iter, old_state, new_state);
> +		drm_atomic_for_each_plane_damage(&iter, &damage) {
> +			struct iosys_map dst[DRM_FORMAT_MAX_PLANES] = {
> +				IOSYS_MAP_INIT_VADDR_IOMEM(priv->vram + gpu_addr),
> +			};
>  
> -	gpu_addr = drm_gem_vram_offset(gbo);
> -	if (WARN_ON_ONCE(gpu_addr < 0))
> -		return; /* Bug: we didn't pin the BO to VRAM in prepare_fb. */
> +			iosys_map_incr(&dst[0],
> +				       drm_fb_clip_offset(fb->pitches[0], fb->format, &damage));
> +			drm_fb_memcpy(dst, fb->pitches, shadow_plane_state->data, fb, &damage);

[Severity: Critical]
Could this overflow the priv->vram I/O memory mapping?

With the transition from gem-vram to gem-shmem, the driver no longer
implicitly restricts the GEM object size to the physical VRAM size. If
userspace provides a framebuffer with a pitch that exceeds what fits in
priv->vram_size, it appears this drm_fb_memcpy() operation will write past
the end of priv->vram.

Should there be a bounds check on the pitch during the atomic check phase
or during framebuffer creation to ensure the memory footprint fits within
priv->vram_size?

> +		}
> +
> +		drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618123142.92298-1-tzimmermann@suse.de?part=6