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

[sashiko-bot@kernel.org]

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