[PATCH v2 0/6] drm/sysfb: simpledrm: Various improvements

[PATCH v2 0/6] drm/sysfb: simpledrm: Various improvements

[Thomas Zimmermann]

Patches 1 to 5 improve validation of the values provided by the
device firmware.

Patch 6 improves support for panel rotation. There's a rotation
property in the DT panel node. Read the value and set the connector's
orientation property accordingly.

v2:
- further improve input validation (Sashiko)

Thomas Zimmermann (6):
  drm/sysfb: simpledrm: Improve framebuffer-size validation
  drm/sysfb: simpledrm: Improve panel-size validation
  drm/sysfb: simpledrm: Inline simplefb_get_validated_int()
  drm/sysfb: simpledrm: Improve stride validation
  drm/sysfb: simpledrm: Validate mmap size against framebuffer size
  drm/sysfb: simpledrm: Read panel orientation from DT node

 drivers/gpu/drm/sysfb/simpledrm.c | 133 ++++++++++++++++++++++++------
 1 file changed, 106 insertions(+), 27 deletions(-)


base-commit: fc59f76558703febba8056be87d1c97d14f7485e
-- 
2.54.0

[PATCH v2 1/6] drm/sysfb: simpledrm: Improve framebuffer-size validation

[Thomas Zimmermann]

Validate the framebuffer size from the firmware against the
limitations of struct drm_display_mode. The type only stores sizes
in 16-bit fields. Fail probing on errors.

v2:
- remove unused function simplefb_get_validated_int0() (Sashiko)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: 11e8f5fd223b ("drm: Add simpledrm driver")
Cc: <stable@vger.kernel.org> # v5.14+
---
 drivers/gpu/drm/sysfb/simpledrm.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/sysfb/simpledrm.c b/drivers/gpu/drm/sysfb/simpledrm.c
index fc168920f2c6..15dcafa9d524 100644
--- a/drivers/gpu/drm/sysfb/simpledrm.c
+++ b/drivers/gpu/drm/sysfb/simpledrm.c
@@ -48,13 +48,6 @@ simplefb_get_validated_int(struct drm_device *dev, const char *name,
 	return drm_sysfb_get_validated_int(dev, name, value, INT_MAX);
 }
 
-static int
-simplefb_get_validated_int0(struct drm_device *dev, const char *name,
-			    uint32_t value)
-{
-	return drm_sysfb_get_validated_int0(dev, name, value, INT_MAX);
-}
-
 static const struct drm_format_info *
 simplefb_get_validated_format(struct drm_device *dev, const char *format_name)
 {
@@ -88,14 +81,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);
 }
 
 static int
 simplefb_get_height_pd(struct drm_device *dev,
 		       const struct simplefb_platform_data *pd)
 {
-	return simplefb_get_validated_int0(dev, "height", pd->height);
+	return drm_sysfb_get_validated_int0(dev, "height", pd->height, U16_MAX);
 }
 
 static int
@@ -144,7 +137,7 @@ simplefb_get_width_of(struct drm_device *dev, struct device_node *of_node)
 
 	if (ret)
 		return ret;
-	return simplefb_get_validated_int0(dev, "width", width);
+	return drm_sysfb_get_validated_int0(dev, "width", width, U16_MAX);
 }
 
 static int
@@ -155,7 +148,7 @@ simplefb_get_height_of(struct drm_device *dev, struct device_node *of_node)
 
 	if (ret)
 		return ret;
-	return simplefb_get_validated_int0(dev, "height", height);
+	return drm_sysfb_get_validated_int0(dev, "height", height, U16_MAX);
 }
 
 static int
-- 
2.54.0

[PATCH v2 2/6] drm/sysfb: simpledrm: Improve panel-size validation

[Thomas Zimmermann]

Validate the panel size from the device-tree node against the
limitations of struct drm_display_mode. The type only stores sizes
in 16-bit fields. Fail transparently on errors; do not warn.

v2:
- only use initialized values in debugging output (Sashiko)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: 2a6d731a8f16 ("drm/simpledrm: Allow physical width and height configuration via panel node")
Cc: Rayyan Ansari <rayyan@ansari.sh>
Cc: <stable@vger.kernel.org> # v6.4+
---
 drivers/gpu/drm/sysfb/simpledrm.c | 40 ++++++++++++++++++++++++++++---
 1 file changed, 37 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/sysfb/simpledrm.c b/drivers/gpu/drm/sysfb/simpledrm.c
index 15dcafa9d524..fa2121f81def 100644
--- a/drivers/gpu/drm/sysfb/simpledrm.c
+++ b/drivers/gpu/drm/sysfb/simpledrm.c
@@ -193,6 +193,40 @@ simplefb_get_memory_of(struct drm_device *dev, struct device_node *of_node)
 	return res;
 }
 
+static u16
+__simplefb_get_panel_size_mm_of(struct drm_device *dev, struct device_node *of_panel_node,
+				const char *name)
+{
+	int ret;
+	u32 value;
+
+	ret = of_property_read_u32(of_panel_node, name, &value);
+	if (ret) {
+		drm_dbg(dev, "simplefb: cannot parse panel %s: error %d\n",
+			name, ret);
+		return 0; /* not an error, simply ignore */
+	}
+	if (value > U16_MAX) {
+		drm_dbg(dev, "simplefb: panel %s of %u exceeds maximum value\n",
+			name, value);
+		return 0; /* not an error, simply ignore */
+	}
+
+	return value;
+}
+
+static u16
+simplefb_get_panel_width_mm_of(struct drm_device *dev, struct device_node *of_panel_node)
+{
+	return __simplefb_get_panel_size_mm_of(dev, of_panel_node, "width-mm");
+}
+
+static u16
+simplefb_get_panel_height_mm_of(struct drm_device *dev, struct device_node *of_panel_node)
+{
+	return __simplefb_get_panel_size_mm_of(dev, of_panel_node, "height-mm");
+}
+
 /*
  * Simple Framebuffer device
  */
@@ -594,7 +628,7 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 	struct drm_sysfb_device *sysfb;
 	struct drm_device *dev;
 	int width, height, stride;
-	int width_mm = 0, height_mm = 0;
+	u16 width_mm = 0, height_mm = 0;
 	struct device_node *panel_node;
 	const struct drm_format_info *format;
 	struct resource *res, *mem = NULL;
@@ -658,8 +692,8 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 			return ERR_CAST(mem);
 		panel_node = of_parse_phandle(of_node, "panel", 0);
 		if (panel_node) {
-			simplefb_read_u32_of(dev, panel_node, "width-mm", &width_mm);
-			simplefb_read_u32_of(dev, panel_node, "height-mm", &height_mm);
+			width_mm = simplefb_get_panel_width_mm_of(dev, panel_node);
+			height_mm = simplefb_get_panel_height_mm_of(dev, panel_node);
 			of_node_put(panel_node);
 		}
 	} else {
-- 
2.54.0

[PATCH v2 3/6] drm/sysfb: simpledrm: Inline simplefb_get_validated_int()

[Thomas Zimmermann]

The helper simplefb_get_validated_int() is only a wrapper around
drm_sysfb_get_validated_int(). Inline the function into its callers.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/sysfb/simpledrm.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/sysfb/simpledrm.c b/drivers/gpu/drm/sysfb/simpledrm.c
index fa2121f81def..709922ea656c 100644
--- a/drivers/gpu/drm/sysfb/simpledrm.c
+++ b/drivers/gpu/drm/sysfb/simpledrm.c
@@ -41,13 +41,6 @@
  * Helpers for simplefb
  */
 
-static int
-simplefb_get_validated_int(struct drm_device *dev, const char *name,
-			   uint32_t value)
-{
-	return drm_sysfb_get_validated_int(dev, name, value, INT_MAX);
-}
-
 static const struct drm_format_info *
 simplefb_get_validated_format(struct drm_device *dev, const char *format_name)
 {
@@ -95,7 +88,7 @@ static int
 simplefb_get_stride_pd(struct drm_device *dev,
 		       const struct simplefb_platform_data *pd)
 {
-	return simplefb_get_validated_int(dev, "stride", pd->stride);
+	return drm_sysfb_get_validated_int(dev, "stride", pd->stride, INT_MAX);
 }
 
 static const struct drm_format_info *
@@ -159,7 +152,7 @@ simplefb_get_stride_of(struct drm_device *dev, struct device_node *of_node)
 
 	if (ret)
 		return ret;
-	return simplefb_get_validated_int(dev, "stride", stride);
+	return drm_sysfb_get_validated_int(dev, "stride", stride, INT_MAX);
 }
 
 static const struct drm_format_info *
-- 
2.54.0

[PATCH v2 4/6] drm/sysfb: simpledrm: Improve stride validation

[Thomas Zimmermann]

Validate the computed stride against the maximum value INT_MAX.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: 7bfa5c7b28d6 ("drm/simpledrm: Compute linestride with drm_format_info_min_pitch()")
Cc: <stable@vger.kernel.org> # v6.1+
---
 drivers/gpu/drm/sysfb/simpledrm.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/sysfb/simpledrm.c b/drivers/gpu/drm/sysfb/simpledrm.c
index 709922ea656c..76b9a3f5c4ef 100644
--- a/drivers/gpu/drm/sysfb/simpledrm.c
+++ b/drivers/gpu/drm/sysfb/simpledrm.c
@@ -694,9 +694,15 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 		return ERR_PTR(-ENODEV);
 	}
 	if (!stride) {
-		stride = drm_format_info_min_pitch(format, 0, width);
-		if (drm_WARN_ON(dev, !stride))
+		u64 pitch = drm_format_info_min_pitch(format, 0, width);
+
+		if (drm_WARN_ON(dev, !pitch)) {
+			return ERR_PTR(-EINVAL); /* driver bug */
+		} else if (pitch > INT_MAX) {
+			drm_warn(dev, "stride of %llu exceeds maximum\n", pitch);
 			return ERR_PTR(-EINVAL);
+		}
+		stride = pitch;
 	}
 
 	sysfb->fb_mode = drm_sysfb_mode(width, height, width_mm, height_mm);
-- 
2.54.0

[PATCH v2 5/6] drm/sysfb: simpledrm: Validate mmap size against framebuffer size

[Thomas Zimmermann]

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.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Suggested-by: Sashiko <sashiko-bot@kernel.org>
---
 drivers/gpu/drm/sysfb/simpledrm.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/sysfb/simpledrm.c b/drivers/gpu/drm/sysfb/simpledrm.c
index 76b9a3f5c4ef..a04f0c852ea4 100644
--- a/drivers/gpu/drm/sysfb/simpledrm.c
+++ b/drivers/gpu/drm/sysfb/simpledrm.c
@@ -6,6 +6,7 @@
 #include <linux/of_address.h>
 #include <linux/of_clk.h>
 #include <linux/of_reserved_mem.h>
+#include <linux/overflow.h>
 #include <linux/platform_data/simplefb.h>
 #include <linux/platform_device.h>
 #include <linux/pm.h>
@@ -624,6 +625,7 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 	u16 width_mm = 0, height_mm = 0;
 	struct device_node *panel_node;
 	const struct drm_format_info *format;
+	u64 size;
 	struct resource *res, *mem = NULL;
 	struct drm_plane *primary_plane;
 	struct drm_crtc *crtc;
@@ -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");
+		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",
+				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",
+				size, mem);
+			return ERR_PTR(-EINVAL);
+		}
+
 		screen_base = devm_ioremap_wc(&pdev->dev, mem->start, resource_size(mem));
 		if (!screen_base)
 			return ERR_PTR(-ENOMEM);
-- 
2.54.0

[PATCH v2 6/6] drm/sysfb: simpledrm: Read panel orientation from DT node

[Thomas Zimmermann]

A DeviceTree panel node can specify the panel's rotation in steps
of 90 degrees. Set the DRM connector orientation from the field.
Do nothing if the field is missing or invalid.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/sysfb/simpledrm.c | 32 +++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/sysfb/simpledrm.c b/drivers/gpu/drm/sysfb/simpledrm.c
index a04f0c852ea4..b585d17ca817 100644
--- a/drivers/gpu/drm/sysfb/simpledrm.c
+++ b/drivers/gpu/drm/sysfb/simpledrm.c
@@ -221,6 +221,33 @@ simplefb_get_panel_height_mm_of(struct drm_device *dev, struct device_node *of_p
 	return __simplefb_get_panel_size_mm_of(dev, of_panel_node, "height-mm");
 }
 
+static enum drm_panel_orientation
+simplefb_get_panel_orientation_of(struct drm_device *dev, struct device_node *of_panel_node)
+{
+	int ret;
+	u32 rotation;
+
+	ret = of_property_read_u32(of_panel_node, "rotation", &rotation);
+	if (ret) {
+		drm_dbg(dev, "simplefb: cannot parse panel rotation: error %d\n", ret);
+		return DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
+	}
+
+	switch (rotation) {
+	case 0:
+		return DRM_MODE_PANEL_ORIENTATION_NORMAL;
+	case 90:
+		return DRM_MODE_PANEL_ORIENTATION_RIGHT_UP;
+	case 180:
+		return DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP;
+	case 270:
+		return DRM_MODE_PANEL_ORIENTATION_LEFT_UP;
+	default:
+		drm_dbg(dev, "simplefb: cannot parse panel rotation: value %u\n", rotation);
+		return DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
+	}
+}
+
 /*
  * Simple Framebuffer device
  */
@@ -624,6 +651,7 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 	int width, height, stride;
 	u16 width_mm = 0, height_mm = 0;
 	struct device_node *panel_node;
+	enum drm_panel_orientation orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
 	const struct drm_format_info *format;
 	u64 size;
 	struct resource *res, *mem = NULL;
@@ -689,6 +717,7 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 		if (panel_node) {
 			width_mm = simplefb_get_panel_width_mm_of(dev, panel_node);
 			height_mm = simplefb_get_panel_height_mm_of(dev, panel_node);
+			orientation = simplefb_get_panel_orientation_of(dev, panel_node);
 			of_node_put(panel_node);
 		}
 	} else {
@@ -853,8 +882,7 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 	if (ret)
 		return ERR_PTR(ret);
 	drm_connector_helper_add(connector, &simpledrm_connector_helper_funcs);
-	drm_connector_set_panel_orientation_with_quirk(connector,
-						       DRM_MODE_PANEL_ORIENTATION_UNKNOWN,
+	drm_connector_set_panel_orientation_with_quirk(connector, orientation,
 						       width, height);
 
 	ret = drm_connector_attach_encoder(connector, encoder);
-- 
2.54.0

Re: [PATCH v2 5/6] drm/sysfb: simpledrm: Validate mmap size against framebuffer size

[sashiko-bot]

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

Re: [PATCH v2 6/6] drm/sysfb: simpledrm: Read panel orientation from DT node

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Blindly applying physical panel rotation to a logical framebuffer can cause double rotation if the firmware already applied hardware rotation.
- [Low] The patch duplicates the existing `of_drm_get_panel_orientation()` helper and introduces noisy debug logging for normal conditions.
--

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

drm/sysfb: simpledrm: Read panel orientation from DT node

A DeviceTree panel node can specify the panel's rotation in steps
of 90 degrees. Set the DRM connector orientation from the field.
Do nothing if the field is missing or invalid.

> diff --git a/drivers/gpu/drm/sysfb/simpledrm.c b/drivers/gpu/drm/sysfb/simpledrm.c
> index a04f0c852ea48..b585d17ca8177 100644
> --- a/drivers/gpu/drm/sysfb/simpledrm.c
> +++ b/drivers/gpu/drm/sysfb/simpledrm.c
> @@ -221,6 +221,33 @@ simplefb_get_panel_height_mm_of(struct drm_device *dev, struct device_node *of_p
>  	return __simplefb_get_panel_size_mm_of(dev, of_panel_node, "height-mm");
>  }
>  
> +static enum drm_panel_orientation
> +simplefb_get_panel_orientation_of(struct drm_device *dev, struct device_node *of_panel_node)
> +{
> +	int ret;
> +	u32 rotation;
> +
> +	ret = of_property_read_u32(of_panel_node, "rotation", &rotation);
> +	if (ret) {
> +		drm_dbg(dev, "simplefb: cannot parse panel rotation: error %d\n", ret);

[Severity: Low]
Is it intentional to log a debug message here when the property is missing?

Since most device tree panel nodes do not include a rotation property,
of_property_read_u32() will return -EINVAL and spam the DRM debug logs with
"cannot parse panel rotation: error -22" on almost every boot when debug
logging is enabled.

Also, does this function reimplement the parsing logic from
of_drm_get_panel_orientation() which explicitly treats missing properties
silently?

> +		return DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
> +	}
> +
> +	switch (rotation) {
> +	case 0:
> +		return DRM_MODE_PANEL_ORIENTATION_NORMAL;

[ ... ]

> @@ -689,6 +717,7 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>  		if (panel_node) {
>  			width_mm = simplefb_get_panel_width_mm_of(dev, panel_node);
>  			height_mm = simplefb_get_panel_height_mm_of(dev, panel_node);
> +			orientation = simplefb_get_panel_orientation_of(dev, panel_node);

[Severity: High]
Does unconditionally applying the physical panel rotation to the logical
framebuffer risk double rotation?

The simpledrm driver operates on a logical framebuffer pre-configured by the
firmware. If the firmware has already configured the display controller to
perform hardware rotation (e.g., exposing a 1920x1080 memory buffer for a
physically 1080x1920 panel mounted sideways), the simplefb node describes
an already-upright logical framebuffer.

If simpledrm blindly applies the physical rotation from the panel node to the
DRM connector, userspace compositors might apply a software rotation on top
of the firmware's hardware rotation, causing the final image to be rendered
sideways or distorted.

Is there a way to verify if the firmware already applied hardware rotation
before mapping physical panel properties here?

>  			of_node_put(panel_node);
>  		}
>  	} else {

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

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

[sashiko-bot]

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

Pre-existing issues:
- [High] The simpledrm driver fails to correctly validate that the framebuffer dimensions fit within the mapped memory resource size due to an inverted condition.
--

commit 1c3adcb617c4a3add398f3f24ada5131889b5d6a
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. The type only stores sizes
in 16-bit fields. Fail probing on errors.

Cc: <stable@vger.kernel.org> # v5.14+

> diff --git a/drivers/gpu/drm/sysfb/simpledrm.c b/drivers/gpu/drm/sysfb/simpledrm.c
> index fc168920f2c64..15dcafa9d524a 100644
> --- a/drivers/gpu/drm/sysfb/simpledrm.c
> +++ b/drivers/gpu/drm/sysfb/simpledrm.c

[ ... ]

> @@ -155,7 +148,7 @@ simplefb_get_height_of(struct drm_device *dev, struct device_node *of_node)
>  
>  	if (ret)
>  		return ret;
> -	return simplefb_get_validated_int0(dev, "height", height);
> +	return drm_sysfb_get_validated_int0(dev, "height", height, U16_MAX);
>  }
>  
>  static int

[Severity: High]
This isn't a bug introduced by this patch, but commit 'drm/sysfb:
simpledrm: Validate mmap size against framebuffer size' in this series
attempted to fix the missing validation and failed due to an inverted
condition.

Does simpledrm_device_create() fail to correctly validate that the
framebuffer dimensions fit within the mapped memory resource size?

Looking at simpledrm_device_create(), the check appears to be inverted in
both the system memory and I/O memory fallback paths:

drivers/gpu/drm/sysfb/simpledrm.c:simpledrm_device_create() {
        ...
		if (resource_size(mem) > size) {
			drm_err(dev,
				"framebuffer size of %llu exceeds memory range %pr\n",
				size, mem);
			return ERR_PTR(-EINVAL);
		}
        ...
}

Because this check returns -EINVAL when resource_size(mem) > size, it
incorrectly rejects valid configurations where the physical memory region
is larger than the required framebuffer.

More importantly, it allows invalid configurations where size >
resource_size(mem). In this case, devm_memremap() maps the smaller
resource_size(mem), but drm_fb_memcpy() will access size bytes during
plane updates, leading to an out-of-bounds kernel memory access past the
mapped area.

Should this check be size > resource_size(mem) instead?

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

Re: [PATCH v2 5/6] drm/sysfb: simpledrm: Validate mmap size against framebuffer size

[Thomas Zimmermann]

Am 22.06.26 um 15:33 schrieb 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.

This protects against overflowing in the ALIGN() call. While we cannot 
trigger this problem now, it is still the safer thing to do this.

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

Neither height nor stride can be 0 IIRC. We should not see size being 0.

>
>> +		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.

Arghh, when I tested with bochs, I hit exactly the case of mem == size, 
so it worked. :/

>
> 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));

-- 
--
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)

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 652 bytes --]

On Mon, Jun 22, 2026 at 03:19:35PM +0200, Thomas Zimmermann wrote:
> Validate the framebuffer size from the firmware against the
> limitations of struct drm_display_mode. The type only stores sizes
> in 16-bit fields. Fail probing on errors.
> 
> v2:
> - remove unused function simplefb_get_validated_int0() (Sashiko)
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: 11e8f5fd223b ("drm: Add simpledrm driver")
> Cc: <stable@vger.kernel.org> # v5.14+
> ---
>  drivers/gpu/drm/sysfb/simpledrm.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/6] drm/sysfb: simpledrm: Improve panel-size validation

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3698 bytes --]

On Mon, Jun 22, 2026 at 03:19:36PM +0200, Thomas Zimmermann wrote:
> Validate the panel size from the device-tree node against the
> limitations of struct drm_display_mode. The type only stores sizes
> in 16-bit fields. Fail transparently on errors; do not warn.
> 
> v2:
> - only use initialized values in debugging output (Sashiko)
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: 2a6d731a8f16 ("drm/simpledrm: Allow physical width and height configuration via panel node")
> Cc: Rayyan Ansari <rayyan@ansari.sh>
> Cc: <stable@vger.kernel.org> # v6.4+
> ---
>  drivers/gpu/drm/sysfb/simpledrm.c | 40 ++++++++++++++++++++++++++++---
>  1 file changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sysfb/simpledrm.c b/drivers/gpu/drm/sysfb/simpledrm.c
> index 15dcafa9d524..fa2121f81def 100644
> --- a/drivers/gpu/drm/sysfb/simpledrm.c
> +++ b/drivers/gpu/drm/sysfb/simpledrm.c
> @@ -193,6 +193,40 @@ simplefb_get_memory_of(struct drm_device *dev, struct device_node *of_node)
>  	return res;
>  }
>  
> +static u16
> +__simplefb_get_panel_size_mm_of(struct drm_device *dev, struct device_node *of_panel_node,
> +				const char *name)
> +{
> +	int ret;
> +	u32 value;
> +
> +	ret = of_property_read_u32(of_panel_node, name, &value);
> +	if (ret) {
> +		drm_dbg(dev, "simplefb: cannot parse panel %s: error %d\n",
> +			name, ret);
> +		return 0; /* not an error, simply ignore */
> +	}
> +	if (value > U16_MAX) {
> +		drm_dbg(dev, "simplefb: panel %s of %u exceeds maximum value\n",
> +			name, value);
> +		return 0; /* not an error, simply ignore */

I wonder if it's perhaps better to move this comment to the function
scope and explain why this can be ignored. I didn't know and had to go
look at drm_sysfb_mode() to see that if these are 0, it'll compute the
physical dimensions based on a default of 96 DPI.

> +	}
> +
> +	return value;
> +}
> +
> +static u16
> +simplefb_get_panel_width_mm_of(struct drm_device *dev, struct device_node *of_panel_node)
> +{
> +	return __simplefb_get_panel_size_mm_of(dev, of_panel_node, "width-mm");
> +}
> +
> +static u16
> +simplefb_get_panel_height_mm_of(struct drm_device *dev, struct device_node *of_panel_node)
> +{
> +	return __simplefb_get_panel_size_mm_of(dev, of_panel_node, "height-mm");
> +}
> +
>  /*
>   * Simple Framebuffer device
>   */
> @@ -594,7 +628,7 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>  	struct drm_sysfb_device *sysfb;
>  	struct drm_device *dev;
>  	int width, height, stride;
> -	int width_mm = 0, height_mm = 0;
> +	u16 width_mm = 0, height_mm = 0;
>  	struct device_node *panel_node;
>  	const struct drm_format_info *format;
>  	struct resource *res, *mem = NULL;
> @@ -658,8 +692,8 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>  			return ERR_CAST(mem);
>  		panel_node = of_parse_phandle(of_node, "panel", 0);
>  		if (panel_node) {
> -			simplefb_read_u32_of(dev, panel_node, "width-mm", &width_mm);
> -			simplefb_read_u32_of(dev, panel_node, "height-mm", &height_mm);
> +			width_mm = simplefb_get_panel_width_mm_of(dev, panel_node);
> +			height_mm = simplefb_get_panel_height_mm_of(dev, panel_node);
>  			of_node_put(panel_node);
>  		}
>  	} else {

The drm_sysfb_mode() function that width_mm and height_mm get passed
into accepts them as unsigned int, so maybe that should be changed as
well for more consistency?

In either case, since they all end up in the u16 in the struct, it's
obviously correct to check for the range when parsing, so:

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/6] drm/sysfb: simpledrm: Inline simplefb_get_validated_int()

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 436 bytes --]

On Mon, Jun 22, 2026 at 03:19:37PM +0200, Thomas Zimmermann wrote:
> The helper simplefb_get_validated_int() is only a wrapper around
> drm_sysfb_get_validated_int(). Inline the function into its callers.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/sysfb/simpledrm.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 4/6] drm/sysfb: simpledrm: Improve stride validation

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 608 bytes --]

On Mon, Jun 22, 2026 at 03:19:38PM +0200, Thomas Zimmermann wrote:
> Validate the computed stride against the maximum value INT_MAX.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: 7bfa5c7b28d6 ("drm/simpledrm: Compute linestride with drm_format_info_min_pitch()")
> Cc: <stable@vger.kernel.org> # v6.1+
> ---
>  drivers/gpu/drm/sysfb/simpledrm.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)

Ugh... I have a hard time imagining that we'd ever exceed INT_MAX here,
but I guess better safe than sorry?

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 5/6] drm/sysfb: simpledrm: Validate mmap size against framebuffer size

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2005 bytes --]

On Mon, Jun 22, 2026 at 03:19:39PM +0200, Thomas Zimmermann wrote:
> 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.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Suggested-by: Sashiko <sashiko-bot@kernel.org>
> ---
>  drivers/gpu/drm/sysfb/simpledrm.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/gpu/drm/sysfb/simpledrm.c b/drivers/gpu/drm/sysfb/simpledrm.c
> index 76b9a3f5c4ef..a04f0c852ea4 100644
> --- a/drivers/gpu/drm/sysfb/simpledrm.c
> +++ b/drivers/gpu/drm/sysfb/simpledrm.c
> @@ -6,6 +6,7 @@
>  #include <linux/of_address.h>
>  #include <linux/of_clk.h>
>  #include <linux/of_reserved_mem.h>
> +#include <linux/overflow.h>
>  #include <linux/platform_data/simplefb.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm.h>
> @@ -624,6 +625,7 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
>  	u16 width_mm = 0, height_mm = 0;
>  	struct device_node *panel_node;
>  	const struct drm_format_info *format;
> +	u64 size;
>  	struct resource *res, *mem = NULL;
>  	struct drm_plane *primary_plane;
>  	struct drm_crtc *crtc;
> @@ -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");

That error message doesn't make sense to me. Maybe "framebuffer
alignment below minimum", or something along those lines?

With that fixed:

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 6/6] drm/sysfb: simpledrm: Read panel orientation from DT node

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2011 bytes --]

On Mon, Jun 22, 2026 at 03:19:40PM +0200, Thomas Zimmermann wrote:
> A DeviceTree panel node can specify the panel's rotation in steps
> of 90 degrees. Set the DRM connector orientation from the field.
> Do nothing if the field is missing or invalid.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/sysfb/simpledrm.c | 32 +++++++++++++++++++++++++++++--
>  1 file changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sysfb/simpledrm.c b/drivers/gpu/drm/sysfb/simpledrm.c
> index a04f0c852ea4..b585d17ca817 100644
> --- a/drivers/gpu/drm/sysfb/simpledrm.c
> +++ b/drivers/gpu/drm/sysfb/simpledrm.c
> @@ -221,6 +221,33 @@ simplefb_get_panel_height_mm_of(struct drm_device *dev, struct device_node *of_p
>  	return __simplefb_get_panel_size_mm_of(dev, of_panel_node, "height-mm");
>  }
>  
> +static enum drm_panel_orientation
> +simplefb_get_panel_orientation_of(struct drm_device *dev, struct device_node *of_panel_node)
> +{
> +	int ret;
> +	u32 rotation;
> +
> +	ret = of_property_read_u32(of_panel_node, "rotation", &rotation);
> +	if (ret) {
> +		drm_dbg(dev, "simplefb: cannot parse panel rotation: error %d\n", ret);

Why is this debug level? Maybe we don't want an error if the property is
absent, but if it really can't be parsed, why not print out a proper
error?

> +		return DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
> +	}
> +
> +	switch (rotation) {
> +	case 0:
> +		return DRM_MODE_PANEL_ORIENTATION_NORMAL;
> +	case 90:
> +		return DRM_MODE_PANEL_ORIENTATION_RIGHT_UP;
> +	case 180:
> +		return DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP;
> +	case 270:
> +		return DRM_MODE_PANEL_ORIENTATION_LEFT_UP;
> +	default:
> +		drm_dbg(dev, "simplefb: cannot parse panel rotation: value %u\n", rotation);

Same here. Seems like we should be verbose when this contains an invalid
value. DT checkers should catch these already, so if the invalid value
makes it here, maybe we should sound the alarm.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]