Am 27.08.26 um 16:21 schrieb [email protected]:
Thank you for your contribution! Sashiko AI review found 2 potential issue(s)
to consider:
New issues:
- [Medium] The sysfb drivers parse unvalidated EDID data to extract the
preferred size.
If we read bogus values, the panel orientation could be wrong. Nothing
worse happens. Apart from that, most drivers call
drm_edid_header_is_valid(). I'll add this call to ofdrm, which is
currently missing it.
Pre-existing issues:
- [High] drm_edid_get_preferred_size() incorrectly checks the value pointed to
by width/height instead of validating the pointers themselves.
That's just fallout from the typo in patch 1.
--
commit df008c5490e15b30036ebe81983d7a6c0c6e5b33
Author: Thomas Zimmermann <[email protected]>
drm/sysfb: Use preferred panel size for panel orientation quirks
Sysfb drivers currently use the given display mode for looking up the
panel orientation. But the look-up table stores the native geometry of
the panels, so the lookup fails if the current mode sizes differs.
Get the panel's native geometry with drm_edid_get_preferred_size() from
the EDID and use it for looking up the panel orientation.
diff --git a/drivers/gpu/drm/sysfb/efidrm.c b/drivers/gpu/drm/sysfb/efidrm.c
index 3f9cd5d03efb2..7a7d12003304f 100644
--- a/drivers/gpu/drm/sysfb/efidrm.c
+++ b/drivers/gpu/drm/sysfb/efidrm.c
[ ... ]
@@ -217,6 +218,20 @@ static struct efidrm_device *efidrm_device_create(struct
drm_driver *drv,
if (drm_edid_header_is_valid(dpy->edid.dummy) == 8)
sysfb->edid = dpy->edid.dummy;
#endif
+
+ panel_width = width;
+ panel_height = height;
+
+ if (sysfb->edid) {
+ const struct drm_edid *drm_edid;
+
+ drm_edid = drm_edid_alloc(sysfb->edid, EDID_LENGTH);
+ if (drm_edid) {
+ drm_edid_get_preferred_size(drm_edid, &panel_width,
&panel_height);
[Severity: High]
This isn't a bug introduced by this patch, but I wanted to ask about the
semantics of drm_edid_get_preferred_size(). In drivers/gpu/drm/drm_edid.c,
it does:
if (*width)
*width = (pt->hactive_hblank_hi & 0xf0) << 4 | pt->hactive_lo;
if (*height)
*height = (pt->vactive_vblank_hi & 0xf0) << 4 | pt->vactive_lo;
This incorrectly checks the values pointed to by width/height instead of
validating the pointers themselves. A caller passing a NULL pointer for
either width or height will cause a kernel panic (NULL pointer dereference).
Furthermore, passing a pointer to a zero-initialized variable evaluates to
false, causing the function to silently skip populating the preferred size.
While this patch bypasses the crash by passing non-NULL pointers initialized
to >0, is it worth fixing the generic API so other callers do not trip over
this?
[Severity: Medium]
Does parsing an unvalidated EDID block pose a risk here? The sysfb drivers
parse unvalidated EDID data to extract the preferred size.
If the firmware provides a corrupted EDID where only the 8-byte header is
valid, this code extracts geometry from potentially corrupted firmware data,
which can result in bogus resolutions being passed to the panel orientation
quirk lookup.
+ drm_edid_free(drm_edid);
+ }
+ }
+
[ ... ]
diff --git a/drivers/gpu/drm/sysfb/ofdrm.c b/drivers/gpu/drm/sysfb/ofdrm.c
index 819aed466727e..6cf2440d7d9e2 100644
--- a/drivers/gpu/drm/sysfb/ofdrm.c
+++ b/drivers/gpu/drm/sysfb/ofdrm.c
[ ... ]
@@ -994,6 +995,19 @@ static struct ofdrm_device *ofdrm_device_create(struct
drm_driver *drv,
sysfb->fb_gamma_lut_size = OFDRM_GAMMA_LUT_SIZE;
sysfb->edid = edid;
+ panel_width = width;
+ panel_height = height;
+
+ if (sysfb->edid) {
+ const struct drm_edid *drm_edid;
+
+ drm_edid = drm_edid_alloc(sysfb->edid, EDID_LENGTH);
+ if (drm_edid) {
+ drm_edid_get_preferred_size(drm_edid, &panel_width,
&panel_height);
[Severity: Medium]
Similarly here in ofdrm_device_create(), unvalidated EDID data is parsed.
Could this lead to bogus geometry being extracted from a corrupted EDID?
+ drm_edid_free(drm_edid);
+ }
+ }
+
[ ... ]
diff --git a/drivers/gpu/drm/sysfb/vesadrm.c b/drivers/gpu/drm/sysfb/vesadrm.c
index 6a67b2d2e451b..8c52bbac6d2ff 100644
--- a/drivers/gpu/drm/sysfb/vesadrm.c
+++ b/drivers/gpu/drm/sysfb/vesadrm.c
[ ... ]
@@ -484,6 +485,20 @@ static struct vesadrm_device *vesadrm_device_create(struct
drm_driver *drv,
if (drm_edid_header_is_valid(dpy->edid.dummy) == 8)
sysfb->edid = dpy->edid.dummy;
#endif
+
+ panel_width = width;
+ panel_height = height;
+
+ if (sysfb->edid) {
+ const struct drm_edid *drm_edid;
+
+ drm_edid = drm_edid_alloc(sysfb->edid, EDID_LENGTH);
+ if (drm_edid) {
+ drm_edid_get_preferred_size(drm_edid, &panel_width,
&panel_height);
[Severity: Medium]
Likewise in vesadrm_device_create(), parsing unvalidated EDID data may
result in bogus resolutions being passed to the panel orientation quirk
lookup. Should the EDID be fully validated first?
+ drm_edid_free(drm_edid);
+ }
+ }
+
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)
