Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] The patch introduces a build failure and a potential deterministic NULL pointer dereference due to API misuse of `drm_fb_dma_get_gem_addr()`. - [Low] A dangling function prototype for `vs_fb_get_dma_addr()` is left in `vs_plane.h`. Pre-existing issues: - [High] A pre-existing bug causes the hardware display controller to read out-of-bounds physical memory when the cursor is clipped, potentially leaking adjacent kernel memory to the screen. -- --- Patch [1]: [PATCH RFC] drm/verisilicon: Switch to drm_fb_dma_get_addr() for framebuffer addresses --- commit 236e399ed29b505c46a162513a0a121f26bb52bf Author: Chen-Yu Tsai <[email protected]> drm/verisilicon: Switch to drm_fb_dma_get_addr() for framebuffer addresses The verisilicon driver has a custom framebuffer address calculating helper that the common drm_fb_dma_get_addr() can substitute. > diff --git a/drivers/gpu/drm/verisilicon/vs_cursor_plane.c > b/drivers/gpu/drm/verisilicon/vs_cursor_plane.c > index fa4f601dd0c87..ae03d415d66b0 100644 > --- a/drivers/gpu/drm/verisilicon/vs_cursor_plane.c > +++ b/drivers/gpu/drm/verisilicon/vs_cursor_plane.c [ ... ] > @@ -176,7 +176,8 @@ static void vs_cursor_plane_atomic_update(struct > drm_plane *plane, > break; > } > > - dma_addr = vs_fb_get_dma_addr(fb, &state->src); > + /* hardware handles clipping as seen below */ > + dma_addr = drm_fb_dma_get_gem_addr(fb, 0); [Severity: High] Does this compile with modern compilers? The drm_fb_dma_get_gem_addr() function is defined in drm_fb_dma_helper.c as taking three arguments: drm_fb_dma_helper.c: dma_addr_t drm_fb_dma_get_gem_addr(struct drm_framebuffer *fb, struct drm_plane_state *state, unsigned int plane) It looks like this call passes only two arguments. If the compiler permits this due to a missing include, the 0 would map to the state pointer. Since drm_fb_dma_get_gem_addr() unconditionally dereferences the state pointer for sample_x and sample_y: drm_fb_dma_helper.c:drm_fb_dma_get_gem_addr() { ... sample_x = (state->src_x >> 16) / h_div; sample_y = (state->src_y >> 16) / v_div; ... } Could this lead to a deterministic NULL pointer dereference when updating the cursor plane? [Severity: High] This isn't a bug introduced by this patch, but does the cursor clipping implementation properly shift the offsets when moving the cursor off the left or top edges? Looking at vs_cursor_plane_atomic_update() right below this address update: vs_cursor_plane.c:vs_cursor_plane_atomic_update() { ... } else { regmap_update_bits(dc->regs, VSDC_CURSOR_CONFIG(output), VSDC_CURSOR_CONFIG_X_OFF_MASK, -state->crtc_x); ... } Since the -state->crtc_x value is passed directly without applying a shift macro, could the mask clear the unshifted value and write a 0 offset to the hardware? If the DRM core already clipped the source coordinates, might this cause the hardware to overshoot the end of the cursor buffer and read out-of-bounds physical memory? > regmap_write(dc->regs, VSDC_CURSOR_ADDRESS(output), > lower_32_bits(dma_addr)); [ ... ] > diff --git a/drivers/gpu/drm/verisilicon/vs_plane.c > b/drivers/gpu/drm/verisilicon/vs_plane.c > index d81f7b8f4c650..38b8b536eccbd 100644 > --- a/drivers/gpu/drm/verisilicon/vs_plane.c > +++ b/drivers/gpu/drm/verisilicon/vs_plane.c > @@ -107,26 +107,6 @@ int drm_format_to_vs_format(u32 drm_format, struct > vs_format *vs_format) > return 0; > } > > -dma_addr_t vs_fb_get_dma_addr(struct drm_framebuffer *fb, > - const struct drm_rect *src_rect) > -{ [ ... ] > - return dma_addr; > -} > - [Severity: Low] Since vs_fb_get_dma_addr() is being removed here, should its function prototype also be removed from drivers/gpu/drm/verisilicon/vs_plane.h to avoid leaving a dangling declaration? > struct drm_plane_state *vs_plane_duplicate_state(struct drm_plane *plane) [ ... ] > diff --git a/drivers/gpu/drm/verisilicon/vs_primary_plane.c > b/drivers/gpu/drm/verisilicon/vs_primary_plane.c > index 1f2be41ae496c..46653469caaf7 100644 > --- a/drivers/gpu/drm/verisilicon/vs_primary_plane.c > +++ b/drivers/gpu/drm/verisilicon/vs_primary_plane.c > @@ -126,7 +126,11 @@ static void vs_primary_plane_atomic_update(struct > drm_plane *plane, > VSDC_FB_CONFIG_UV_SWIZZLE_EN, > vs_state->format.uv_swizzle); > > - dma_addr = vs_fb_get_dma_addr(fb, &state->src); > + /* > + * Primary plane cannot be moved, no clipping is involved, > + * so the non-clipped framebuffer address can be used. > + */ > + dma_addr = drm_fb_dma_get_gem_addr(fb, 0); [Severity: High] Similar to the cursor plane, this call to drm_fb_dma_get_gem_addr() appears to be missing the state argument, which might cause the same build failure or runtime NULL pointer dereference. > > regmap_write(dc->regs, VSDC_FB_ADDRESS(output), > lower_32_bits(dma_addr)); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
