| Message ID | 1377465159-8117-1-git-send-email-dh.herrmann@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Aug 25, 2013 at 11:12:39PM +0200, David Herrmann wrote: > DRM_IOCTL_MODE_GETFB is used to retrieve information about a given > framebuffer ID. It is a read-only helper and was thus declassified for > unprivileged access in: > > commit a14b1b42477c5ef089fcda88cbaae50d979eb8f9 > Author: Mandeep Singh Baines <mandeep.baines@gmail.com> > Date: Fri Jan 20 12:11:16 2012 -0800 > > drm: remove master fd restriction on mode setting getters > > However, beside width, height and stride information, > DRM_IOCTL_MODE_GETFB also passes back a handle to the underlying gem-bo of > the framebuffer. This handle allows users to mmap() it and read or write > into it. Obviously, this should be restricted to DRM-Master. > > With the current setup, *any* process with access to /dev/dri/card0 (which > means any process with access to hardware-accelerated rendering) can > access the current screen framebuffer and modify it ad libitum. > > For backwards-compatibility reasons we want to keep the > DRM_IOCTL_MODE_GETFB call unprivileged. Besides, it provides quite useful > information regarding screen setup. So we simply test whether the caller > is the current DRM-Master and if not, we return 0 as handle, which is > always invalid. A following DRM_IOCTL_GEM_CLOSE on this handle will fail > with EINVAL, but we accept this. Users shouldn't test for errors during > GEM_CLOSE, anyway. And it is still better than a failing MODE_GETFB call. Hmm, I do have userspace that takes advantage of this already for reads. So if you can make it a DRM_MASTER || cappable(SYS_ADMID) privileged operation instead, I can live with that. -Chris
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 54b4169..096e170 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -2598,10 +2598,22 @@ int drm_mode_getfb(struct drm_device *dev, r->depth = fb->depth; r->bpp = fb->bits_per_pixel; r->pitch = fb->pitches[0]; - if (fb->funcs->create_handle) - ret = fb->funcs->create_handle(fb, file_priv, &r->handle); - else + if (fb->funcs->create_handle) { + if (file_priv->is_master) { + ret = fb->funcs->create_handle(fb, file_priv, + &r->handle); + } else { + /* GET_FB() is an unprivileged ioctl so we must not + * return a buffer-handle to non-master processes! For + * backwards-compatibility reasons, we cannot make + * GET_FB() privileged, so just return an invalid handle + * for non-masters. */ + r->handle = 0; + ret = 0; + } + } else { ret = -ENODEV; + } drm_framebuffer_unreference(fb);
DRM_IOCTL_MODE_GETFB is used to retrieve information about a given framebuffer ID. It is a read-only helper and was thus declassified for unprivileged access in: commit a14b1b42477c5ef089fcda88cbaae50d979eb8f9 Author: Mandeep Singh Baines <mandeep.baines@gmail.com> Date: Fri Jan 20 12:11:16 2012 -0800 drm: remove master fd restriction on mode setting getters However, beside width, height and stride information, DRM_IOCTL_MODE_GETFB also passes back a handle to the underlying gem-bo of the framebuffer. This handle allows users to mmap() it and read or write into it. Obviously, this should be restricted to DRM-Master. With the current setup, *any* process with access to /dev/dri/card0 (which means any process with access to hardware-accelerated rendering) can access the current screen framebuffer and modify it ad libitum. For backwards-compatibility reasons we want to keep the DRM_IOCTL_MODE_GETFB call unprivileged. Besides, it provides quite useful information regarding screen setup. So we simply test whether the caller is the current DRM-Master and if not, we return 0 as handle, which is always invalid. A following DRM_IOCTL_GEM_CLOSE on this handle will fail with EINVAL, but we accept this. Users shouldn't test for errors during GEM_CLOSE, anyway. And it is still better than a failing MODE_GETFB call. Cc: <stable@vger.kernel.org> Signed-off-by: David Herrmann <dh.herrmann@gmail.com> --- drivers/gpu/drm/drm_crtc.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)