Re: [PATCH 02/13] drm/sti: Don't use GFP_DMA when calling dma_alloc_wc()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] The `sti_cursor_atomic_check` callback incorrectly mutates persistent global plane state and frees active DMA memory during the validation-only atomic check phase. This violates the DRM atomic API contract and causes a hardware use-after-free if triggered by a TEST_ONLY commit or if the commit fails.
- [High] Missing error handling for DMA allocation failure in `sti_hqvdp_init` leads to a NULL pointer dereference later in `sti_hqvdp_atomic_update`.
--

commit ffcf141c00928add2d88a408f8402ffc52667925
Author: Baoquan He <hebaoquan@kylinos.cn>
drm/sti: Don't use GFP_DMA when calling dma_alloc_wc()

This commit removes the redundant GFP_DMA flag from dma_alloc_wc() calls
in the sti DRM driver, as the DMA core already handles zone selection based
on the device's coherent DMA mask.

> diff --git a/drivers/gpu/drm/sti/sti_cursor.c b/drivers/gpu/drm/sti/sti_cursor.c
> --- a/drivers/gpu/drm/sti/sti_cursor.c
> +++ b/drivers/gpu/drm/sti/sti_cursor.c
[ ... ]
> @@ -240,7 +240,7 @@ static int sti_cursor_atomic_check(struct drm_plane *drm_plane,
>  		cursor->pixmap.base = dma_alloc_wc(cursor->dev,
>  						   cursor->pixmap.size,
>  						   &cursor->pixmap.paddr,
> -						   GFP_KERNEL | GFP_DMA);
> +						   GFP_KERNEL);
>  		if (!cursor->pixmap.base) {
>  			DRM_ERROR("Failed to allocate memory for pixmap\n");
>  			return -EINVAL;

[Severity: Critical]
This is a pre-existing issue, but does this mutate persistent global plane
state during the validation-only atomic check phase?

Just before this allocation, sti_cursor_atomic_check() frees the active
DMA buffer (cursor->pixmap.base). Could this cause a hardware use-after-free
if triggered by a TEST_ONLY commit while the hardware is still scanning out
the old buffer? It appears the mutation of global state here breaks the core
assumption that TEST_ONLY commits are side-effect free.

[ ... ]
> diff --git a/drivers/gpu/drm/sti/sti_hqvdp.c b/drivers/gpu/drm/sti/sti_hqvdp.c
> --- a/drivers/gpu/drm/sti/sti_hqvdp.c
> +++ b/drivers/gpu/drm/sti/sti_hqvdp.c
[ ... ]
> @@ -860,7 +860,7 @@ static void sti_hqvdp_init(struct sti_hqvdp *hqvdp)
>  	size = NB_VDP_CMD * sizeof(struct sti_hqvdp_cmd);
>  	hqvdp->hqvdp_cmd = dma_alloc_wc(hqvdp->dev, size,
>  					&dma_addr,
> -					GFP_KERNEL | GFP_DMA);
> +					GFP_KERNEL);
>  	if (!hqvdp->hqvdp_cmd) {
>  		DRM_ERROR("Failed to allocate memory for VDP cmd\n");
>  		return;
>  	}

[Severity: High]
This is a pre-existing issue, but should this function return an error code
if the allocation fails instead of just returning void?

If dma_alloc_wc() fails here in sti_hqvdp_init(), the error is logged but
not propagated to the caller, leaving hqvdp->hqvdp_cmd as NULL. Could this
allow the plane to be successfully registered, resulting in a NULL pointer
dereference later when userspace attempts to use the plane?

For example, sti_hqvdp_atomic_update() dereferences the pointer
unconditionally:

sti_hqvdp_atomic_update()
   cmd = hqvdp->hqvdp_cmd + cmd_offset;
   cmd->top.config = TOP_CONFIG_PROGRESSIVE;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903111836.1777265-1-hebaoquan@kylinos.cn?part=2