[PATCH 0/3] drm/sysfb: simpledrm: Various improvements

[PATCH 0/3] drm/sysfb: simpledrm: Various improvements

[Thomas Zimmermann]

Patches 1 and 2 improve validation of the values provided by the
device firmware.

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

Thomas Zimmermann (3):
  drm/sysfb: simpledrm: Improve framebuffer-size validation
  drm/sysfb: simpledrm: Improve panel-size validation
  drm/sysfb: simpledrm: Read panel orientation from DT node

 drivers/gpu/drm/sysfb/simpledrm.c | 75 +++++++++++++++++++++++++++----
 1 file changed, 66 insertions(+), 9 deletions(-)

-- 
2.54.0

[PATCH 1/3] 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.

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 | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/sysfb/simpledrm.c b/drivers/gpu/drm/sysfb/simpledrm.c
index fc168920f2c6..03bd19fadccd 100644
--- a/drivers/gpu/drm/sysfb/simpledrm.c
+++ b/drivers/gpu/drm/sysfb/simpledrm.c
@@ -88,14 +88,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 +144,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 +155,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 2/3] 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.

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 | 35 ++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/sysfb/simpledrm.c b/drivers/gpu/drm/sysfb/simpledrm.c
index 03bd19fadccd..5c1db0785d92 100644
--- a/drivers/gpu/drm/sysfb/simpledrm.c
+++ b/drivers/gpu/drm/sysfb/simpledrm.c
@@ -200,6 +200,35 @@ 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 || value > U16_MAX) {
+		drm_dbg(dev, "simplefb: cannot parse panel %s: value %u, error %d\n",
+			name, value, ret);
+		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
  */
@@ -601,7 +630,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;
@@ -665,8 +694,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 3/3] 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 5c1db0785d92..a84d4284b814 100644
--- a/drivers/gpu/drm/sysfb/simpledrm.c
+++ b/drivers/gpu/drm/sysfb/simpledrm.c
@@ -229,6 +229,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
  */
@@ -632,6 +659,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;
 	struct resource *res, *mem = NULL;
 	struct drm_plane *primary_plane;
@@ -696,6 +724,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 {
@@ -831,8 +860,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 2/3] drm/sysfb: simpledrm: Improve panel-size validation

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Uninitialized stack variable `value` is printed in `drm_dbg()` when `of_property_read_u32()` fails.
--

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

drm/sysfb: simpledrm: Improve panel-size validation

This commit validates the panel size from the device-tree node against
the limitations of struct drm_display_mode and fails transparently on
errors without warning.

Cc: stable@vger.kernel.org # v6.4+

> diff --git a/drivers/gpu/drm/sysfb/simpledrm.c b/drivers/gpu/drm/sysfb/simpledrm.c
> index 03bd19fadccd5..5c1db0785d926 100644
> --- a/drivers/gpu/drm/sysfb/simpledrm.c
> +++ b/drivers/gpu/drm/sysfb/simpledrm.c
> @@ -200,6 +200,35 @@ 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 || value > U16_MAX) {
> +		drm_dbg(dev, "simplefb: cannot parse panel %s: value %u, error %d\n",
> +			name, value, ret);

[Severity: Low]
Does this code print an uninitialized stack variable when
of_property_read_u32() fails?

If the property is missing from the device tree or another error occurs,
of_property_read_u32() will return a non-zero error code and leave the
value parameter untouched. When this happens, the uninitialized stack
memory will be unconditionally passed to drm_dbg() for printing.

> +		return 0; /* not an error, simply ignore */
> +	}
> +
> +	return value;
> +}

[ ... ]

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

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

[sashiko-bot]

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

New issues:
- [Low] The patch updates all callers of the local static wrapper `simplefb_get_validated_int0()` to call `drm_sysfb_get_validated_int0()` directly, but leaves the static function definition behind, causing a compiler warning.

Pre-existing issues:
- [High] Missing validation between the firmware-provided framebuffer dimensions (stride, width, height) and the size of the memory region allows out-of-bounds virtual memory writes.
--

commit 934b7fe8edd3f05582affbe138506d555774f080
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. Fail probing on errors if the size exceeds the
16-bit fields.

> diff --git a/drivers/gpu/drm/sysfb/simpledrm.c b/drivers/gpu/drm/sysfb/simpledrm.c
> index fc168920f2c64..03bd19fadccd5 100644
> --- a/drivers/gpu/drm/sysfb/simpledrm.c
> +++ b/drivers/gpu/drm/sysfb/simpledrm.c
> @@ -88,14 +88,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);
>  }

[Severity: Low]
Does this leave simplefb_get_validated_int0() as an unused static function?

Since all four callers (simplefb_get_width_pd, simplefb_get_height_pd,
simplefb_get_width_of, simplefb_get_height_of) are updated to call
drm_sysfb_get_validated_int0() directly, simplefb_get_validated_int0()
appears to have zero callers remaining in simpledrm.c. This can trigger a
-Wunused-function compiler warning.

[Severity: High]
This isn't a bug introduced by this patch, but while we are improving the
validation of framebuffer dimensions from firmware, should we also validate
that the resulting framebuffer size actually fits within the mapped memory
region?

The firmware-provided stride and height are used to calculate the framebuffer
size, but when acquiring the memory region:

drivers/gpu/drm/sysfb/simpledrm.c:simpledrm_device_create() {
    ...
	screen_base = devm_memremap(dev->dev, mem->start, resource_size(mem), MEMREMAP_WC);
    ...
}

or the fallback:

drivers/gpu/drm/sysfb/simpledrm.c:simpledrm_device_create() {
    ...
	screen_base = devm_ioremap_wc(&pdev->dev, mem->start, resource_size(mem));
    ...
}

It seems we map exactly resource_size(mem) bytes without verifying if
(stride * height) exceeds that size. If the firmware provides a mismatched
configuration, out-of-bounds writes could occur during plane updates when
copying pixel data into sysfb->fb_addr.

[ ... ]

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