Re: [PATCH] drm/sysfb: ofdrm: Fix integer overflow in fb_size calculation

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

Pre-existing issues:
- [High] The driver accepts any non-zero `linebytes` from the Device Tree without ensuring it is at least as large as `width * bytes_per_pixel`.
- [High] The 64-bit result of `drm_format_info_min_pitch()` is silently truncated when assigned to the 32-bit `int linebytes` variable.
- [High] The `round_up(fb_size, PAGE_SIZE)` calculation can overflow to 0 on 32-bit systems, leading to an undersized memory mapping.
- [High] On 32-bit architectures with 64-bit physical addresses (PAE/LPAE), passing a 64-bit size to `ioremap` via `devm_ioremap` truncates it to a 32-bit `size_t`.
--

--- Patch [1]: [PATCH] drm/sysfb: ofdrm: Fix integer overflow in fb_size calculation ---
commit ad44f8947b62f451f5098acf1a574e6b16699aa6
Author: Shixiong Ou <[email protected]>

drm/sysfb: ofdrm: Fix integer overflow in fb_size calculation

The framebuffer size calculation fb_size = linebytes * height can
overflow when both values are large (e.g., 46341 * 46341 > INT_MAX).
Since linebytes and height are both int types, the multiplication is
performed as int * int, which results in undefined behavior on overflow.

Use check_mul_overflow() to detect and prevent this overflow, consistent
with the approach used in simpledrm.c and corebootdrm.c.

> diff --git a/drivers/gpu/drm/sysfb/ofdrm.c b/drivers/gpu/drm/sysfb/ofdrm.c
> index 819aed466727e..a6dc34b9ec0f3 100644
> --- a/drivers/gpu/drm/sysfb/ofdrm.c
> +++ b/drivers/gpu/drm/sysfb/ofdrm.c
> @@ -913,7 +914,10 @@ static struct ofdrm_device *ofdrm_device_create(struct drm_driver *drv,
>  			return ERR_PTR(-EINVAL);
>  	}
>  
> -	fb_size = linebytes * height;
> +	if (check_mul_overflow(linebytes, height, &fb_size)) {

[Severity: High]
This isn't a bug introduced by this patch, but does drm_format_info_min_pitch()
called earlier in this function return a 64-bit value that is silently truncated
when assigned to the 32-bit int linebytes variable?

	if (!linebytes) {
		linebytes = drm_format_info_min_pitch(format, 0, width);
		if (drm_WARN_ON(dev, !linebytes))
			return ERR_PTR(-EINVAL);
	}

If width is maliciously large (e.g., 0x40000001), the 64-bit result could be
truncated to 4, bypassing the !linebytes check and the new check_mul_overflow()
check.

[Severity: High]
This is a pre-existing issue, but is there a missing lower bound check for
linebytes in the same block above? 

If the Device Tree provides a maliciously small but non-zero linebytes (e.g., 4),
it bypasses the !linebytes check. It would also pass the new
check_mul_overflow() check but could cause out-of-bounds memory writes
downstream when DRM plane updates write data based on the logical width.

[Severity: High]
This is a pre-existing issue, but on 32-bit systems where resource_size_t is
32-bit, could round_up(fb_size, PAGE_SIZE) computed later in this function
overflow and wrap around to 0?

	fb_pgbase = round_down(fb_base, PAGE_SIZE);
	fb_pgsize = fb_base - fb_pgbase + round_up(fb_size, PAGE_SIZE);

If fb_size is near U32_MAX, the overflow check succeeds, but this calculation
would result in an undersized memory mapping.

[Severity: High]
This is a pre-existing issue, but on 32-bit architectures with 64-bit physical
addresses (PAE/LPAE), does devm_ioremap() silently truncate the 64-bit size to a
32-bit size_t? 

	screen_base = devm_ioremap(&pdev->dev, mem->start, resource_size(mem));

Since check_mul_overflow() checks against the 64-bit resource_size_t fb_size, a
massive size wouldn't be caught there, and would be truncated here, leading to a
smaller mapping than the DRM core expects.

> +		drm_err(dev, "framebuffer size exceeds maximum\n");
> +		return ERR_PTR(-EINVAL);
> +	}
>  
>  	/*
>  	 * Try to figure out the address of the framebuffer. Unfortunately, Open

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1