Re: [PATCH 61/75] drm/sun4i: tv: Convert to atomic_create_state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm: NULL pointer dereference when initializing TV connector state
--

commit a85a6a0ad13c063d072cdcda87267ea8867cafa3
Author: Maxime Ripard <mripard@kernel.org>

drm/sun4i: tv: Convert to atomic_create_state

The sun4i TV connector reset implementation is converted to an
atomic_create_state hook. It creates the pristine state and initializes
the analog TV properties.

> diff --git a/drivers/gpu/drm/sun4i/sun4i_tv.c b/drivers/gpu/drm/sun4i/sun4i_tv.c
> index ea86f8a6a35e7..a68e5f7a03d2c 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tv.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tv.c
[ ... ]
> -static void sun4i_tv_connector_reset(struct drm_connector *connector)
> +static struct drm_connector_state *
> +sun4i_tv_connector_create_state(struct drm_connector *connector)
>  {
> -	drm_atomic_helper_connector_reset(connector);
> -	drm_atomic_helper_connector_tv_state_init(connector->state, connector);
> +	struct drm_connector_state *connector_state;
> +
> +	connector_state = drm_atomic_helper_connector_create_state(connector);
> +	if (IS_ERR(connector_state))
> +		return connector_state;
> +
> +	drm_atomic_helper_connector_tv_state_init(connector_state, connector);

[Severity: High]
Does this trigger a NULL pointer dereference during initialization?

When the DRM core resets the connector state, it explicitly clears the old
state before invoking the .atomic_create_state hook:

drm_mode_config_connector_reset_with_create_state() {
        ...
        if (connector->state) {
                ...
                connector->state = NULL;
        }
        return drm_mode_config_connector_create_state(connector);
}

The core helper drm_atomic_helper_connector_tv_state_init() then mistakenly
passes the unassigned connector->state down to the margins initialization
instead of the newly allocated state:

drm_atomic_helper_connector_tv_state_init() {
        ...
        drm_atomic_helper_connector_tv_margins_state_init(connector->state,
                                                          connector);
}

This causes drm_atomic_helper_connector_tv_margins_state_init() to
unconditionally dereference the NULL state pointer:

drm_atomic_helper_connector_tv_margins_state_init(state, ...) {
        ...
        state->tv.margins.left = cmdline->tv_margins.left;
        ...
}

The previous .reset hook implementation masked this core helper bug because
it assigned connector->state prior to calling the TV state initialization
helper.

Could we fix the core helper to pass the correct state pointer, or should
this driver handle the initialization differently to avoid the crash?

> +
> +	return connector_state;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-drm-no-more-connector-reset-v1-0-586c9c026823@kernel.org?part=61