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 <[email protected]> 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/[email protected]?part=6
