Re: [PATCH v7 8/9] drm: writeback: clean up writeback connectors in drm_connector_cleanup()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm: out-of-bounds access in drm_writeback_connector_cleanup() downcast
- [High] drm: writeback: drm_writeback_cleanup_job() called under spin_lock_irqsave()
--

commit 01e86ffc01bb0a8fee1f22a88d9725baf85bf61d
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

drm: writeback: clean up writeback connectors in drm_connector_cleanup()

This commit calls drm_writeback_connector_cleanup() from
drm_connector_cleanup() for writeback connectors so that connectors
initialized with drm_writeback_connector_init() also drop their formats
blob and clean up queued writeback jobs upon teardown.

> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 893716a9e4895..1c2bef5e06363 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -929,6 +929,9 @@ void drm_connector_cleanup(struct drm_connector *connector)
>  	if (connector->hdmi.scdc_work_initialized)
>  		cancel_delayed_work_sync(&connector->hdmi.scdc_work);
>  
> +	if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK)
> +		drm_writeback_connector_cleanup(connector);
> +
>  	platform_device_unregister(connector->hdmi_audio.codec_pdev);
>  
>  	if (connector->privacy_screen) {

[Severity: High]
Is every connector with connector_type == DRM_MODE_CONNECTOR_WRITEBACK
guaranteed to be embedded inside a struct drm_writeback_connector?

Unlike connector->hdmi and connector->hdmi_audio, which are fields inside
struct drm_connector itself, drm_writeback_connector_cleanup() downcasts
connector via drm_connector_to_writeback() and accesses
wb_connector->pixel_formats_blob_ptr, wb_connector->job_lock, and
wb_connector->job_queue beyond sizeof(struct drm_connector).

However, drm_connector_init(), drmm_connector_init(), and
drm_connector_dynamic_init() allow initializing a plain struct
drm_connector (or a different wrapper struct) with
DRM_MODE_CONNECTOR_WRITEBACK, and several in-tree KUnit tests do so:

1. In drivers/gpu/drm/tests/drm_connector_test.c, struct
drm_connector_init_priv places a plain struct drm_connector immediately
before struct i2c_adapter ddc:

struct drm_connector_init_priv {
	struct drm_device drm;
	struct drm_connector connector;
	struct i2c_adapter ddc;
};

When drm_test_drmm_connector_init_type_valid() or
drm_test_drm_connector_dynamic_init_type_valid() initializes
priv->connector with DRM_MODE_CONNECTOR_WRITEBACK and later cleans it up
via drm_connector_cleanup(), wb_connector->pixel_formats_blob_ptr aliases
priv->ddc.owner (set to THIS_MODULE). When built as a module,
drm_writeback_connector_cleanup() passes THIS_MODULE to
drm_property_blob_put(), zeroes priv->ddc.owner, locks priv->ddc memory as
job_lock, and iterates job_queue over priv->ddc fields.

2. In drivers/gpu/drm/vc4/tests/vc4_mock_output.c, vc4_dummy_output()
allocates struct vc4_dummy_output (ending with a plain struct
drm_connector) and calls drmm_connector_init() with
DRM_MODE_CONNECTOR_WRITEBACK, causing drm_writeback_connector_cleanup() to
read past the end of the slab allocation.

3. In drivers/gpu/drm/amd/display/amdgpu_dm/tests/amdgpu_dm_audio_test.c,
dm_test_audio_component_get_eld_copies_matching_connector() allocates
sizeof(struct drm_connector) with DRM_MODE_CONNECTOR_WRITEBACK and cleans
it up via drm_connector_cleanup(), also reading out of bounds.

Could this out-of-bounds access be avoided by tracking writeback
initialization on struct drm_connector or only cleaning up when initialized
through the writeback connector helpers?

[ ... ]

> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index 2f6a7b50336c0..272012a70efee 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -271,21 +273,27 @@ EXPORT_SYMBOL(drm_writeback_connector_init);
>  
>  /**
>   * drm_writeback_connector_cleanup - Cleanup the writeback connector
> - * @dev: DRM device
> - * @data: Pointer to the writeback connector to clean up
> + * @connector: Writeback connector to clean up
>   *
>   * This will decrement the reference counter of blobs. It will also clean the
>   * remaining jobs in this writeback connector. Caution: This helper will not
>   * clean up the attached encoder and the drm_connector.
> + *
> + * Called from drm_connector_cleanup() for all writeback connectors.
>   */
> -static void drm_writeback_connector_cleanup(struct drm_device *dev,
> -					    void *data)
> +void drm_writeback_connector_cleanup(struct drm_connector *connector)
>  {
> -	unsigned long flags;
> +	struct drm_writeback_connector *wb_connector =
> +		drm_connector_to_writeback(connector);
>  	struct drm_writeback_job *pos, *n;
> -	struct drm_writeback_connector *wb_connector = data;
> +	unsigned long flags;
> +
> +	/* __drm_writeback_connector_init() has failed */
> +	if (!wb_connector->pixel_formats_blob_ptr)
> +		return;
>  
>  	drm_property_blob_put(wb_connector->pixel_formats_blob_ptr);
> +	wb_connector->pixel_formats_blob_ptr = NULL;
>  
>  	spin_lock_irqsave(&wb_connector->job_lock, flags);
>  	list_for_each_entry_safe(pos, n, &wb_connector->job_queue, list_entry) {
>  		list_del(&pos->list_entry);
>  		drm_writeback_cleanup_job(pos);
>  	}
>  	spin_unlock_irqrestore(&wb_connector->job_lock, flags);
>  }

[Severity: High]
Can drm_writeback_cleanup_job() be safely called while holding
wb_connector->job_lock with interrupts disabled?

When drm_connector_cleanup() tears down a writeback connector that still
has queued jobs, drm_writeback_connector_cleanup() holds
spin_lock_irqsave(&wb_connector->job_lock, flags) across
drm_writeback_cleanup_job():

drm_writeback_connector_cleanup()
  spin_lock_irqsave(&wb_connector->job_lock, flags)
  drm_writeback_cleanup_job()
    funcs->cleanup_writeback_job()
    drm_framebuffer_put()

Inside drm_writeback_cleanup_job(), dropping the final reference via
drm_framebuffer_put() calls drm_framebuffer_free() ->
drm_mode_object_unregister(), which acquires
mutex_lock(&dev->mode_config.idr_mutex), as well as
drm_framebuffer_cleanup(), which acquires
mutex_lock(&dev->object_name_lock) and
mutex_lock(&dev->mode_config.fb_lock).

In addition, hooking drm_writeback_connector_cleanup() into
drm_connector_cleanup() makes this path reachable for drivers using
drm_writeback_connector_init() such as amdgpu, where
amdgpu_dm_wb_cleanup_job() calls wb_bo_reserve() -> amdgpu_bo_reserve()
and acquires a sleeping dma_resv ww_mutex.

Should the jobs be spliced onto a local list under wb_connector->job_lock
and cleaned up after spin_unlock_irqrestore()?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019-wb-drop-encoder-v7-0-d4288a26eb54@oss.qualcomm.com?part=8