Re: [PATCH 1/3] drm/sysfb: simpledrm: Improve framebuffer-size validation

[sashiko-bot@kernel.org]

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

New issues:
- [Low] The patch updates all callers of the local static wrapper `simplefb_get_validated_int0()` to call `drm_sysfb_get_validated_int0()` directly, but leaves the static function definition behind, causing a compiler warning.

Pre-existing issues:
- [High] Missing validation between the firmware-provided framebuffer dimensions (stride, width, height) and the size of the memory region allows out-of-bounds virtual memory writes.
--

commit 934b7fe8edd3f05582affbe138506d555774f080
Author: Thomas Zimmermann <tzimmermann@suse.de>

drm/sysfb: simpledrm: Improve framebuffer-size validation

Validate the framebuffer size from the firmware against the limitations of
struct drm_display_mode. Fail probing on errors if the size exceeds the
16-bit fields.

> diff --git a/drivers/gpu/drm/sysfb/simpledrm.c b/drivers/gpu/drm/sysfb/simpledrm.c
> index fc168920f2c64..03bd19fadccd5 100644
> --- a/drivers/gpu/drm/sysfb/simpledrm.c
> +++ b/drivers/gpu/drm/sysfb/simpledrm.c
> @@ -88,14 +88,14 @@ static int
>  simplefb_get_width_pd(struct drm_device *dev,
>  		      const struct simplefb_platform_data *pd)
>  {
> -	return simplefb_get_validated_int0(dev, "width", pd->width);
> +	return drm_sysfb_get_validated_int0(dev, "width", pd->width, U16_MAX);
>  }

[Severity: Low]
Does this leave simplefb_get_validated_int0() as an unused static function?

Since all four callers (simplefb_get_width_pd, simplefb_get_height_pd,
simplefb_get_width_of, simplefb_get_height_of) are updated to call
drm_sysfb_get_validated_int0() directly, simplefb_get_validated_int0()
appears to have zero callers remaining in simpledrm.c. This can trigger a
-Wunused-function compiler warning.

[Severity: High]
This isn't a bug introduced by this patch, but while we are improving the
validation of framebuffer dimensions from firmware, should we also validate
that the resulting framebuffer size actually fits within the mapped memory
region?

The firmware-provided stride and height are used to calculate the framebuffer
size, but when acquiring the memory region:

drivers/gpu/drm/sysfb/simpledrm.c:simpledrm_device_create() {
    ...
	screen_base = devm_memremap(dev->dev, mem->start, resource_size(mem), MEMREMAP_WC);
    ...
}

or the fallback:

drivers/gpu/drm/sysfb/simpledrm.c:simpledrm_device_create() {
    ...
	screen_base = devm_ioremap_wc(&pdev->dev, mem->start, resource_size(mem));
    ...
}

It seems we map exactly resource_size(mem) bytes without verifying if
(stride * height) exceeds that size. If the firmware provides a mismatched
configuration, out-of-bounds writes could occur during plane updates when
copying pixel data into sysfb->fb_addr.

[ ... ]

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