[PATCH 0/2] drm/sysfb: Fix visible size and related checks

[PATCH 0/2] drm/sysfb: Fix visible size and related checks

[Thomas Zimmermann]

We need the system framebuffer's visible size in efidrm and
vesadrm. Fix the size calculation for some corner cases and
handle errors correctly.

Thomas Zimmermann (2):
  drm/sysfb: Do not page-align visible size of the framebuffer
  drm/sysfb: Return errno code from drm_sysfb_get_visible_size()

 drivers/gpu/drm/sysfb/drm_sysfb_helper.h      | 2 +-
 drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c | 4 ++--
 drivers/gpu/drm/sysfb/efidrm.c                | 7 ++++---
 drivers/gpu/drm/sysfb/vesadrm.c               | 6 +++---
 4 files changed, 10 insertions(+), 9 deletions(-)


base-commit: fc59f76558703febba8056be87d1c97d14f7485e
-- 
2.54.0

[PATCH 1/2] drm/sysfb: Do not page-align visible size of the framebuffer

[Thomas Zimmermann]

Only return the actually visible size of the system framebuffer in
drm_sysfb_get_visible_size_si(). Drivers use this size value for
reserving access to framebuffer memory. Increasing the value can
make later attempts to do so fail.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: 32ae90c66fb6 ("drm/sysfb: Add efidrm for EFI displays")
Fixes: a84eb6abe2b6 ("drm/sysfb: Add vesadrm for VESA displays")
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Javier Martinez Canillas <javierm@redhat.com>
Cc: dri-devel@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v6.16+
---
 drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c b/drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c
index 749290196c6a..361b7233600c 100644
--- a/drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c
+++ b/drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c
@@ -67,7 +67,7 @@ EXPORT_SYMBOL(drm_sysfb_get_stride_si);
 u64 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 = PAGE_ALIGN(height * stride);
+	u64 vsize = height * stride;
 
 	return drm_sysfb_get_validated_size0(dev, "visible size", vsize, size);
 }
-- 
2.54.0

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

[Thomas Zimmermann]

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.

The currently returned unsigned type converts an errno code to a
very large size value, which drivers interpret as visible size of
the system framebuffer. Later efforts to reserve the framebuffer
resource fail.

The bug has been present since efidrm and vesadrm got merged. It
was then part of each driver.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: 32ae90c66fb6 ("drm/sysfb: Add efidrm for EFI displays")
Fixes: a84eb6abe2b6 ("drm/sysfb: Add vesadrm for VESA displays")
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Javier Martinez Canillas <javierm@redhat.com>
Cc: dri-devel@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v6.16+
---
 drivers/gpu/drm/sysfb/drm_sysfb_helper.h      | 2 +-
 drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c | 2 +-
 drivers/gpu/drm/sysfb/efidrm.c                | 7 ++++---
 drivers/gpu/drm/sysfb/vesadrm.c               | 6 +++---
 4 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/sysfb/drm_sysfb_helper.h b/drivers/gpu/drm/sysfb/drm_sysfb_helper.h
index 2a2b553366fb..547f2327af5e 100644
--- a/drivers/gpu/drm/sysfb/drm_sysfb_helper.h
+++ b/drivers/gpu/drm/sysfb/drm_sysfb_helper.h
@@ -50,7 +50,7 @@ struct resource *drm_sysfb_get_memory_si(struct drm_device *dev,
 int drm_sysfb_get_stride_si(struct drm_device *dev, const struct screen_info *si,
 			    const struct drm_format_info *format,
 			    unsigned int width, unsigned int height, u64 size);
-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);
 #endif
 
diff --git a/drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c b/drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c
index 361b7233600c..40b4f7883b9b 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;
diff --git a/drivers/gpu/drm/sysfb/efidrm.c b/drivers/gpu/drm/sysfb/efidrm.c
index 1a1e36700976..3f9cd5d03efb 100644
--- a/drivers/gpu/drm/sysfb/efidrm.c
+++ b/drivers/gpu/drm/sysfb/efidrm.c
@@ -152,7 +152,8 @@ static struct efidrm_device *efidrm_device_create(struct drm_driver *drv,
 	const struct screen_info *si;
 	const struct drm_format_info *format;
 	int width, height, stride;
-	u64 vsize, mem_flags;
+	s64 vsize;
+	u64 mem_flags;
 	struct resource resbuf;
 	struct resource *res;
 	struct efidrm_device *efi;
@@ -206,8 +207,8 @@ static struct efidrm_device *efidrm_device_create(struct drm_driver *drv,
 	if (stride < 0)
 		return ERR_PTR(stride);
 	vsize = drm_sysfb_get_visible_size_si(dev, si, height, stride, resource_size(res));
-	if (!vsize)
-		return ERR_PTR(-EINVAL);
+	if (vsize < 0)
+		return ERR_PTR(vsize);
 
 	drm_dbg(dev, "framebuffer format=%p4cc, size=%dx%d, stride=%d bytes\n",
 		&format->format, width, height, stride);
diff --git a/drivers/gpu/drm/sysfb/vesadrm.c b/drivers/gpu/drm/sysfb/vesadrm.c
index dbc317778d54..6a67b2d2e451 100644
--- a/drivers/gpu/drm/sysfb/vesadrm.c
+++ b/drivers/gpu/drm/sysfb/vesadrm.c
@@ -402,7 +402,7 @@ static struct vesadrm_device *vesadrm_device_create(struct drm_driver *drv,
 	const struct screen_info *si;
 	const struct drm_format_info *format;
 	int width, height, stride;
-	u64 vsize;
+	s64 vsize;
 	struct resource resbuf;
 	struct resource *res;
 	struct vesadrm_device *vesa;
@@ -457,8 +457,8 @@ static struct vesadrm_device *vesadrm_device_create(struct drm_driver *drv,
 	if (stride < 0)
 		return ERR_PTR(stride);
 	vsize = drm_sysfb_get_visible_size_si(dev, si, height, stride, resource_size(res));
-	if (!vsize)
-		return ERR_PTR(-EINVAL);
+	if (vsize < 0)
+		return ERR_PTR(vsize);
 
 	drm_dbg(dev, "framebuffer format=%p4cc, size=%dx%d, stride=%d bytes\n",
 		&format->format, width, height, stride);
-- 
2.54.0

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

[sashiko-bot]

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

Re: [PATCH 1/2] drm/sysfb: Do not page-align visible size of the framebuffer

[sashiko-bot]

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

Pre-existing issues:
- [High] 32-bit integer overflow in `height * stride` calculation causes truncated visible size for framebuffers >4GB, leading to out-of-bounds kernel memory accesses.
--

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

drm/sysfb: Do not page-align visible size of the framebuffer

This commit stops page-aligning the visible size of the system
framebuffer in drm_sysfb_get_visible_size_si(). Drivers use this size
value for reserving access to framebuffer memory, and increasing the
value can make later attempts to reserve memory fail.

> diff --git a/drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c b/drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c
> --- a/drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c
> +++ b/drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c
> @@ -67,7 +67,7 @@ EXPORT_SYMBOL(drm_sysfb_get_stride_si);
>  u64 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 = PAGE_ALIGN(height * stride);
> +	u64 vsize = height * stride;

[Severity: High]
This is a pre-existing issue, but does this calculation overflow on 32-bit
arithmetic before assignment?

Both height and stride are passed as unsigned int. If the system provides a
framebuffer resource larger than 4GB, the height * stride product could
overflow and truncate before being zero-extended to u64 vsize.

This truncated value could pass validation against the resource bounds
and lead to an undersized devm_ioremap_wc(). Could this cause out-of-bounds
kernel memory accesses during subsequent DRM operations, such as
drm_fb_memcpy(), if they iterate using the original non-truncated dimensions?

>  
>  	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=1

Re: [PATCH 1/2] drm/sysfb: Do not page-align visible size of the framebuffer

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

Hello Thomas,

> Only return the actually visible size of the system framebuffer in
> drm_sysfb_get_visible_size_si(). Drivers use this size value for
> reserving access to framebuffer memory. Increasing the value can
> make later attempts to do so fail.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: 32ae90c66fb6 ("drm/sysfb: Add efidrm for EFI displays")
> Fixes: a84eb6abe2b6 ("drm/sysfb: Add vesadrm for VESA displays")
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Javier Martinez Canillas <javierm@redhat.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v6.16+
> ---
>  drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c b/drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c
> index 749290196c6a..361b7233600c 100644
> --- a/drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c
> +++ b/drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c
> @@ -67,7 +67,7 @@ EXPORT_SYMBOL(drm_sysfb_get_stride_si);
>  u64 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 = PAGE_ALIGN(height * stride);

Do you know why the original efidrm_get_visible_size_si() (from where
you took this code to make it generic) did the page align ?

The change makes sense to me though from your explanation:

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

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

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> 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.
>
> The currently returned unsigned type converts an errno code to a
> very large size value, which drivers interpret as visible size of
> the system framebuffer. Later efforts to reserve the framebuffer
> resource fail.
>
> The bug has been present since efidrm and vesadrm got merged. It
> was then part of each driver.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: 32ae90c66fb6 ("drm/sysfb: Add efidrm for EFI displays")
> Fixes: a84eb6abe2b6 ("drm/sysfb: Add vesadrm for VESA displays")
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Javier Martinez Canillas <javierm@redhat.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v6.16+
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 1/2] drm/sysfb: Do not page-align visible size of the framebuffer

[Thomas Zimmermann]

Hi

Am 17.06.26 um 13:44 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
>
> Hello Thomas,
>
>> Only return the actually visible size of the system framebuffer in
>> drm_sysfb_get_visible_size_si(). Drivers use this size value for
>> reserving access to framebuffer memory. Increasing the value can
>> make later attempts to do so fail.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Fixes: 32ae90c66fb6 ("drm/sysfb: Add efidrm for EFI displays")
>> Fixes: a84eb6abe2b6 ("drm/sysfb: Add vesadrm for VESA displays")
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: Javier Martinez Canillas <javierm@redhat.com>
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: <stable@vger.kernel.org> # v6.16+
>> ---
>>   drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c b/drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c
>> index 749290196c6a..361b7233600c 100644
>> --- a/drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c
>> +++ b/drivers/gpu/drm/sysfb/drm_sysfb_screen_info.c
>> @@ -67,7 +67,7 @@ EXPORT_SYMBOL(drm_sysfb_get_stride_si);
>>   u64 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 = PAGE_ALIGN(height * stride);
> Do you know why the original efidrm_get_visible_size_si() (from where
> you took this code to make it generic) did the page align ?

There's nothing in the old reviews AFAICT. I think, it was an oversight 
from prototyping the driver. I certainly wanted to return 0 for errors 
at first, but later changed it to an errno code. There, I forgot to 
update the code accordingly.

>
> The change makes sense to me though from your explanation:
>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Thanks.

Best regards
Thomas

>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)