Re: [PATCH RFC] drm/verisilicon: Switch to drm_fb_dma_get_addr() for framebuffer addresses

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