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. Pre-existing issues: - [High] drm_edid_get_preferred_size() incorrectly checks the value pointed to by width/height instead of validating the pointers themselves. -- 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); > + } > + } > + -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
