diff mbox series

[1/3] drm: change DROP_MASTER permissions to allow DRM_MASTER

Message ID 20181219192247.29880-2-emil.l.velikov@gmail.com (mailing list archive)
State New, archived
Headers show
Series drm: tweak permission handling | expand

Commit Message

Emil Velikov Dec. 19, 2018, 7:22 p.m. UTC 
From: Emil Velikov <emil.velikov@collabora.com>

Currently only DRM_ROOT_ONLY is allowed to call the ioctl.

Change that to DRM_MASTER, which means that only a process that is the
current DRM master can drop it. Which makes sense, the process should
be able to opt-out without any specific requirements.

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 drivers/gpu/drm/drm_ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Daniel Vetter Dec. 19, 2018, 8:36 p.m. UTC | #1 
On Wed, Dec 19, 2018 at 07:22:45PM +0000, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> Currently only DRM_ROOT_ONLY is allowed to call the ioctl.
> 
> Change that to DRM_MASTER, which means that only a process that is the
> current DRM master can drop it. Which makes sense, the process should
> be able to opt-out without any specific requirements.
> 
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>

I guess this makes sense, but then you already need someone else to do the
setmaster for you if you want to run as non-root and be able to switch
between compositors. So no idea where this will be useful.

Either way: New uapi -> needs the userspace patches to exist.
-Daniel

> ---
>  drivers/gpu/drm/drm_ioctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 94bd872d56c4..2221c8857fb0 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -578,7 +578,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
>  	DRM_IOCTL_DEF(DRM_IOCTL_GET_SAREA_CTX, drm_legacy_getsareactx, DRM_AUTH),
>  
>  	DRM_IOCTL_DEF(DRM_IOCTL_SET_MASTER, drm_setmaster_ioctl, DRM_UNLOCKED|DRM_ROOT_ONLY),
> -	DRM_IOCTL_DEF(DRM_IOCTL_DROP_MASTER, drm_dropmaster_ioctl, DRM_UNLOCKED|DRM_ROOT_ONLY),
> +	DRM_IOCTL_DEF(DRM_IOCTL_DROP_MASTER, drm_dropmaster_ioctl, DRM_UNLOCKED|DRM_MASTER),
>  
>  	DRM_IOCTL_DEF(DRM_IOCTL_ADD_CTX, drm_legacy_addctx, DRM_AUTH|DRM_ROOT_ONLY),
>  	DRM_IOCTL_DEF(DRM_IOCTL_RM_CTX, drm_legacy_rmctx, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
> -- 
> 2.19.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Emil Velikov Dec. 20, 2018, 1:50 p.m. UTC | #2 
On Wed, 19 Dec 2018 at 20:36, Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Wed, Dec 19, 2018 at 07:22:45PM +0000, Emil Velikov wrote:
> > From: Emil Velikov <emil.velikov@collabora.com>
> >
> > Currently only DRM_ROOT_ONLY is allowed to call the ioctl.
> >
> > Change that to DRM_MASTER, which means that only a process that is the
> > current DRM master can drop it. Which makes sense, the process should
> > be able to opt-out without any specific requirements.
> >
> > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>
> I guess this makes sense, but then you already need someone else to do the
> setmaster for you if you want to run as non-root and be able to switch
> between compositors. So no idea where this will be useful.
>
X, Weston and the Gnome/KDE wayland compositors use logind for managing that.
Some have codepaths to manage drm{Set,Drop}Master manually, although
they don't seems to bother adjusting privileges, I'd imagine due to VT
switching.

If ones has CONFIG_VT=n system, then it should be a matter of once-off
drmSetMaster + lower priv.

> Either way: New uapi -> needs the userspace patches to exist.

Slightly confused - apps already use the uapi, what do you mean with
"new uapi" here?
I'm OK with adding an IGT, although beyond that I'm not sure what
other userspace patches I could provide.

Thanks
Emil
Daniel Vetter Dec. 20, 2018, 2:45 p.m. UTC | #3 
On Thu, Dec 20, 2018 at 01:50:26PM +0000, Emil Velikov wrote:
> On Wed, 19 Dec 2018 at 20:36, Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Wed, Dec 19, 2018 at 07:22:45PM +0000, Emil Velikov wrote:
> > > From: Emil Velikov <emil.velikov@collabora.com>
> > >
> > > Currently only DRM_ROOT_ONLY is allowed to call the ioctl.
> > >
> > > Change that to DRM_MASTER, which means that only a process that is the
> > > current DRM master can drop it. Which makes sense, the process should
> > > be able to opt-out without any specific requirements.
> > >
> > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> >
> > I guess this makes sense, but then you already need someone else to do the
> > setmaster for you if you want to run as non-root and be able to switch
> > between compositors. So no idea where this will be useful.
> >
> X, Weston and the Gnome/KDE wayland compositors use logind for managing that.
> Some have codepaths to manage drm{Set,Drop}Master manually, although
> they don't seems to bother adjusting privileges, I'd imagine due to VT
> switching.
> 
> If ones has CONFIG_VT=n system, then it should be a matter of once-off
> drmSetMaster + lower priv.
> 
> > Either way: New uapi -> needs the userspace patches to exist.
> 
> Slightly confused - apps already use the uapi, what do you mean with
> "new uapi" here?
> I'm OK with adding an IGT, although beyond that I'm not sure what
> other userspace patches I could provide.

You change the uapi to allow more stuff (dropmaster without having
CAP_SYS_ADMIN), that needs userspace. Since current userspace has no use
for calling drop_master without being root.

Same way your patch to automatically auth clients if the driver supports
rendernodes is a uapi extension, and it's good to know what code exactly
it's meant for.

uapi is a lot more than include/uapi, it's anything the kernel does that
can influence userspace in a meaningful way.
-Daniel
Emil Velikov Dec. 20, 2018, 7:09 p.m. UTC | #4 
On Thu, 20 Dec 2018 at 14:45, Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Thu, Dec 20, 2018 at 01:50:26PM +0000, Emil Velikov wrote:
> > On Wed, 19 Dec 2018 at 20:36, Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Wed, Dec 19, 2018 at 07:22:45PM +0000, Emil Velikov wrote:
> > > > From: Emil Velikov <emil.velikov@collabora.com>
> > > >
> > > > Currently only DRM_ROOT_ONLY is allowed to call the ioctl.
> > > >
> > > > Change that to DRM_MASTER, which means that only a process that is the
> > > > current DRM master can drop it. Which makes sense, the process should
> > > > be able to opt-out without any specific requirements.
> > > >
> > > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> > >
> > > I guess this makes sense, but then you already need someone else to do the
> > > setmaster for you if you want to run as non-root and be able to switch
> > > between compositors. So no idea where this will be useful.
> > >
> > X, Weston and the Gnome/KDE wayland compositors use logind for managing that.
> > Some have codepaths to manage drm{Set,Drop}Master manually, although
> > they don't seems to bother adjusting privileges, I'd imagine due to VT
> > switching.
> >
> > If ones has CONFIG_VT=n system, then it should be a matter of once-off
> > drmSetMaster + lower priv.
> >
> > > Either way: New uapi -> needs the userspace patches to exist.
> >
> > Slightly confused - apps already use the uapi, what do you mean with
> > "new uapi" here?
> > I'm OK with adding an IGT, although beyond that I'm not sure what
> > other userspace patches I could provide.
>
> You change the uapi to allow more stuff (dropmaster without having
> CAP_SYS_ADMIN), that needs userspace. Since current userspace has no use
> for calling drop_master without being root.
>
> Same way your patch to automatically auth clients if the driver supports
> rendernodes is a uapi extension, and it's good to know what code exactly
> it's meant for.
>
Ack. Upon further testing I've spotted some issues, so this will go on
the back-burner.

Thanks
Emil
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 94bd872d56c4..2221c8857fb0 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -578,7 +578,7 @@  static const struct drm_ioctl_desc drm_ioctls[] = {
 	DRM_IOCTL_DEF(DRM_IOCTL_GET_SAREA_CTX, drm_legacy_getsareactx, DRM_AUTH),
 
 	DRM_IOCTL_DEF(DRM_IOCTL_SET_MASTER, drm_setmaster_ioctl, DRM_UNLOCKED|DRM_ROOT_ONLY),
-	DRM_IOCTL_DEF(DRM_IOCTL_DROP_MASTER, drm_dropmaster_ioctl, DRM_UNLOCKED|DRM_ROOT_ONLY),
+	DRM_IOCTL_DEF(DRM_IOCTL_DROP_MASTER, drm_dropmaster_ioctl, DRM_UNLOCKED|DRM_MASTER),
 
 	DRM_IOCTL_DEF(DRM_IOCTL_ADD_CTX, drm_legacy_addctx, DRM_AUTH|DRM_ROOT_ONLY),
 	DRM_IOCTL_DEF(DRM_IOCTL_RM_CTX, drm_legacy_rmctx, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),