From 4933c9a494416a8edc3166ce3f3ce3836a6af01d Mon Sep 17 00:00:00 2001 From: Emil Velikov Date: Mon, 14 Jan 2019 08:43:05 +0000 Subject: drm: drop DRM_AUTH requirement from AUTH_MAGIC ioctl Currently only an authenticated master can authenticate another client. In practise the client can only be master if CAP_SYS_ADMIN is present, although having the CAP also sets the client as authenticated. Thus DRM_AUTH in AUTH_MAGIC's "DRM_AUTH | DRM_MASTER" is superfluous. Notices while working on IGT tests. Signed-off-by: Emil Velikov Reviewed-by: Daniel Vetter Link: https://patchwork.freedesktop.org/patch/msgid/20190114084305.15141-1-emil.l.velikov@gmail.com --- drivers/gpu/drm/drm_ioctl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/gpu/drm') diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 7e6746b2d704..ab5692104ea0 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -570,7 +570,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = { DRM_IOCTL_DEF(DRM_IOCTL_SET_UNIQUE, drm_invalid_op, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY), DRM_IOCTL_DEF(DRM_IOCTL_BLOCK, drm_noop, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY), DRM_IOCTL_DEF(DRM_IOCTL_UNBLOCK, drm_noop, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY), - DRM_IOCTL_DEF(DRM_IOCTL_AUTH_MAGIC, drm_authmagic, DRM_AUTH|DRM_UNLOCKED|DRM_MASTER), + DRM_IOCTL_DEF(DRM_IOCTL_AUTH_MAGIC, drm_authmagic, DRM_UNLOCKED|DRM_MASTER), DRM_IOCTL_DEF(DRM_IOCTL_ADD_MAP, drm_legacy_addmap_ioctl, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY), DRM_IOCTL_DEF(DRM_IOCTL_RM_MAP, drm_legacy_rmmap_ioctl, DRM_AUTH), -- cgit v1.2.3 From e21710a893c8705360b764fa44061d72d65fb2ae Mon Sep 17 00:00:00 2001 From: Emil Velikov Date: Mon, 14 Jan 2019 08:44:09 +0000 Subject: drm: factor out drm_close_helper() function Will be used to plug an existing memory leak. Signed-off-by: Emil Velikov Reviewed-by: Daniel Vetter Link: https://patchwork.freedesktop.org/patch/msgid/20190114084410.15266-1-emil.l.velikov@gmail.com --- drivers/gpu/drm/drm_file.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) (limited to 'drivers/gpu/drm') diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c index 46f48f245eb5..e9607acfb629 100644 --- a/drivers/gpu/drm/drm_file.c +++ b/drivers/gpu/drm/drm_file.c @@ -262,6 +262,18 @@ void drm_file_free(struct drm_file *file) kfree(file); } +static void drm_close_helper(struct file *filp) +{ + struct drm_file *file_priv = filp->private_data; + struct drm_device *dev = file_priv->minor->dev; + + mutex_lock(&dev->filelist_mutex); + list_del(&file_priv->lhead); + mutex_unlock(&dev->filelist_mutex); + + drm_file_free(file_priv); +} + static int drm_setup(struct drm_device * dev) { int ret; @@ -473,11 +485,7 @@ int drm_release(struct inode *inode, struct file *filp) DRM_DEBUG("open_count = %d\n", dev->open_count); - mutex_lock(&dev->filelist_mutex); - list_del(&file_priv->lhead); - mutex_unlock(&dev->filelist_mutex); - - drm_file_free(file_priv); + drm_close_helper(filp); if (!--dev->open_count) { drm_lastclose(dev); -- cgit v1.2.3 From 4acc5be3cd89cc4132d8456144ee066905a58e41 Mon Sep 17 00:00:00 2001 From: Emil Velikov Date: Mon, 14 Jan 2019 08:44:10 +0000 Subject: drm: plug memory leak on drm_setup() failure Currently we fail to free and detach the drm_file when drm_setup() fails. Use the drm_close_helper to do address that. Signed-off-by: Emil Velikov Reviewed-by: Daniel Vetter Link: https://patchwork.freedesktop.org/patch/msgid/20190114084410.15266-2-emil.l.velikov@gmail.com --- drivers/gpu/drm/drm_file.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'drivers/gpu/drm') diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c index e9607acfb629..21fa65b68a47 100644 --- a/drivers/gpu/drm/drm_file.c +++ b/drivers/gpu/drm/drm_file.c @@ -330,8 +330,10 @@ int drm_open(struct inode *inode, struct file *filp) goto err_undo; if (need_setup) { retcode = drm_setup(dev); - if (retcode) + if (retcode) { + drm_close_helper(filp); goto err_undo; + } } return 0; -- cgit v1.2.3 From 8059add0478e29cb641936011a8fcc9ce9fd80be Mon Sep 17 00:00:00 2001 From: Emil Velikov Date: Mon, 14 Jan 2019 08:54:08 +0000 Subject: drm: allow render capable master with DRM_AUTH ioctls There are cases (in mesa and applications) where one would open the primary node without properly authenticating the client. Sometimes we don't check if the authentication succeeds, but there's also cases we simply forget to do it. The former was a case for Mesa where it did not not check the return value of drmGetMagic() [1]. That was fixed recently although, there's the question of older drivers or other apps that exbibit this behaviour. While omitting the call results in issues as seen in [2] and [3]. In the libva case, libva itself doesn't authenticate the DRM client and the vaGetDisplayDRM documentation doesn't mention if the app should either. As of today, the official vainfo utility doesn't authenticate. To workaround issues like these, some users resort to running their apps under sudo. Which admittedly isn't always a good idea. Since any DRIVER_RENDER driver has sufficient isolation between clients, we can use that, for unauthenticated [primary node] ioctls that require DRM_AUTH. But only if the respective ioctl is tagged as DRM_RENDER_ALLOW. v2: - Rework/simplify if check (Daniel V) - Add examples to commit messages, elaborate. (Daniel V) v3: - Use single unlikely (Daniel V) [1] https://gitlab.freedesktop.org/mesa/mesa/blob/2bc1f5c2e70fe3b4d41f060af9859bc2a94c5b62/src/egl/drivers/dri2/platform_wayland.c#L1136 [2] https://lists.freedesktop.org/archives/libva/2016-July/004185.html [3] https://gitlab.freedesktop.org/mesa/kmscube/issues/1 Testcase: igt/core_unauth_vs_render Cc: intel-gfx@lists.freedesktop.org Signed-off-by: Emil Velikov Reviewed-by: Daniel Vetter Link: https://patchwork.freedesktop.org/patch/msgid/20190114085408.15933-2-emil.l.velikov@gmail.com --- drivers/gpu/drm/drm_ioctl.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) (limited to 'drivers/gpu/drm') diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index ab5692104ea0..687943df58e1 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -508,6 +508,13 @@ int drm_version(struct drm_device *dev, void *data, return err; } +static inline bool +drm_render_driver_and_ioctl(const struct drm_device *dev, u32 flags) +{ + return drm_core_check_feature(dev, DRIVER_RENDER) && + (flags & DRM_RENDER_ALLOW); +} + /** * drm_ioctl_permit - Check ioctl permissions against caller * @@ -522,14 +529,19 @@ int drm_version(struct drm_device *dev, void *data, */ int drm_ioctl_permit(u32 flags, struct drm_file *file_priv) { + const struct drm_device *dev = file_priv->minor->dev; + /* ROOT_ONLY is only for CAP_SYS_ADMIN */ if (unlikely((flags & DRM_ROOT_ONLY) && !capable(CAP_SYS_ADMIN))) return -EACCES; - /* AUTH is only for authenticated or render client */ - if (unlikely((flags & DRM_AUTH) && !drm_is_render_client(file_priv) && - !file_priv->authenticated)) - return -EACCES; + /* AUTH is only for master ... */ + if (unlikely((flags & DRM_AUTH) && drm_is_primary_client(file_priv))) { + /* authenticated ones, or render capable on DRM_RENDER_ALLOW. */ + if (!file_priv->authenticated && + !drm_render_driver_and_ioctl(dev, flags)) + return -EACCES; + } /* MASTER is only for master or control clients */ if (unlikely((flags & DRM_MASTER) && -- cgit v1.2.3 From 25dc194b34dd5919dd07b8873ee338182e15df9d Mon Sep 17 00:00:00 2001 From: Nicholas Kazlauskas Date: Mon, 7 Jan 2019 12:41:46 -0500 Subject: drm: Block fb changes for async plane updates The prepare_fb call always happens on new_plane_state. The drm_atomic_helper_cleanup_planes checks to see if plane state pointer has changed when deciding to call cleanup_fb on either the new_plane_state or the old_plane_state. For a non-async atomic commit the state pointer is swapped, so this helper calls prepare_fb on the new_plane_state and cleanup_fb on the old_plane_state. This makes sense, since we want to prepare the framebuffer we are going to use and cleanup the the framebuffer we are no longer using. For the async atomic update helpers this differs. The async atomic update helpers perform in-place updates on the existing state. They call drm_atomic_helper_cleanup_planes but the state pointer is not swapped. This means that prepare_fb is called on the new_plane_state and cleanup_fb is called on the new_plane_state (not the old). In the case where old_plane_state->fb == new_plane_state->fb then there should be no behavioral difference between an async update and a non-async commit. But there are issues that arise when old_plane_state->fb != new_plane_state->fb. The first is that the new_plane_state->fb is immediately cleaned up after it has been prepared, so we're using a fb that we shouldn't be. The second occurs during a sequence of async atomic updates and non-async regular atomic commits. Suppose there are two framebuffers being interleaved in a double-buffering scenario, fb1 and fb2: - Async update, oldfb = NULL, newfb = fb1, prepare fb1, cleanup fb1 - Async update, oldfb = fb1, newfb = fb2, prepare fb2, cleanup fb2 - Non-async commit, oldfb = fb2, newfb = fb1, prepare fb1, cleanup fb2 We call cleanup_fb on fb2 twice in this example scenario, and any further use will result in use-after-free. The simple fix to this problem is to block framebuffer changes in the drm_atomic_helper_async_check function for now. v2: Move check by itself, add a FIXME (Daniel) Cc: Daniel Vetter Cc: Harry Wentland Cc: Andrey Grodzovsky Cc: # v4.14+ Fixes: fef9df8b5945 ("drm/atomic: initial support for asynchronous plane update") Signed-off-by: Nicholas Kazlauskas Acked-by: Andrey Grodzovsky Acked-by: Harry Wentland Reviewed-by: Daniel Vetter Signed-off-by: Harry Wentland Link: https://patchwork.freedesktop.org/patch/275364/ --- drivers/gpu/drm/drm_atomic_helper.c | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'drivers/gpu/drm') diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 54e2ae614dcc..f4290f6b0c38 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1602,6 +1602,15 @@ int drm_atomic_helper_async_check(struct drm_device *dev, old_plane_state->crtc != new_plane_state->crtc) return -EINVAL; + /* + * FIXME: Since prepare_fb and cleanup_fb are always called on + * the new_plane_state for async updates we need to block framebuffer + * changes. This prevents use of a fb that's been cleaned up and + * double cleanups from occuring. + */ + if (old_plane_state->fb != new_plane_state->fb) + return -EINVAL; + funcs = plane->helper_private; if (!funcs->atomic_async_update) return -EINVAL; -- cgit v1.2.3 From 04b9c48851582aa0971b7d4e9d42c872b8acd0e2 Mon Sep 17 00:00:00 2001 From: Alistair Francis Date: Thu, 21 Feb 2019 00:33:03 +0000 Subject: drm/bochs: Fix the ID mismatch error When running RISC-V QEMU with the Bochs device attached via PCIe the probe of the Bochs device fails with: [drm:bochs_hw_init] *ERROR* ID mismatch This was introduced by this commit: 7780eb9ce8 bochs: convert to drm_dev_register To fix the error we ensure that pci_enable_device() is called before bochs_load(). Fixes: 7780eb9ce80f ("bochs: convert to drm_dev_register") Signed-off-by: Alistair Francis Reported-by: David Abdurachmanov Link: http://patchwork.freedesktop.org/patch/msgid/20190221003231.31625-1-alistair.francis@wdc.com Signed-off-by: Gerd Hoffmann --- drivers/gpu/drm/bochs/bochs_drv.c | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'drivers/gpu/drm') diff --git a/drivers/gpu/drm/bochs/bochs_drv.c b/drivers/gpu/drm/bochs/bochs_drv.c index f3dd66ae990a..aa35007262cd 100644 --- a/drivers/gpu/drm/bochs/bochs_drv.c +++ b/drivers/gpu/drm/bochs/bochs_drv.c @@ -154,6 +154,10 @@ static int bochs_pci_probe(struct pci_dev *pdev, if (IS_ERR(dev)) return PTR_ERR(dev); + ret = pci_enable_device(pdev); + if (ret) + goto err_free_dev; + dev->pdev = pdev; pci_set_drvdata(pdev, dev); -- cgit v1.2.3