Re: [PATCH 2/3] drm/sysfb: Use preferred panel size for panel orientation quirks

[sashiko-bot@kernel.org]

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 <tzimmermann@suse.de>

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/20260827140726.420734-1-tzimmermann@suse.de?part=2