Re: [PATCH v3 5/7] drm/sysfb: simpledrm: Validate mmap size against framebuffer size

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Dead code in mathematical overflow checks for framebuffer size and alignment

Pre-existing issues:
- [High] Missing bounds check for explicitly provided stride allows out-of-bounds writes
--

commit 192f927f236077dbe56914205861b2c980ca71ec
Author: Thomas Zimmermann <tzimmermann@suse.de>

drm/sysfb: simpledrm: Validate mmap size against framebuffer size

The size of the mmap'ed framebuffer could be smaller than the minimum
required framebuffer size. Validate the resource size against the
framebuffer size.

> diff --git a/drivers/gpu/drm/sysfb/simpledrm.c b/drivers/gpu/drm/sysfb/simpledrm.c
> index a899dfb747bfa..9e0711e0095a5 100644
> --- a/drivers/gpu/drm/sysfb/simpledrm.c
> +++ b/drivers/gpu/drm/sysfb/simpledrm.c

[ ... ]

> @@ -623,6 +624,7 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>  	u16 width_mm = 0, height_mm = 0;
>  	struct device_node *panel_node;
>  	const struct drm_format_info *format;
> +	u64 size;
>  	struct resource *res, *mem = NULL;
>  	struct drm_plane *primary_plane;
>  	struct drm_crtc *crtc;

[ ... ]

> @@ -713,6 +715,14 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>  		}
>  		stride = pitch;
>  	}
> +	if (check_mul_overflow(height, stride, &size)) {
> +		drm_err(dev, "framebuffer size exceeds maximum\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +	if (ALIGN(size, PAGE_SIZE) < PAGE_SIZE) {
> +		drm_err(dev, "page-aligned framebuffer exceeds maximum\n");
> +		return ERR_PTR(-EINVAL);
> +	}

[Severity: Medium]
Are these overflow checks functioning as intended?

Looking at simpledrm_device_create() in drivers/gpu/drm/sysfb/simpledrm.c,
height and stride are 32-bit integers while size is a u64. Their maximum
possible product (~1.4e14) easily fits into size (~1.8e19), meaning
check_mul_overflow() mathematically cannot overflow here.

Additionally, the alignment check ALIGN(size, PAGE_SIZE) < PAGE_SIZE
only evaluates to true if size == 0. Since both height and stride are
guaranteed to be strictly greater than 0 by earlier validation, size
cannot be 0. As a u64, the PAGE_SIZE alignment cannot wrap around,
making this check dead code.

>  
>  	sysfb->fb_mode = drm_sysfb_mode(width, height, width_mm, height_mm);
>  	sysfb->fb_format = format;

[ ... ]

> @@ -738,6 +748,13 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>  
>  		drm_dbg(dev, "using system memory framebuffer at %pr\n", mem);
>  
> +		if (size > resource_size(mem)) {
> +			drm_err(dev,
> +				"framebuffer size of %llu exceeds memory range %pr\n",
> +				size, mem);
> +			return ERR_PTR(-EINVAL);
> +		}
> +

[Severity: High]
This isn't a bug introduced by this patch, but does the size calculation
allow out-of-bounds writes if the device tree provides an artificially
small stride?

Looking at simpledrm_device_create() in drivers/gpu/drm/sysfb/simpledrm.c,
if stride is provided explicitly by the device tree, it is never validated
against the minimum required pitch (width * bytes_per_pixel). A buggy or
malicious device tree could provide a stride of 1, resulting in an
artificially small computed size that easily passes this new
size > resource_size(mem) check.

During atomic updates, DRM shadow plane blitting functions like
drm_fb_memcpy() copy width * bytes_per_pixel bytes per row. The total
memory offset accessed would reach:
(height - 1) * stride + width * bytes_per_pixel

With an artificially small stride, this could greatly exceed the validated
size and mapped resource_size(mem), leading to an out-of-bounds write
into kernel or I/O memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625094509.157581-1-tzimmermann@suse.de?part=5