From e40b7642884239346265cd7adc06dfbb8988f93d Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Thu, 28 Feb 2019 15:49:04 +0100 Subject: [PATCH 1/8] drm/leases: Drop object_id validation for negative ids Not exactly sure what's the aim here, but the canonical nil object has id == 0, we don't use negative object ids for anything. Plus all object_id are valided by the object_idr, there's nothing we need to do on top of that ENOENT check a bit further down. Spotted while typing exhaustive igt coverage for all these corner-cases. Cc: Keith Packard Reviewed-by: Boris Brezillon Reviewed-by: Dave Airlie Signed-off-by: Daniel Vetter Link: https://patchwork.freedesktop.org/patch/msgid/20190228144910.26488-2-daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_lease.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c index 603b0bd9c5ce..1176d814cf7f 100644 --- a/drivers/gpu/drm/drm_lease.c +++ b/drivers/gpu/drm/drm_lease.c @@ -403,11 +403,6 @@ static int fill_object_idr(struct drm_device *dev, /* step one - get references to all the mode objects and check for validity. */ for (o = 0; o < object_count; o++) { - if ((int) object_ids[o] < 0) { - ret = -EINVAL; - goto out_free_objects; - } - objects[o] = drm_mode_object_find(dev, lessor_priv, object_ids[o], DRM_MODE_OBJECT_ANY); From ae9d6c076c0d39e97c14cd531d278ea8471730f4 Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Thu, 28 Feb 2019 15:49:05 +0100 Subject: [PATCH 2/8] drm/lease: Drop recursive leads checks We disallow subleasing, so no point checking whether the master holds all the leases - it will. Spotted while typing exhaustive igt coverage for all these corner cases. Cc: Keith Packard Reviewed-by: Boris Brezillon Reviewed-by: Dave Airlie Signed-off-by: Daniel Vetter Link: https://patchwork.freedesktop.org/patch/msgid/20190228144910.26488-3-daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_lease.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c index 1176d814cf7f..cce5d9dd52ff 100644 --- a/drivers/gpu/drm/drm_lease.c +++ b/drivers/gpu/drm/drm_lease.c @@ -220,8 +220,6 @@ static struct drm_master *drm_lease_create(struct drm_master *lessor, struct idr error = 0; if (!idr_find(&dev->mode_config.object_idr, object)) error = -ENOENT; - else if (!_drm_lease_held_master(lessor, object)) - error = -EACCES; else if (_drm_has_leased(lessor, object)) error = -EBUSY; From 4285c7e1aaad6ffe5520b36047a28fc960e32482 Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Thu, 28 Feb 2019 15:49:06 +0100 Subject: [PATCH 3/8] drm/leases: Don't init to 0 in drm_master_create We kzalloc. Cc: Keith Packard Reviewed-by: Boris Brezillon Reviewed-by: Dave Airlie Reviewed-by: Mika Kuoppala Signed-off-by: Daniel Vetter Link: https://patchwork.freedesktop.org/patch/msgid/20190228144910.26488-4-daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_auth.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c index 1669c42c40ed..bcf0a5a1018f 100644 --- a/drivers/gpu/drm/drm_auth.c +++ b/drivers/gpu/drm/drm_auth.c @@ -109,8 +109,6 @@ struct drm_master *drm_master_create(struct drm_device *dev) master->dev = dev; /* initialize the tree of output resource lessees */ - master->lessor = NULL; - master->lessee_id = 0; INIT_LIST_HEAD(&master->lessees); INIT_LIST_HEAD(&master->lessee_list); idr_init(&master->leases); From 46b757780dfe60596124b85befa935554dba73a3 Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Thu, 28 Feb 2019 15:49:07 +0100 Subject: [PATCH 4/8] drm/lease: Check for lessor outside of locks The lessor is invariant over a lifetime of a lease, we don't have to grab any locks for that. Speeds up the common case of not being a lease. Cc: Keith Packard Reviewed-by: Boris Brezillon Reviewed-by: Dave Airlie Signed-off-by: Daniel Vetter Link: https://patchwork.freedesktop.org/patch/msgid/20190228144910.26488-5-daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_lease.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c index cce5d9dd52ff..694ff363a90b 100644 --- a/drivers/gpu/drm/drm_lease.c +++ b/drivers/gpu/drm/drm_lease.c @@ -111,7 +111,7 @@ static bool _drm_has_leased(struct drm_master *master, int id) */ bool _drm_lease_held(struct drm_file *file_priv, int id) { - if (file_priv == NULL || file_priv->master == NULL) + if (!file_priv || !file_priv->master) return true; return _drm_lease_held_master(file_priv->master, id); @@ -133,7 +133,7 @@ bool drm_lease_held(struct drm_file *file_priv, int id) struct drm_master *master; bool ret; - if (file_priv == NULL || file_priv->master == NULL) + if (!file_priv || !file_priv->master || !file_priv->master->lessor) return true; master = file_priv->master; @@ -159,7 +159,7 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in) int count_in, count_out; uint32_t crtcs_out = 0; - if (file_priv == NULL || file_priv->master == NULL) + if (!file_priv || !file_priv->master || !file_priv->master->lessor) return crtcs_in; master = file_priv->master; From 204f640da6914844b3270b41b29c84f6e3b74083 Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Thu, 28 Feb 2019 15:49:08 +0100 Subject: [PATCH 5/8] drm/lease: Make sure implicit planes are leased If userspace doesn't enable universal planes, then we automatically add the primary and cursor planes. But for universal userspace there's no such check (and maybe we only want to give the lessee one plane, maybe not even the primary one), hence we need to check for the implied plane. v2: don't forget setcrtc ioctl. v3: Still allow disabling of the crtc in SETCRTC. Cc: stable@vger.kernel.org Cc: Keith Packard Reviewed-by: Boris Brezillon Signed-off-by: Daniel Vetter Link: https://patchwork.freedesktop.org/patch/msgid/20190228144910.26488-6-daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_crtc.c | 4 ++++ drivers/gpu/drm/drm_plane.c | 8 ++++++++ 2 files changed, 12 insertions(+) diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 7dabbaf033a1..790ba5941954 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -559,6 +559,10 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, plane = crtc->primary; + /* allow disabling with the primary plane leased */ + if (crtc_req->mode_valid && !drm_lease_held(file_priv, plane->base.id)) + return -EACCES; + mutex_lock(&crtc->dev->mode_config.mutex); DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE, ret); diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 4cfb56893b7f..d6ad60ab0d38 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -960,6 +960,11 @@ retry: if (ret) goto out; + if (!drm_lease_held(file_priv, crtc->cursor->base.id)) { + ret = -EACCES; + goto out; + } + ret = drm_mode_cursor_universal(crtc, req, file_priv, &ctx); goto out; } @@ -1062,6 +1067,9 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, plane = crtc->primary; + if (!drm_lease_held(file_priv, plane->base.id)) + return -EACCES; + if (crtc->funcs->page_flip_target) { u32 current_vblank; int r; From 36e4523aaf4a35de963e190064b53839fa131653 Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Thu, 28 Feb 2019 15:49:09 +0100 Subject: [PATCH 6/8] drm/atomic: Wire file_priv through for property changes We need this to make sure lessees can only connect their plane/connectors to crtc objects they own. And note that this is irrespective of whether the lessor is atomic or not, lessor cannot prevent lessees from enabling atomic. Cc: stable@vger.kernel.org Cc: Keith Packard Reviewed-by: Dave Airlie Signed-off-by: Daniel Vetter Link: https://patchwork.freedesktop.org/patch/msgid/20190228144910.26488-7-daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_atomic_uapi.c | 32 +++++++++++++++++------------ drivers/gpu/drm/drm_crtc_internal.h | 1 + drivers/gpu/drm/drm_mode_object.c | 5 +++-- 3 files changed, 23 insertions(+), 15 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index ea797d4c82ee..77d16f5d4a9a 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -512,8 +512,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc, } static int drm_atomic_plane_set_property(struct drm_plane *plane, - struct drm_plane_state *state, struct drm_property *property, - uint64_t val) + struct drm_plane_state *state, struct drm_file *file_priv, + struct drm_property *property, uint64_t val) { struct drm_device *dev = plane->dev; struct drm_mode_config *config = &dev->mode_config; @@ -521,7 +521,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, int ret; if (property == config->prop_fb_id) { - struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, NULL, val); + struct drm_framebuffer *fb; + fb = drm_framebuffer_lookup(dev, file_priv, val); drm_atomic_set_fb_for_plane(state, fb); if (fb) drm_framebuffer_put(fb); @@ -537,7 +538,7 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, return -EINVAL; } else if (property == config->prop_crtc_id) { - struct drm_crtc *crtc = drm_crtc_find(dev, NULL, val); + struct drm_crtc *crtc = drm_crtc_find(dev, file_priv, val); return drm_atomic_set_crtc_for_plane(state, crtc); } else if (property == config->prop_crtc_x) { state->crtc_x = U642I64(val); @@ -668,14 +669,14 @@ static int drm_atomic_set_writeback_fb_for_connector( } static int drm_atomic_connector_set_property(struct drm_connector *connector, - struct drm_connector_state *state, struct drm_property *property, - uint64_t val) + struct drm_connector_state *state, struct drm_file *file_priv, + struct drm_property *property, uint64_t val) { struct drm_device *dev = connector->dev; struct drm_mode_config *config = &dev->mode_config; if (property == config->prop_crtc_id) { - struct drm_crtc *crtc = drm_crtc_find(dev, NULL, val); + struct drm_crtc *crtc = drm_crtc_find(dev, file_priv, val); return drm_atomic_set_crtc_for_connector(state, crtc); } else if (property == config->dpms_property) { /* setting DPMS property requires special handling, which @@ -736,8 +737,10 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, } else if (property == connector->colorspace_property) { state->colorspace = val; } else if (property == config->writeback_fb_id_property) { - struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, NULL, val); - int ret = drm_atomic_set_writeback_fb_for_connector(state, fb); + struct drm_framebuffer *fb; + int ret; + fb = drm_framebuffer_lookup(dev, file_priv, val); + ret = drm_atomic_set_writeback_fb_for_connector(state, fb); if (fb) drm_framebuffer_put(fb); return ret; @@ -934,6 +937,7 @@ out: } int drm_atomic_set_property(struct drm_atomic_state *state, + struct drm_file *file_priv, struct drm_mode_object *obj, struct drm_property *prop, uint64_t prop_value) @@ -956,7 +960,8 @@ int drm_atomic_set_property(struct drm_atomic_state *state, } ret = drm_atomic_connector_set_property(connector, - connector_state, prop, prop_value); + connector_state, file_priv, + prop, prop_value); break; } case DRM_MODE_OBJECT_CRTC: { @@ -984,7 +989,8 @@ int drm_atomic_set_property(struct drm_atomic_state *state, } ret = drm_atomic_plane_set_property(plane, - plane_state, prop, prop_value); + plane_state, file_priv, + prop, prop_value); break; } default: @@ -1354,8 +1360,8 @@ retry: goto out; } - ret = drm_atomic_set_property(state, obj, prop, - prop_value); + ret = drm_atomic_set_property(state, file_priv, + obj, prop, prop_value); if (ret) { drm_mode_object_put(obj); goto out; diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h index 216f2a9ee3d4..0719a235d6cc 100644 --- a/drivers/gpu/drm/drm_crtc_internal.h +++ b/drivers/gpu/drm/drm_crtc_internal.h @@ -214,6 +214,7 @@ int drm_atomic_connector_commit_dpms(struct drm_atomic_state *state, struct drm_connector *connector, int mode); int drm_atomic_set_property(struct drm_atomic_state *state, + struct drm_file *file_priv, struct drm_mode_object *obj, struct drm_property *prop, uint64_t prop_value); diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c index a9005c1c2384..f32507e65b79 100644 --- a/drivers/gpu/drm/drm_mode_object.c +++ b/drivers/gpu/drm/drm_mode_object.c @@ -451,6 +451,7 @@ static int set_property_legacy(struct drm_mode_object *obj, } static int set_property_atomic(struct drm_mode_object *obj, + struct drm_file *file_priv, struct drm_property *prop, uint64_t prop_value) { @@ -477,7 +478,7 @@ retry: obj_to_connector(obj), prop_value); } else { - ret = drm_atomic_set_property(state, obj, prop, prop_value); + ret = drm_atomic_set_property(state, file_priv, obj, prop, prop_value); if (ret) goto out; ret = drm_atomic_commit(state); @@ -520,7 +521,7 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data, goto out_unref; if (drm_drv_uses_atomic_modeset(property->dev)) - ret = set_property_atomic(arg_obj, property, arg->value); + ret = set_property_atomic(arg_obj, file_priv, property, arg->value); else ret = set_property_legacy(arg_obj, property, arg->value); From b8c7d7bc8386924f57aac0a745ae3240f07b743b Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Thu, 28 Feb 2019 15:49:10 +0100 Subject: [PATCH 7/8] drm/atomic: -EACCESS for lease-denied crtc lookup With the previous patch drm_crtc_find will return NULL when the crtc isn't in our lease, which will then disable the plane/connector. No longer an issue since the lessor can't escape their lease terms anymore, but not quite great semantics yet either. Catch this and return -EACCES, so that at least evil test cases have a better chance of making sure the kernel works correctly. Reviewed-by: Dave Airlie Signed-off-by: Daniel Vetter Link: https://patchwork.freedesktop.org/patch/msgid/20190228144910.26488-8-daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_atomic_uapi.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 77d16f5d4a9a..428d82662dc4 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -539,6 +539,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, } else if (property == config->prop_crtc_id) { struct drm_crtc *crtc = drm_crtc_find(dev, file_priv, val); + if (val && !crtc) + return -EACCES; return drm_atomic_set_crtc_for_plane(state, crtc); } else if (property == config->prop_crtc_x) { state->crtc_x = U642I64(val); @@ -677,6 +679,8 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, if (property == config->prop_crtc_id) { struct drm_crtc *crtc = drm_crtc_find(dev, file_priv, val); + if (val && !crtc) + return -EACCES; return drm_atomic_set_crtc_for_connector(state, crtc); } else if (property == config->dpms_property) { /* setting DPMS property requires special handling, which From 1de7259275ca4ebc66459de6620558d3e38d4142 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Noralf=20Tr=C3=B8nnes?= Date: Tue, 23 Apr 2019 16:53:53 +0200 Subject: [PATCH 8/8] drm/fb-helper: Fix drm_fb_helper_firmware_config() NULL pointer deref MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Non-atomic drivers like ast doesn't have connector->state set resulting in a NULL pointer deref: [ 29.609593] BUG: unable to handle kernel NULL pointer dereference at 0000000000000010 [ 29.609619] Call Trace: [ 29.609630] ? drm_helper_probe_single_connector_modes+0x27f/0x680 [ 29.609640] drm_setup_crtcs+0x431/0xd80 [drm_kms_helper] [ 29.753065] __drm_fb_helper_initial_config_and_unlock+0x6f/0x6a0 [ 29.753160] ? drm_modeset_unlock_all+0x31/0x50 [drm] [ 29.765758] ast_fbdev_init+0xa8/0xc0 [ast] [ 29.765762] ast_driver_load.cold.7+0x2b3/0xe11 [ast] [ 29.765775] drm_dev_register+0x111/0x150 [drm] Fix by bailing out if the driver does not support atomic modesetting. Fixes: 09ded8af57bc ("drm/i915/fbdev: Move intel_fb_initial_config() to fbdev helper") Reported-by: Thomas Zimmermann Cc: Daniel Vetter Cc: Jani Nikula Signed-off-by: Noralf Trønnes Reviewed-by: Daniel Vetter Tested-by: Thomas Zimmermann Link: https://patchwork.freedesktop.org/patch/msgid/20190423145353.30158-1-noralf@tronnes.org --- drivers/gpu/drm/drm_fb_helper.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 226c0910ba91..498f95c3e81d 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -2588,6 +2588,9 @@ static bool drm_fb_helper_firmware_config(struct drm_fb_helper *fb_helper, int num_connectors_detected = 0; struct drm_modeset_acquire_ctx ctx; + if (!drm_drv_uses_atomic_modeset(dev)) + return false; + save_enabled = kcalloc(count, sizeof(bool), GFP_KERNEL); if (!save_enabled) return false;