Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm: NULL pointer dereference of modes[i] in drm_client_firmware_config - [Medium] drm: Logical bug in drm_client_target_cloned due to stale modes[i] values --
commit 0e7e38749489efdbc43f11da085113315e670013 Author: Hugo Sales <[email protected]> drm/client: Probe connectors with the device's mode size limits This patch updates drm_client_modeset_probe() to use the device's mode size limits rather than the client's dimensions when probing connectors. It shifts the size validation to the mode pickers by calling drm_mode_validate_size() to ensure the shared modes list retains larger modes. This allows keeping panels' preferred modes even if smaller modes are initially selected. Link: https://gitlab.freedesktop.org/drm/amd/-/issues/5983 > diff --git a/drivers/gpu/drm/drm_client_modeset.c > b/drivers/gpu/drm/drm_client_modeset.c > index 0080a8e9556a5..e79c7257aaa2e 100644 > --- a/drivers/gpu/drm/drm_client_modeset.c > +++ b/drivers/gpu/drm/drm_client_modeset.c > @@ -357,6 +370,8 @@ static bool drm_client_target_cloned(struct drm_device > *dev, > continue; > > list_for_each_entry(mode, &connectors[i]->modes, head) { > + if (drm_mode_validate_size(mode, width, height) != > MODE_OK) > + continue; > if (drm_mode_match(mode, dmt_mode, > DRM_MODE_MATCH_TIMINGS | > DRM_MODE_MATCH_CLOCK | [Severity: Medium] Can this result in a stale modes[i] value being incorrectly accepted? In drm_client_target_cloned(), the driver tries to find a matching 1024x768 mode across all connectors if command-line cloning fails. If a connector doesn't have a 1024x768 mode, or if the mode is skipped because it exceeds the width and height constraints added by this patch, mode_replace() is bypassed: list_for_each_entry(mode, &connectors[i]->modes, head) { if (drm_mode_validate_size(mode, width, height) != MODE_OK) continue; if (drm_mode_match(mode, dmt_mode, ...)) mode_replace(dev, &modes[i], mode); } if (!modes[i]) can_clone = false; Because modes[i] is not cleared before this loop, it retains whatever value it was assigned in the earlier command-line pass. The termination check if (!modes[i]) evaluates to false, and can_clone incorrectly remains true even if heterogeneous modes are present. While this is a preexisting structural flaw in the function, does adding the drm_mode_validate_size() check inside the 1024x768 loop trigger it by making it far more likely for a 1024x768 mode to be skipped? [ ... ] > @@ -750,7 +767,8 @@ static bool drm_client_firmware_config(struct > drm_client_dev *client, > num_tiled_conns < connector->num_h_tile * > connector->num_v_tile) { > mode_type = "non tiled"; > mode_replace(dev, &modes[i], > - > drm_connector_fallback_non_tiled_mode(connector)); > + > drm_connector_fallback_non_tiled_mode(connector, > + > width, height)); > } > crtcs[i] = crtc; [Severity: High] Does this code risk a NULL pointer dereference in drm_client_firmware_config()? If a connector is part of a tiled display but some tiles are missing, the driver attempts to fall back to a non-tiled mode by calling drm_connector_fallback_non_tiled_mode(). This patch modifies the fallback function to filter out any modes that exceed the client's width and height. If no modes fit these constraints, the function returns NULL. When this happens, mode_replace() safely destroys the existing mode and assigns NULL to modes[i]. However, immediately following this block, drm_dbg_kms() unconditionally dereferences modes[i]->name when logging the selected mode: crtcs[i] = crtc; drm_dbg_kms(dev, "[CONNECTOR::%d:%s] on [CRTC:%d:%s] using %s mode: %s\n", connector->base.id, connector->name, crtc->base.id, crtc->name, mode_type, modes[i]->name); Since the new size filter makes returning NULL far more likely, can this cause a crash if modes[i] becomes NULL? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
