Re: [PATCH 1/3] drm/edid: Add drm_edid_get_preferred_size()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Pointer Dereference and Logic Error in drm_edid_get_preferred_size
- [High] Missing EDID Hardware Validation for Detailed Timing Descriptor
--

commit 2d529ec582fce7d82757afd45c986da44f50ceb6
Author: Thomas Zimmermann <tzimmermann@suse.de>
Subject: drm/edid: Add drm_edid_get_preferred_size()

This commit adds drm_edid_get_preferred_size() to extract the panel's
preferred display resolution from a given EDID. This is required for
DRM's panel orientation quirks.

> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index df3c25bac761d..650ccd05977be 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c

[ ... ]

> @@ -2844,6 +2844,46 @@ u32 drm_edid_get_panel_id(const struct drm_edid *drm_edid)
>  }
>  EXPORT_SYMBOL(drm_edid_get_panel_id);
>  
> +/**
> + * drm_edid_get_preferred_size - Get a panel's preferred size from EDID
> + * @drm_edid: EDID that contains panel ID.
> + * @width: Returns the panel's width in pixels per scanline, if given
> + * @height: Returns the panel's height in scanlines, if given
> + *
> + * This function detects the preferred size of a panel from the given
> + * EDID. There is no such information stored in the EDID block directly,
> + * but the preferred mode often corresponds to the panel's native geometry.
> + *
> + * Return: Zero on success, or a negative errno code otherwise.
> + */
> +int drm_edid_get_preferred_size(const struct drm_edid *drm_edid,
> +				unsigned int *width, unsigned int *height)
> +{
> +	const struct edid *edid = drm_edid->edid;
> +	const struct detailed_pixel_timing *pt;
> +
> +	if (drm_edid->size < EDID_LENGTH)
> +		return -EINVAL;
> +
> +	/*
> +	 * Use whatever the Preferred Timing Descriptor tells us. For old
> +	 * and obscure displays, we might need better heuristics.
> +	 */
> +
> +	if (edid->revision < 4 && !(edid->features & DRM_EDID_FEATURE_PREFERRED_TIMING))
> +		return -EINVAL; /* no Preferred Timing Descriptor */
> +
> +	pt = &edid->detailed_timings[0].data.pixel_data;

[Severity: High]
Does this safely handle non-compliant or corrupted EDIDs where the first
detailed timing block might be a Display Descriptor rather than a pixel
timing descriptor?

In the EDID specification, a detailed timing block is only a Detailed Timing
Descriptor if its pixel clock is non-zero. If the pixel clock is 0, the block
is a Display Descriptor like an ASCII monitor name. By unconditionally
accessing it as pixel data without validating it, could we end up reading
overlapping union fields and returning garbage display geometry data to the
driver?

> +
> +	if (*width)
> +		*width = (pt->hactive_hblank_hi & 0xf0) << 4 | pt->hactive_lo;
> +	if (*height)
> +		*height = (pt->vactive_vblank_hi & 0xf0) << 4 | pt->vactive_lo;

[Severity: High]
Does this logic incorrectly dereference the pointers before checking if they
are valid?

The kernel doc mentions that width and height are optional, which implies
callers might pass a NULL pointer. If a caller passes NULL for width or
height, evaluating if (*width) will cause a NULL pointer dereference crash.

Additionally, if a caller passes a valid pointer to a 0-initialized variable,
evaluating if (*width) will result in false, skipping the assignment entirely
and leaving the output as 0. Should these conditions check if (width) and
if (height) instead?

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_edid_get_preferred_size);

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