Re: [PATCH 2/2] drm/sysfb: Return errno code from drm_sysfb_get_visible_size()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] 32-bit integer overflow in `height * stride` calculation leading to out-of-bounds I/O memory mapping and subsequent out-of-bounds writes.
- [Medium] Truncation of `div64_u64` result when passed to the `u32 max` parameter of `drm_sysfb_get_validated_int0`.
--

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

drm/sysfb: Return errno code from drm_sysfb_get_visible_size()
    
Change the return type of drm_sysfb_get_visible_size() to s64 so
that it returns a possible errno code from _get_validated_size0().
Fix callers to handle the errno code.

> diff --git a/drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c b/drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c
> index 361b7233600c2..40b4f7883b9bc 100644
> --- a/drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c
> +++ b/drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c
> @@ -64,7 +64,7 @@ int drm_sysfb_get_stride_si(struct drm_device *dev, const struct screen_info *si
>  }
>  EXPORT_SYMBOL(drm_sysfb_get_stride_si);
>  
> -u64 drm_sysfb_get_visible_size_si(struct drm_device *dev, const struct screen_info *si,
> +s64 drm_sysfb_get_visible_size_si(struct drm_device *dev, const struct screen_info *si,
>  				  unsigned int height, unsigned int stride, u64 size)
>  {
>  	u64 vsize = height * stride;

[Severity: High]
This is a pre-existing issue, but does this calculation overflow if height
and stride are large?

Both height and stride are passed as 32-bit unsigned integers to
drm_sysfb_get_visible_size_si(), so their product is computed as a 32-bit
unsigned integer before being implicitly extended to the 64-bit vsize.

If they overflow (e.g., height up to U16_MAX and stride up to U16_MAX * 4),
the truncated 32-bit value could bypass the bounds check in
drm_sysfb_get_validated_size0(). This could result in requesting and mapping
a falsely small I/O memory region for the framebuffer, potentially leading
to an out-of-bounds memory write later.

Could explicit 64-bit casting or mul_u32_u32() be used to prevent this?

[Severity: Medium]
This isn't a bug introduced by this patch, but looking at
drm_sysfb_get_stride_si() located just above this in
drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c:

int drm_sysfb_get_stride_si(...)
{
    ...
    return drm_sysfb_get_validated_int0(dev, "stride", lfb_linelength,
                                        div64_u64(size, height));
}

Could the result of div64_u64(size, height) be truncated when passed to
drm_sysfb_get_validated_int0()?

The max parameter of drm_sysfb_get_validated_int0() is typed as u32. If
the total resource size is large and the height is small (for example,
size = 4GB and height = 1), the division result would overflow the u32 type
and truncate to 0.

This would cause drm_sysfb_get_validated_int0() to use a max value of 0,
falsely rejecting a valid configuration with -EINVAL.

>  	return drm_sysfb_get_validated_size0(dev, "visible size", vsize, size);
>  }

[ ... ]

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