Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/vkms: NULL dereference in vkms_plane_create_state() if property alloc fails - [Medium] drm/vkms: missing connector atomic_create_state breaks DRM_MODE_ATOMIC_RESET --
commit f0d606e22df5b4b30135537f8737279247724656 Author: Maxime Ripard <[email protected]> drm/vkms: Add driver-specific plane property for testing This commit adds a driver-specific "vkms-test-do-not-use-in-prod" range property to vkms planes so userspace tests can verify that DRM_MODE_ATOMIC_RESET resets plane state properties to their default values. > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c > b/drivers/gpu/drm/vkms/vkms_plane.c > index fd3d34dcf0fe5..0ebc131c2788f 100644 > --- a/drivers/gpu/drm/vkms/vkms_plane.c > +++ b/drivers/gpu/drm/vkms/vkms_plane.c [ ... ] > @@ -85,7 +92,9 @@ static void vkms_plane_destroy_state(struct drm_plane > *plane, > > static struct drm_plane_state *vkms_plane_create_state(struct drm_plane > *plane) > { > + struct vkms_plane *vkms_plane = to_vkms_plane(plane); > struct vkms_plane_state *vkms_state; > + u64 val; > > vkms_state = kzalloc_obj(*vkms_state); > if (!vkms_state) { > @@ -95,14 +104,53 @@ static struct drm_plane_state > *vkms_plane_create_state(struct drm_plane *plane) > > __drm_gem_shadow_plane_state_init(plane, &vkms_state->base); > > + if (!drm_object_property_get_default_value(&plane->base, > + > vkms_plane->vkms_test_do_not_use_property, > + &val)) > + vkms_state->vkms_test_do_not_use_value = val; [Severity: High] Can this trigger a NULL pointer dereference in drm_object_property_get_default_value() if drm_property_create_range() fails in vkms_plane_init()? In drivers/gpu/drm/vkms/vkms_plane.c, vkms_plane_init() treats a drm_property_create_range() failure as non-fatal and leaves plane->vkms_test_do_not_use_property as NULL: plane->vkms_test_do_not_use_property = drm_property_create_range(dev, 0, "vkms-test-do-not-use-in-prod", 0, U32_MAX); if (plane->vkms_test_do_not_use_property) drm_object_attach_property(&plane->base.base, plane->vkms_test_do_not_use_property, 0); When vkms_plane_create_state() is subsequently called during device initialization via vkms_output_init() -> drm_mode_config_reset() (or during a DRM_MODE_ATOMIC_RESET commit via drm_atomic_commit_fill_with_defaults()), it unconditionally passes vkms_plane->vkms_test_do_not_use_property to drm_object_property_get_default_value(). However, in drivers/gpu/drm/drm_mode_object.c, drm_object_property_get_default_value() immediately dereferences property->dev without checking whether property is NULL: int drm_object_property_get_default_value(struct drm_mode_object *obj, struct drm_property *property, uint64_t *val) { WARN_ON(!drm_drv_uses_atomic_modeset(property->dev)); return __drm_object_property_get_prop_value(obj, property, val); } Should vkms_plane_create_state() check that vkms_plane->vkms_test_do_not_use_property is non-NULL before calling drm_object_property_get_default_value(), or should vkms_plane_init() fail when drm_property_create_range() returns NULL? [ ... ] > @@ -283,6 +331,15 @@ struct vkms_plane *vkms_plane_init(struct vkms_device > *vkmsdev, > DRM_COLOR_YCBCR_BT601, > DRM_COLOR_YCBCR_FULL_RANGE); > > + plane->vkms_test_do_not_use_property = > + drm_property_create_range(dev, 0, > + "vkms-test-do-not-use-in-prod", > + 0, U32_MAX); > + if (plane->vkms_test_do_not_use_property) > + drm_object_attach_property(&plane->base.base, > + plane->vkms_test_do_not_use_property, > + 0); [Severity: Medium] Will userspace be able to use DRM_MODE_ATOMIC_RESET to test resetting this property on vkms while vkms connectors do not implement atomic_create_state? When userspace queries DRM_CAP_ATOMIC_RESET in drm_getcap() or submits an atomic commit with DRM_MODE_ATOMIC_RESET in drm_mode_atomic_ioctl(), drm_atomic_implements_create_state() and drm_atomic_commit_fill_with_defaults() in drivers/gpu/drm/drm_atomic.c require every connector on the device to provide an atomic_create_state callback: drm_connector_list_iter_begin(dev, &conn_iter); drm_for_each_connector_iter(connector, &conn_iter) { if (!connector->funcs->atomic_create_state) { drm_connector_list_iter_end(&conn_iter); return false; } } drm_connector_list_iter_end(&conn_iter); However, vkms_connector_funcs in drivers/gpu/drm/vkms/vkms_connector.c and vkms_wb_connector_funcs in drivers/gpu/drm/vkms/vkms_writeback.c still define .reset = drm_atomic_helper_connector_reset without setting .atomic_create_state: static const struct drm_connector_funcs vkms_connector_funcs = { .detect = vkms_connector_detect, .fill_modes = drm_helper_probe_single_connector_modes, .reset = drm_atomic_helper_connector_reset, .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, }; Because every valid vkms device registers at least one connector, drm_atomic_implements_create_state() always returns false on vkms, so DRM_CAP_ATOMIC_RESET reports 0 and DRM_MODE_ATOMIC_RESET commits fail with -EOPNOTSUPP before vkms_plane_create_state() can be reached. Do vkms_connector_funcs and vkms_wb_connector_funcs also need to use .atomic_create_state = drm_atomic_helper_connector_create_state so that DRM_MODE_ATOMIC_RESET is supported on vkms? [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=15
