Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Medium] drm: Missing DRM core validation for VRR target properties - [Medium] drm: Missing DRM_MODE_PROP_ATOMIC flag on multi-part VRR target properties - [Low] drm: Redundant bounds check for VRR target properties in drm_atomic_crtc_set_property --
commit 5c8aa8fe0326bb376d279373eb0661cd2fcc114e Author: Nicolas Frattaroli <[email protected]> Subject: drm: Add VRR target frame rate properties This commit introduces new VRR target frame rate properties, allowing user-space to set a target refresh rate range. The rates are expressed as rational fractions using numerators and denominators. It adds the UAPI properties, validation fields, and attaches them to CRTC objects. > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c > b/drivers/gpu/drm/drm_atomic_uapi.c > index 1eebcf6f5e05b..c9eb01d740b3e 100644 > --- a/drivers/gpu/drm/drm_atomic_uapi.c > +++ b/drivers/gpu/drm/drm_atomic_uapi.c > @@ -420,6 +420,22 @@ static int drm_atomic_crtc_set_property(struct drm_crtc > *crtc, > return ret; > } else if (property == config->prop_vrr_enabled) { > state->vrr_enabled = val; > + } else if (property == config->prop_vrr_min_numerator) { > + if (val > U32_MAX) > + return -EINVAL; [Severity: Low] Is this manual bounds check necessary? Since this is a range property created with limits of 0 to U32_MAX, the DRM core's drm_property_change_valid_get() should already validate val before reaching drm_atomic_crtc_set_property(), making this check redundant. > + state->vrr_state.vrr_min_n = val; > + } else if (property == config->prop_vrr_min_denominator) { > + if (val > U32_MAX) > + return -EINVAL; > + state->vrr_state.vrr_min_d = val; [Severity: Medium] Could this lack of validation allow structurally invalid states? The UAPI documentation states the denominator must not be zero if the numerator is non-zero. By accepting the denominator without validating it against the numerator here, this constraint isn't enforced in the DRM core. Could this cause drivers implementing VRR to hit divide-by-zero crashes if they don't manually validate these constraints themselves? [ ... ] > diff --git a/drivers/gpu/drm/drm_mode_config.c > b/drivers/gpu/drm/drm_mode_config.c > index 48a56f23dadbb..da967d60f832a 100644 > --- a/drivers/gpu/drm/drm_mode_config.c > +++ b/drivers/gpu/drm/drm_mode_config.c > @@ -535,6 +535,26 @@ static int drm_mode_create_standard_properties(struct > drm_device *dev) > return -ENOMEM; > dev->mode_config.prop_vrr_enabled = prop; > > + prop = drm_property_create_range(dev, 0, "VRR_MIN_NUMERATOR", 0, > U32_MAX); [Severity: Medium] Should these multi-part properties be created with the DRM_MODE_PROP_ATOMIC flag? Without it, they are exposed to legacy user-space applications. Since legacy userspace updates properties one at a time using drm_mode_obj_set_property_ioctl(), setting a non-zero numerator will immediately trigger a commit while the denominator is still 0. This passes an invalid intermediate state to the driver, causing it to reject the modeset and making it impossible for legacy userspace to configure a target rate. > + if (!prop) > + return -ENOMEM; > + dev->mode_config.prop_vrr_min_numerator = prop; -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=13
