Re: [PATCH v6 13/28] drm/atomic_sro: Provide helpers to implement hardware state readout

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] drm: NULL pointer dereference in drm_atomic_helper_sro_readout_state() due to missing fallback
- [High] drm: NULL pointer dereference for legacy bridges in drm_atomic_helper_sro_readout_bridges_state()
- [High] drm/atomic_sro: NULL pointer dereference in disabled bridge readout
- [High] drm/atomic_sro: out-of-bounds write in connector state array
- [Medium] drm: O(N^2) loop and OOB read risk in drm_atomic_sro_install_state()
- [Low] drm/atomic: error code shadowed in drm_atomic_helper_sro_build_state()
--

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

drm/atomic_sro: Provide helpers to implement hardware state readout

Drivers implementing SRO need to iterate over their KMS objects,
allocate a pristine state for each, call the readout hook, and store
the result in the SRO state container. Provide helpers for each object type
and a top-level function that calls them in sequence.

> diff --git a/drivers/gpu/drm/drm_atomic_sro_helper.c b/drivers/gpu/drm/drm_atomic_sro_helper.c
> new file mode 100644
> index 0000000000000..50d1af53bfc24
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_atomic_sro_helper.c
[ ... ]
> +int
> +drm_atomic_helper_sro_readout_bridges_state(struct drm_device *dev,
> +					    struct drm_atomic_sro_state *state)
> +{
> +	struct drm_encoder *encoder;
> +
> +	might_alloc(GFP_KERNEL);
[ ... ]
> +	drm_for_each_encoder(encoder, dev) {
> +		struct drm_bridge *bridge;
> +		int ret;
> +
> +		list_for_each_entry(bridge, &encoder->bridge_chain, chain_node) {
> +			struct drm_private_obj *bridge_obj = &bridge->base;
> +			const struct drm_private_state_funcs *bridge_obj_funcs =
> +				bridge_obj->funcs;
> +			struct drm_private_state *bridge_obj_state;
> +
> +			if (!bridge_obj_funcs->atomic_sro_readout_state) {

[Severity: High]
Can bridge_obj_funcs be NULL here for legacy non-atomic bridges?

Legacy (non-atomic) bridges do not call drm_atomic_private_obj_init() during
attachment. This means their embedded bridge->base is uninitialized, and its
funcs pointer remains NULL.

If this readout helper encounters a non-atomic bridge in the encoder's bridge
chain, dereferencing bridge_obj_funcs unconditionally will cause a NULL
pointer dereference.

> +				drm_warn(dev,
> +					 "No bridge %s readout implementation.",
> +					 bridge->name);
> +				continue;
> +			}
[ ... ]
> +struct drm_atomic_sro_state *
> +drm_atomic_helper_sro_build_state(struct drm_device *dev)
> +{
> +	struct drm_atomic_sro_state *state;
> +	struct drm_printer p = drm_dbg_printer(dev, DRM_UT_ATOMIC, NULL);
> +	int ret;
> +
> +	drm_dbg_kms(dev, "Starting to build atomic state from hardware state.\n");
> +
> +	state = drm_atomic_sro_state_alloc(dev);
> +	if (drm_WARN_ON(dev, IS_ERR(state)))
> +		return ERR_PTR(-ENOMEM);

[Severity: Low]
Does this error handling shadow the actual error pointer returned by
drm_atomic_sro_state_alloc()?

In drm_atomic_sro_state_alloc(), an allocation failure incorrectly returns
ERR_PTR(-EINVAL). Then, this function masks that error pointer and replaces
it with ERR_PTR(-ENOMEM) instead of propagating it.

While this results in a misleading error code on failure, it also violates
standard kernel error propagation patterns where the true error pointer is
typically returned directly.

> +
> +	ret = drm_atomic_helper_sro_readout_crtcs_state(dev, state);
> +	if (ret)
> +		goto err_state_free;
[ ... ]
> +int drm_atomic_helper_sro_readout_state(struct drm_device *dev)
> +{
> +	const struct drm_mode_config_helper_funcs *funcs =
> +		dev->mode_config.helper_private;
> +	struct drm_atomic_sro_state *state;
> +
> +	if (!drm_atomic_sro_device_can_readout(dev)) {
> +		drm_info(dev, "Device doesn't support hardware state readout.");
> +		return -ENOTSUPP;
> +	}
> +
> +	state = funcs->atomic_sro_build_state(dev);

[Severity: High]
Does this code handle cases where funcs is NULL, or where
funcs->atomic_sro_build_state is NULL?

The documentation for atomic_sro_build_state states that
drm_atomic_helper_sro_build_state() is the default implementation, which
implies drivers are not required to provide a custom hook.

If a driver does not provide this hook, or does not use helper private data,
calling funcs->atomic_sro_build_state(dev) unconditionally will result in a
NULL pointer dereference.

> +	if (IS_ERR(state))
> +		return PTR_ERR(state);
> +
> +	drm_atomic_sro_install_state(state);
> +	drm_atomic_sro_state_free(state);
> +
> +	return 0;
> +}

[Severity: High]
Is there a risk of a NULL pointer dereference during disabled bridge readout
when using drm_atomic_sro_get_crtc_state()?

If the bridge readout is called when the pipeline is inactive, a connector
state may not have an associated CRTC. The
drm_bridge_atomic_readout_priv_state() hook appears to pass this NULL CRTC
directly to drm_atomic_sro_get_crtc_state():

drm_bridge.c:drm_bridge_atomic_readout_priv_state() {
    ...
    crtc_state = drm_atomic_sro_get_crtc_state(state, conn_state->crtc);
    ...
}

Which unconditionally dereferences the CRTC pointer to fetch its index:

drm_atomic_sro.c:drm_atomic_sro_get_crtc_state() {
    ...
    unsigned int index = drm_crtc_index(crtc);
    ...
}

This can cause a kernel oops on boot if SRO is enabled and a disabled
pipeline is read out. Note that this issue was identified as being introduced
in a subsequent patch in the series and persists through the final commit.

[Severity: Medium]
Does drm_atomic_sro_install_state() have an O(N^2) loop and an out-of-bounds
read risk?

The loop condition in drm_atomic_sro_install_state() evaluates a dynamic
list count rather than the allocated array bounds:

drm_atomic_sro.c:drm_atomic_sro_install_state() {
    ...
    for (i = 0; i < count_private_obj(state->dev); i++) {
        const struct drm_private_state_funcs *obj_funcs;
        struct drm_private_state *obj_state;
        struct drm_private_obj *obj;

        obj = state->private_objs[i].ptr;
    ...
}

Because count_private_obj() iterates over a linked list on every iteration,
this creates an O(N^2) time complexity. Furthermore, if a private object is
registered between the allocation of the state->private_objs array and the
invocation of this loop, count_private_obj() will return a value larger than
the allocated array size. This would lead to an out-of-bounds read of
state->private_objs[i].ptr.

Using state->num_private_objs instead of count_private_obj() would avoid
both the complexity and the bounds risk.

[Severity: High]
Does drm_atomic_sro_set_connector_state() risk an out-of-bounds write when
connector indices exceed the active connector count?

drm_atomic_sro_state_init() allocates the state->connectors array sized
strictly to the current num_connector count:

drm_atomic_sro.c:drm_atomic_sro_state_init() {
    ...
    state->connectors = kzalloc_objs(*state->connectors,
                     dev->mode_config.num_connector);
    if (!state->connectors) {
    ...
}

However, drm_connector_index(conn) returns an IDA-allocated index. If
connectors are dynamically added and removed (for example, via DP MST
hot-unplug), the IDA index can be larger than the total number of currently
registered connectors. 

drm_atomic_sro_set_connector_state() blindly uses this potentially
out-of-bounds index to write into the array:

drm_atomic_sro.c:drm_atomic_sro_set_connector_state() {
    ...
    unsigned int index = drm_connector_index(conn);

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

This can corrupt kernel memory if state readout is performed after connector
removals.

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