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 <[email protected]> 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/[email protected]?part=8
