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

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

[sashiko-bot]

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

Re: [PATCH 05/13] fbdev: fsl-diu-fb: Don't use GFP_DMA when calling dmam_alloc_coherent()

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing `GFP_KERNEL` (or other `__GFP_RECLAIM` bits) in memory allocations, causing large allocations to act as non-blocking `GFP_NOWAIT` atomic allocations which are highly prone to failure.
- [High] Video memory is freed while the LCDC hardware is still performing DMA reads from it, and error paths can leave the hardware permanently reading freed memory.
- [High] Concurrent mode setting can lead to a double free of video memory because sysfs accesses do not take the necessary locks.
- [High] The struct fb_info is embedded in a devres-managed structure, leading to a Use-After-Free if a process holds the device open during unbind.
--

commit 719f5155a18f433533c0efd3851ce4c2f8cf3732
Author: Baoquan He <hebaoquan@kylinos.cn>
fbdev: fsl-diu-fb: Don't use GFP_DMA when calling dmam_alloc_coherent()

This commit removes the redundant GFP_DMA flag from the dmam_alloc_coherent()
call in fsl_diu_probe(), as the DMA core ignores it and picks the zone from
the device's coherent DMA mask.

> diff --git a/drivers/video/fbdev/fsl-diu-fb.c b/drivers/video/fbdev/fsl-diu-fb.c
> index b71d15794ce8b..d7ed007915c18 100644
> --- a/drivers/video/fbdev/fsl-diu-fb.c
> +++ b/drivers/video/fbdev/fsl-diu-fb.c
> @@ -1690,7 +1690,7 @@ static int fsl_diu_probe(struct platform_device *pdev)
>  	int ret;
>  
>  	data = dmam_alloc_coherent(&pdev->dev, sizeof(struct fsl_diu_data),
> -				   &dma_addr, GFP_DMA | __GFP_ZERO);
> +				   &dma_addr, __GFP_ZERO);

[Severity: High]
This isn't a bug introduced by this patch, but removing the redundant flag
highlights that this allocation lacks GFP_KERNEL (and thus __GFP_RECLAIM
bits). Does this multi-page allocation for the driver data structure act as
an atomic non-blocking allocation because it lacks GFP_KERNEL, making it
highly prone to failure?

The same pattern appears in map_video_memory(), where a massive multi-megabyte
framebuffer allocation is requested:

drivers/video/fbdev/fsl-diu-fb.c:map_video_memory() {
    ...
    p = alloc_pages_exact(smem_len, GFP_DMA | __GFP_ZERO);
    ...
}

Could this lack of GFP_KERNEL cause these large allocations to fail during
uptime mode setting via ioctl(FBIOPUT_VSCREENINFO) due to fragmentation?

[Severity: High]
This is a pre-existing issue, but does using a devres-managed lifecycle for
struct fsl_diu_data bypass the reference counting required by the framebuffer
core?

The data pointer returned by dmam_alloc_coherent() contains an array of
fb_info structures:

drivers/video/fbdev/fsl-diu-fb.c:fsl_diu_probe() {
    ...
    for (i = 0; i < NUM_AOIS; i++) {
        struct fb_info *info = &data->fsl_diu_info[i];
    ...
}

If a user process holds /dev/fb0 open during driver unbind, the driver core
will free struct fsl_diu_data upon return from fsl_diu_remove(). Will this
lead to a use-after-free when the process subsequently closes the descriptor
and VFS calls into fbmem.c to access the already-freed fb_info struct?

[Severity: High]
This is a pre-existing issue, but could fsl_diu_set_par() leave the LCDC
hardware permanently reading freed memory?

drivers/video/fbdev/fsl-diu-fb.c:fsl_diu_set_par() {
        ...
        if (info->fix.smem_start)
            unmap_video_memory(info);

        /* Memory allocation for framebuffer */
        if (map_video_memory(info)) {
            fb_err(info, "unable to allocate fb memory 1\n");
            return -ENOMEM;
        }
        ...
}

Since unmap_video_memory() frees the active display buffer pages via
free_pages_exact() before the hardware is disabled, the hardware continues
fetching from these physical addresses. If map_video_memory() fails (which
is highly likely due to missing GFP_KERNEL), it returns -ENOMEM early. Does
this mean the hardware is never stopped and its DMA descriptor is never
updated, persistently leaking the contents of arbitrary newly-allocated kernel
pages to the screen?

[Severity: High]
This is a pre-existing issue, but can concurrent mode setting lead to a
double free of video memory?

In store_monitor(), fsl_diu_set_par() is called locklessly:

drivers/video/fbdev/fsl-diu-fb.c:store_monitor() {
        ...
        for (i=0; i < NUM_AOIS; i++)
            fsl_diu_set_par(&data->fsl_diu_info[i]);
        ...
}

If ioctl(FBIOPUT_VSCREENINFO) concurrently updates info->var and calls
fb_set_par() (holding info->lock), both threads could simultaneously evaluate
len != info->fix.smem_len as true and call unmap_video_memory().

Inside unmap_video_memory():

drivers/video/fbdev/fsl-diu-fb.c:unmap_video_memory() {
    ...
    void *p = info->screen_base;
    size_t l = info->fix.smem_len;

    mutex_lock(&info->mm_lock);
    info->screen_base = NULL;
    info->fix.smem_start = 0;
    info->fix.smem_len = 0;
    mutex_unlock(&info->mm_lock);

    if (p)
        free_pages_exact(p, l);
    ...
}

Since the pointer p = info->screen_base is read outside the lock, couldn't
both racing threads capture the same pointer, clear the base inside the lock
sequentially, and then unconditionally execute a double free on the identical
pointer?

>  	if (!data)
>  		return -ENOMEM;
>  	data->dma_addr = dma_addr;

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