Re: [PATCH v2 5/6] 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:
- [High] The validation check comparing the memory resource size against the required framebuffer size is inverted, causing valid device tree configurations to fail probing and bypassing protection for undersized framebuffers.
- [Low] The condition `if (size < PAGE_SIZE)` functions as a check for a zero-sized framebuffer rather than an alignment overflow guard, resulting in a misleading error message.
--

commit fe687b775049a45634b1930d3d958d911916b07a
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.

Buggy firmware that triggers this check should be fixed up with a quirk
on a case-by-case base.

> diff --git a/drivers/gpu/drm/sysfb/simpledrm.c b/drivers/gpu/drm/sysfb/simpledrm.c
> index 76b9a3f5c4efa..a04f0c852ea48 100644
> --- a/drivers/gpu/drm/sysfb/simpledrm.c
> +++ b/drivers/gpu/drm/sysfb/simpledrm.c

[ ... ]

> @@ -704,6 +706,15 @@ 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);
> +	}
> +	size = ALIGN(size, PAGE_SIZE);
> +	if (size < PAGE_SIZE) {
> +		drm_err(dev, "framebuffer alignment exceeds maximum\n");

[Severity: Low]
Does this code accurately report an alignment overflow?

Because both height and stride are bounded by INT_MAX, their product
(size) cannot exceed ~2^62, meaning the ALIGN operation will not overflow
a 64-bit integer.

The condition size < PAGE_SIZE will only be true if size was exactly 0
before alignment. While rejecting a zero-sized framebuffer is correct,
the error message 'framebuffer alignment exceeds maximum' might be
misleading for developers debugging initialization failures.

> +		return ERR_PTR(-EINVAL);
> +	}
>  
>  	sysfb->fb_mode = drm_sysfb_mode(width, height, width_mm, height_mm);
>  	sysfb->fb_format = format;

[ ... ]

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

[Severity: High]
Is the validation check here inverted?

The condition evaluates to true when the provided memory region is larger
than the required framebuffer size, which is a valid configuration when
the reserved-memory region is larger than the exact resolution requires.

This logic inversion causes the driver to incorrectly log 'framebuffer size...
exceeds memory range' and abort probing for valid configurations, while
allowing memory regions that are actually too small.

Will this bypass the intended validation and potentially lead to out-of-bounds
memory accesses if the framebuffer is not fully backed by the memory resource?

> +				size, mem);
> +			return ERR_PTR(-EINVAL);
> +		}
> +
>  		screen_base = devm_memremap(dev->dev, mem->start, resource_size(mem), MEMREMAP_WC);
>  		if (IS_ERR(screen_base))
>  			return screen_base;

[ ... ]

> @@ -762,6 +780,13 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>  			mem = res;
>  		}
>  
> +		if (resource_size(mem) > size) {
> +			drm_err(dev,
> +				"framebuffer size of %llu exceeds memory range %pr\n",

[Severity: High]
Is this check also inverted for the I/O memory path?

Similar to the system memory path above, this will reject valid framebuffers
that fit within the memory range and allow undersized ones.

> +				size, mem);
> +			return ERR_PTR(-EINVAL);
> +		}
> +
>  		screen_base = devm_ioremap_wc(&pdev->dev, mem->start, resource_size(mem));

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