Re: [PATCH v6 08/28] drm/atomic_sro: Add atomic state readout infrastructure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] drm: NULL pointer dereference in SRO readout and compare hooks
- [High] drm: Buffer overflow in atomic SRO state due to dynamic connector counts
--

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

drm/atomic_sro: Add atomic state readout infrastructure

This patch introduces the atomic state readout (SRO) infrastructure by
adding readout hooks to KMS object vtables and providing top-level entry
points for hardware state readout at initialization.

> diff --git a/drivers/gpu/drm/drm_atomic_sro.c b/drivers/gpu/drm/drm_atomic_sro.c
> index 1551f6de6c89a..6921af73eb697 100644
> --- a/drivers/gpu/drm/drm_atomic_sro.c
> +++ b/drivers/gpu/drm/drm_atomic_sro.c
[ ... ]
> @@ -25,6 +26,59 @@ module_param_unsafe(atomic_readout, uint, 0400);
>  MODULE_PARM_DESC(atomic_readout,
>  		 "Enable Hardware State Readout (0 = disabled, 1 = enabled, 2 = ignore missing compares, 3 = ignore missing readouts and compares, default = 1)");
>  
> +static bool drm_atomic_sro_can_readout(struct drm_device *dev)
> +{
> +	struct drm_crtc *crtc;
> +	struct drm_plane *plane;
> +	struct drm_connector *connector;
> +	struct drm_private_obj *privobj;
> +	struct drm_connector_list_iter conn_iter;
> +
> +	if (atomic_readout == DRM_ATOMIC_READOUT_SKIP_MISSING_READOUT)
> +		return true;

[Severity: Critical]
If atomic_readout is set to DRM_ATOMIC_READOUT_SKIP_MISSING_READOUT (3), this
bypasses driver capability checks and returns true. Can this cause a NULL
pointer dereference?

Looking at drm_atomic_sro_readout_and_compare() and
drm_atomic_helper_sro_readout_state(), they unconditionally dereference
dev->mode_config.helper_private (as funcs) and call
funcs->atomic_sro_build_state(dev):

    const struct drm_mode_config_helper_funcs *funcs =
        dev->mode_config.helper_private;
    ...
    readout_state = funcs->atomic_sro_build_state(dev);

Since most existing atomic drivers do not implement SRO and leave
atomic_sro_build_state NULL (or lack helper_private entirely), wouldn't
bypassing this capability check cause an immediate NULL pointer dereference on
any modeset or blocking commit for those drivers?

Also, doesn't drm_atomic_helper_commit_hw_done() unconditionally call
drm_atomic_sro_readout_and_compare() for all blocking commits across all
atomic drivers, triggering this crash even without the module parameter?

    if (!nonblock)
        drm_atomic_sro_readout_and_compare(state);

Looking at the state allocation in drm_atomic_sro_state_init():

    state->connectors = kzalloc_objs(*state->connectors,
                                     dev->mode_config.num_connector);

[Severity: High]
Can this array overflow when DP MST connectors are dynamically added?

Since MST connectors can be added or removed dynamically, a connector's
IDA-allocated index (conn->index) can exceed the originally allocated
dev->mode_config.num_connector count.

Later in drm_atomic_sro_set_connector_state(), the code writes to this array
without a bounds check or dynamic reallocation:

    unsigned int index = drm_connector_index(conn);
    drm_connector_get(conn);
    state->connectors[index].ptr = conn;
    state->connectors[index].state = conn_state;

Does this lead to an out-of-bounds write?

Additionally, drm_atomic_sro_state_free() iterates using the live
dev->mode_config.num_connector value instead of the allocated capacity:

    for (i = 0; i < state->dev->mode_config.num_connector; i++) {
        struct drm_connector *conn = state->connectors[i].ptr;

If the number of connectors increased or decreased since allocation, could this
lead to an out-of-bounds read or a memory leak respectively?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-drm-state-readout-v6-0-6cc02cd10998@kernel.org?part=8