Re: [PATCH v9 03/61] drm/connector: Add drmm_connector_hdmi_init() with new signature

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

New issues:
- [High] Missing validation for `hdmi_funcs->supported_hdmi_ver` leading to a 0 Hz TMDS rate limit or spurious -EINVAL on initialization.

Pre-existing issues:
- [High] Missing NULL check on `connector->state` before calling `drm_connector_attach_max_bpc_property()`, leading to a potential NULL pointer dereference.
--

--- Patch [3]: [PATCH v9 03/61] drm/connector: Add drmm_connector_hdmi_init() with new signature ---
commit 68ee3a0c1e1945b6e973060559a1cafa855a35d7
Author: Cristian Ciocaltea <[email protected]>
Subject: drm/connector: Add drmm_connector_hdmi_init() with new signature

This patch introduces a new function drmm_connector_hdmi_init() with a
reduced signature by moving HDMI-specific parameters into struct
drm_connector_hdmi_funcs. It also infers the maximum TMDS character rate
from the supported HDMI version or allows an override if specified.

> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c

[ ... ]

> +	/*
> +	 * The supported HDMI version can be used to determinate the maximum
> +	 * TMDS character rate allowed by the specification. Some controllers,
> +	 * however, may support a lower rate than that version would imply.
> +	 *
> +	 * A non-zero caps->max_tmds_char_rate lets drivers override this
> +	 * default with the actual controller capability. A value of zero keeps
> +	 * the limit inferred from supported_hdmi_ver.
> +	 */
> +	if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_2_0)
> +		connector->hdmi.max_tmds_char_rate = HDMI_2_0_TMDS_CHAR_RATE_MAX_HZ;
> +	else if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_1_3)
> +		connector->hdmi.max_tmds_char_rate = HDMI_1_3_TMDS_CHAR_RATE_MAX_HZ;
> +	else if (hdmi_funcs->supported_hdmi_ver >= HDMI_VERSION_1_0)
> +		connector->hdmi.max_tmds_char_rate = HDMI_1_0_TMDS_CHAR_RATE_MAX_HZ;

[Severity: High]
What happens if hdmi_funcs->supported_hdmi_ver is 0 (HDMI_VERSION_UNKNOWN) in
drmm_connector_hdmi_init()?

If a driver developer forgets to initialize hdmi_funcs->supported_hdmi_ver,
it defaults to 0. This if/else block falls through without setting
max_tmds_char_rate, leaving it at 0.

If a driver provides a custom supported_tmds_char_rate, initialization will
then fail with -EINVAL. Otherwise, the connector initializes with a 0 Hz
TMDS limit, which could break display mode validation by rejecting all
display modes.

> +
> +	if (hdmi_funcs->supported_tmds_char_rate) {
> +		if (hdmi_funcs->supported_tmds_char_rate > connector->hdmi.max_tmds_char_rate) {
> +			drm_err(dev, "Enforced max_tmds_char_rate exceeds %llu spec limit\n",
> +				connector->hdmi.max_tmds_char_rate);
> +			return -EINVAL;
> +		}
> +		connector->hdmi.max_tmds_char_rate = hdmi_funcs->supported_tmds_char_rate;
> +	}

[ ... ]

> +	if (connector->funcs->atomic_create_state) {
> +		struct drm_connector_state *state;
> +
> +		state = connector->funcs->atomic_create_state(connector);
> +		if (IS_ERR(state))
> +			return PTR_ERR(state);
> +
> +		connector->state = state;
> +	} else if (connector->funcs->reset) {
> +		connector->funcs->reset(connector);
> +	}
> +
> +	drm_connector_attach_max_bpc_property(connector, 8, hdmi_funcs->max_bpc);

[Severity: High]
This isn't a bug introduced by this patch, but could this lead to a NULL
pointer dereference in drm_connector_attach_max_bpc_property() if the reset
callback fails?

In drmm_connector_hdmi_init(), if the reset callback fails to allocate memory
(e.g., inside drm_atomic_helper_connector_reset()), connector->state will
remain NULL.

When this is unconditionally passed to
drm_connector_attach_max_bpc_property(), would it dereference the NULL state:

drivers/gpu/drm/drm_connector.c:drm_connector_attach_max_bpc_property() {
	...
	connector->state->max_requested_bpc = max;
	...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3