* [PATCH 0/3] drm: tweak permission handling
@ 2018-12-19 19:22 Emil Velikov
2018-12-19 19:22 ` [PATCH 1/3] drm: change DROP_MASTER permissions to allow DRM_MASTER Emil Velikov
` (3 more replies)
0 siblings, 4 replies; 16+ messages in thread
From: Emil Velikov @ 2018-12-19 19:22 UTC (permalink / raw)
To: dri-devel; +Cc: emil.l.velikov
Hi all,
This series relaxes some permission handling we have in core.
The first patch, swaps the DRM_ROOT_ONLY to DRM_MASTER on DROP_MASTER
ioctls. Thus any application can drop privileges just after SET_MASTER
and not worry about elevating them, solely for DROP_MASTER.
The last commit, admittedly works around userspace bugs. Although it's
far better than the "run as root" approach that people have been using.
It has the extra side effect of allowing some userspace (but not all)
to use vgem without any modifications ;-)
Would be great if this series is checked through the Intel GFX trybot
but I'm not sure how to do that.
Any comments, review or general ack's are appreciated.
Thanks
Emil
Emil Velikov (3):
drm: change DROP_MASTER permissions to allow DRM_MASTER
drm: annotate drm_core_check_feature() dev arg. as const
drm: allow render capable master with DRM_AUTH ioctls
drivers/gpu/drm/drm_ioctl.c | 10 +++++++---
include/drm/drm_drv.h | 2 +-
2 files changed, 8 insertions(+), 4 deletions(-)
--
2.19.2
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH 1/3] drm: change DROP_MASTER permissions to allow DRM_MASTER 2018-12-19 19:22 [PATCH 0/3] drm: tweak permission handling Emil Velikov @ 2018-12-19 19:22 ` Emil Velikov 2018-12-19 20:36 ` Daniel Vetter 2018-12-19 19:22 ` [PATCH 2/3] drm: annotate drm_core_check_feature() dev arg. as const Emil Velikov ` (2 subsequent siblings) 3 siblings, 1 reply; 16+ messages in thread From: Emil Velikov @ 2018-12-19 19:22 UTC (permalink / raw) To: dri-devel; +Cc: emil.l.velikov 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(-) 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] drm: change DROP_MASTER permissions to allow DRM_MASTER 2018-12-19 19:22 ` [PATCH 1/3] drm: change DROP_MASTER permissions to allow DRM_MASTER Emil Velikov @ 2018-12-19 20:36 ` Daniel Vetter 2018-12-20 13:50 ` Emil Velikov 0 siblings, 1 reply; 16+ messages in thread From: Daniel Vetter @ 2018-12-19 20:36 UTC (permalink / raw) To: Emil Velikov; +Cc: dri-devel 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] drm: change DROP_MASTER permissions to allow DRM_MASTER 2018-12-19 20:36 ` Daniel Vetter @ 2018-12-20 13:50 ` Emil Velikov 2018-12-20 14:45 ` Daniel Vetter 0 siblings, 1 reply; 16+ messages in thread From: Emil Velikov @ 2018-12-20 13:50 UTC (permalink / raw) To: Daniel Vetter; +Cc: ML dri-devel 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] drm: change DROP_MASTER permissions to allow DRM_MASTER 2018-12-20 13:50 ` Emil Velikov @ 2018-12-20 14:45 ` Daniel Vetter 2018-12-20 19:09 ` Emil Velikov 0 siblings, 1 reply; 16+ messages in thread From: Daniel Vetter @ 2018-12-20 14:45 UTC (permalink / raw) To: Emil Velikov; +Cc: ML dri-devel 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] drm: change DROP_MASTER permissions to allow DRM_MASTER 2018-12-20 14:45 ` Daniel Vetter @ 2018-12-20 19:09 ` Emil Velikov 0 siblings, 0 replies; 16+ messages in thread From: Emil Velikov @ 2018-12-20 19:09 UTC (permalink / raw) To: Daniel Vetter; +Cc: ML dri-devel 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/3] drm: annotate drm_core_check_feature() dev arg. as const 2018-12-19 19:22 [PATCH 0/3] drm: tweak permission handling Emil Velikov 2018-12-19 19:22 ` [PATCH 1/3] drm: change DROP_MASTER permissions to allow DRM_MASTER Emil Velikov @ 2018-12-19 19:22 ` Emil Velikov 2018-12-19 20:35 ` Daniel Vetter 2018-12-19 19:22 ` [PATCH 3/3] drm: allow render capable master with DRM_AUTH ioctls Emil Velikov 2018-12-19 20:30 ` [PATCH 0/3] drm: tweak permission handling Daniel Vetter 3 siblings, 1 reply; 16+ messages in thread From: Emil Velikov @ 2018-12-19 19:22 UTC (permalink / raw) To: dri-devel; +Cc: emil.l.velikov From: Emil Velikov <emil.velikov@collabora.com> This static inline function doesn't modify any state. Signed-off-by: Emil Velikov <emil.velikov@collabora.com> --- include/drm/drm_drv.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index 35af23f5fa0d..eca73330ffaf 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h @@ -666,7 +666,7 @@ static inline bool drm_dev_is_unplugged(struct drm_device *dev) * * Returns true if the @feature is supported, false otherwise. */ -static inline bool drm_core_check_feature(struct drm_device *dev, u32 feature) +static inline bool drm_core_check_feature(const struct drm_device *dev, u32 feature) { return dev->driver->driver_features & dev->driver_features & feature; } -- 2.19.2 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] drm: annotate drm_core_check_feature() dev arg. as const 2018-12-19 19:22 ` [PATCH 2/3] drm: annotate drm_core_check_feature() dev arg. as const Emil Velikov @ 2018-12-19 20:35 ` Daniel Vetter 0 siblings, 0 replies; 16+ messages in thread From: Daniel Vetter @ 2018-12-19 20:35 UTC (permalink / raw) To: Emil Velikov; +Cc: dri-devel On Wed, Dec 19, 2018 at 07:22:46PM +0000, Emil Velikov wrote: > From: Emil Velikov <emil.velikov@collabora.com> > > This static inline function doesn't modify any state. > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > include/drm/drm_drv.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h > index 35af23f5fa0d..eca73330ffaf 100644 > --- a/include/drm/drm_drv.h > +++ b/include/drm/drm_drv.h > @@ -666,7 +666,7 @@ static inline bool drm_dev_is_unplugged(struct drm_device *dev) > * > * Returns true if the @feature is supported, false otherwise. > */ > -static inline bool drm_core_check_feature(struct drm_device *dev, u32 feature) > +static inline bool drm_core_check_feature(const struct drm_device *dev, u32 feature) > { > return dev->driver->driver_features & dev->driver_features & feature; > } > -- > 2.19.2 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/3] drm: allow render capable master with DRM_AUTH ioctls 2018-12-19 19:22 [PATCH 0/3] drm: tweak permission handling Emil Velikov 2018-12-19 19:22 ` [PATCH 1/3] drm: change DROP_MASTER permissions to allow DRM_MASTER Emil Velikov 2018-12-19 19:22 ` [PATCH 2/3] drm: annotate drm_core_check_feature() dev arg. as const Emil Velikov @ 2018-12-19 19:22 ` Emil Velikov 2018-12-19 20:34 ` Daniel Vetter 2018-12-19 20:30 ` [PATCH 0/3] drm: tweak permission handling Daniel Vetter 3 siblings, 1 reply; 16+ messages in thread From: Emil Velikov @ 2018-12-19 19:22 UTC (permalink / raw) To: dri-devel; +Cc: emil.l.velikov From: Emil Velikov <emil.velikov@collabora.com> 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. Mesa has been fixed recently although, there's the question of older drivers or other apps that exbibit this behaviour. To workaround this, 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. As an added bonus this allows us to use vgem in userspace with zero change to some (but not all) existing programs. Signed-off-by: Emil Velikov <emil.velikov@collabora.com> --- drivers/gpu/drm/drm_ioctl.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 2221c8857fb0..4c775b775395 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -521,13 +521,17 @@ 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 */ + /* AUTH is only for authenticated/render capable master or render client */ if (unlikely((flags & DRM_AUTH) && !drm_is_render_client(file_priv) && - !file_priv->authenticated)) + !file_priv->authenticated && + !(drm_core_check_feature(dev, DRIVER_RENDER) && + (flags & DRM_RENDER_ALLOW)))) return -EACCES; /* MASTER is only for master or control clients */ -- 2.19.2 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] drm: allow render capable master with DRM_AUTH ioctls 2018-12-19 19:22 ` [PATCH 3/3] drm: allow render capable master with DRM_AUTH ioctls Emil Velikov @ 2018-12-19 20:34 ` Daniel Vetter 2018-12-20 15:16 ` Emil Velikov 0 siblings, 1 reply; 16+ messages in thread From: Daniel Vetter @ 2018-12-19 20:34 UTC (permalink / raw) To: Emil Velikov; +Cc: dri-devel On Wed, Dec 19, 2018 at 07:22:47PM +0000, Emil Velikov wrote: > From: Emil Velikov <emil.velikov@collabora.com> > > 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. Mesa has been fixed recently > although, there's the question of older drivers or other apps that > exbibit this behaviour. Would be good to have links to mesa where these bugs are fixed (or wherever those bugs where). > > To workaround this, 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. > > As an added bonus this allows us to use vgem in userspace with zero > change to some (but not all) existing programs. How/what/where? > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> > --- > drivers/gpu/drm/drm_ioctl.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index 2221c8857fb0..4c775b775395 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -521,13 +521,17 @@ 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 */ > + /* AUTH is only for authenticated/render capable master or render client */ > if (unlikely((flags & DRM_AUTH) && !drm_is_render_client(file_priv) && > - !file_priv->authenticated)) > + !file_priv->authenticated && > + !(drm_core_check_feature(dev, DRIVER_RENDER) && > + (flags & DRM_RENDER_ALLOW)))) Gets a bit unreadable but looks correct. With the commit message improved (since this is new uapi, so needs those pesky userspace links): Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > return -EACCES; > > /* MASTER is only for master or control clients */ > -- > 2.19.2 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] drm: allow render capable master with DRM_AUTH ioctls 2018-12-19 20:34 ` Daniel Vetter @ 2018-12-20 15:16 ` Emil Velikov 2018-12-20 15:34 ` Daniel Vetter 0 siblings, 1 reply; 16+ messages in thread From: Emil Velikov @ 2018-12-20 15:16 UTC (permalink / raw) To: Daniel Vetter; +Cc: ML dri-devel On Wed, 19 Dec 2018 at 20:34, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Wed, Dec 19, 2018 at 07:22:47PM +0000, Emil Velikov wrote: > > From: Emil Velikov <emil.velikov@collabora.com> > > > > 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. Mesa has been fixed recently > > although, there's the question of older drivers or other apps that > > exbibit this behaviour. > > Would be good to have links to mesa where these bugs are fixed (or > wherever those bugs where). > The error checking for drmGetMagic() was missing :-\ https://gitlab.freedesktop.org/mesa/mesa/blob/2bc1f5c2e70fe3b4d41f060af9859bc2a94c5b62/src/egl/drivers/dri2/platform_wayland.c#L1136 > > > > To workaround this, some users resort to running their apps under sudo. > > Which admittedly isn't always a good idea. > > One sudo example: https://lists.freedesktop.org/archives/libva/2016-July/004185.html Note: libva itself doesn't authenticate the DRM client and the vaGetDisplayDRM documentation doesn't mention if the app should either. Fairly neither one does as it - as in the example above. Typical output when auth handling is missing somewhere in the stack: https://gitlab.freedesktop.org/mesa/kmscube/issues/1 > > 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. > > > > As an added bonus this allows us to use vgem in userspace with zero > > change to some (but not all) existing programs. > > How/what/where? > Not quite sure what you mean here. Are you interested in what userspace will work unmodified with vgem, or something else? Userspace (such as some IGT vgem tests) that uses dumb buffers, fences, etc yet lacks drm authentication will work. Admittedly IGT is the first client (or ran as root), so it's implicitly authenticated - others may not. Admittedly, I did not search the webs for explicit vgem examples, although an educated assumption is that people will take the IGT code as norm. > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> > > --- > > drivers/gpu/drm/drm_ioctl.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > > index 2221c8857fb0..4c775b775395 100644 > > --- a/drivers/gpu/drm/drm_ioctl.c > > +++ b/drivers/gpu/drm/drm_ioctl.c > > @@ -521,13 +521,17 @@ 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 */ > > + /* AUTH is only for authenticated/render capable master or render client */ > > if (unlikely((flags & DRM_AUTH) && !drm_is_render_client(file_priv) && > > - !file_priv->authenticated)) > > + !file_priv->authenticated && > > + !(drm_core_check_feature(dev, DRIVER_RENDER) && > > + (flags & DRM_RENDER_ALLOW)))) > > Gets a bit unreadable but looks correct. > Agreed. If you prefer I could add an helper, say: +static inline bool +drm_is_render_driver_and_ioctl(...) +{ + return drm_core_check_feature(dev, DRIVER_RENDER) && (flags & DRM_RENDER_ALLOW)); +} [snip] - /* AUTH is only for authenticated or render client */ + /* AUTH is only for authenticated/render capable master or render client */ if (unlikely((flags & DRM_AUTH) && !drm_is_render_client(file_priv) && - !file_priv->authenticated)) + !file_priv->authenticated && !drm_is_render_driver_and_ioctl(...))) > With the commit message improved (since this is new uapi, so needs those > pesky userspace links): > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Smashing thank you. Please let me know if the above information and links are sufficient. -Emil _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] drm: allow render capable master with DRM_AUTH ioctls 2018-12-20 15:16 ` Emil Velikov @ 2018-12-20 15:34 ` Daniel Vetter 0 siblings, 0 replies; 16+ messages in thread From: Daniel Vetter @ 2018-12-20 15:34 UTC (permalink / raw) To: Emil Velikov; +Cc: ML dri-devel On Thu, Dec 20, 2018 at 03:16:15PM +0000, Emil Velikov wrote: > On Wed, 19 Dec 2018 at 20:34, Daniel Vetter <daniel@ffwll.ch> wrote: > > > > On Wed, Dec 19, 2018 at 07:22:47PM +0000, Emil Velikov wrote: > > > From: Emil Velikov <emil.velikov@collabora.com> > > > > > > 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. Mesa has been fixed recently > > > although, there's the question of older drivers or other apps that > > > exbibit this behaviour. > > > > Would be good to have links to mesa where these bugs are fixed (or > > wherever those bugs where). > > > The error checking for drmGetMagic() was missing :-\ > https://gitlab.freedesktop.org/mesa/mesa/blob/2bc1f5c2e70fe3b4d41f060af9859bc2a94c5b62/src/egl/drivers/dri2/platform_wayland.c#L1136 > > > > > > > To workaround this, some users resort to running their apps under sudo. > > > Which admittedly isn't always a good idea. > > > > One sudo example: > https://lists.freedesktop.org/archives/libva/2016-July/004185.html > > Note: libva itself doesn't authenticate the DRM client and the > vaGetDisplayDRM documentation doesn't mention if the app should > either. > Fairly neither one does as it - as in the example above. > > Typical output when auth handling is missing somewhere in the stack: > https://gitlab.freedesktop.org/mesa/kmscube/issues/1 Yup, the above is exactly what I was looking for. I think pasting the entire thing into the commit message is all we need. > > > > 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. > > > > > > As an added bonus this allows us to use vgem in userspace with zero > > > change to some (but not all) existing programs. > > > > How/what/where? > > > Not quite sure what you mean here. Are you interested in what > userspace will work unmodified with vgem, or something else? > > Userspace (such as some IGT vgem tests) that uses dumb buffers, > fences, etc yet lacks drm authentication will work. > Admittedly IGT is the first client (or ran as root), so it's > implicitly authenticated - others may not. igt isn't really a use-case, there's explicit checks that nothing else is running and that you're root. The tests don't work otherwise. Some might, but you can't make that assumption at all. > Admittedly, I did not search the webs for explicit vgem examples, > although an educated assumption is that people will take the IGT code > as norm. I think clearer to remove this "added bonus" from the commit message. > > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> > > > --- > > > drivers/gpu/drm/drm_ioctl.c | 8 ++++++-- > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > > > index 2221c8857fb0..4c775b775395 100644 > > > --- a/drivers/gpu/drm/drm_ioctl.c > > > +++ b/drivers/gpu/drm/drm_ioctl.c > > > @@ -521,13 +521,17 @@ 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 */ > > > + /* AUTH is only for authenticated/render capable master or render client */ > > > if (unlikely((flags & DRM_AUTH) && !drm_is_render_client(file_priv) && > > > - !file_priv->authenticated)) > > > + !file_priv->authenticated && > > > + !(drm_core_check_feature(dev, DRIVER_RENDER) && > > > + (flags & DRM_RENDER_ALLOW)))) > > > > Gets a bit unreadable but looks correct. > > > Agreed. If you prefer I could add an helper, say: > > +static inline bool > +drm_is_render_driver_and_ioctl(...) > +{ > + return drm_core_check_feature(dev, DRIVER_RENDER) && (flags & > DRM_RENDER_ALLOW)); > +} > > [snip] > > - /* AUTH is only for authenticated or render client */ > + /* AUTH is only for authenticated/render capable master or > render client */ > if (unlikely((flags & DRM_AUTH) && !drm_is_render_client(file_priv) && > - !file_priv->authenticated)) > + !file_priv->authenticated && > !drm_is_render_driver_and_ioctl(...))) Not sure that's really better, was thinking more of splitting this up into a set of separate if checks, e.g. if ((flags & DRM_AUTH) && drm_is_primary_client(file_priv)) { if (unlikely(!file_priv->authenticated)) return -EACCESS; if (unlikely(!drm_render_driver_and_ioctl(...)) return -EACCESS; } > > With the commit message improved (since this is new uapi, so needs those > > pesky userspace links): > > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > Smashing thank you. Please let me know if the above information and > links are sufficient. Ack. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] drm: tweak permission handling 2018-12-19 19:22 [PATCH 0/3] drm: tweak permission handling Emil Velikov ` (2 preceding siblings ...) 2018-12-19 19:22 ` [PATCH 3/3] drm: allow render capable master with DRM_AUTH ioctls Emil Velikov @ 2018-12-19 20:30 ` Daniel Vetter 2018-12-19 20:37 ` Daniel Vetter 3 siblings, 1 reply; 16+ messages in thread From: Daniel Vetter @ 2018-12-19 20:30 UTC (permalink / raw) To: Emil Velikov; +Cc: dri-devel On Wed, Dec 19, 2018 at 07:22:44PM +0000, Emil Velikov wrote: > Hi all, > > This series relaxes some permission handling we have in core. > > The first patch, swaps the DRM_ROOT_ONLY to DRM_MASTER on DROP_MASTER > ioctls. Thus any application can drop privileges just after SET_MASTER > and not worry about elevating them, solely for DROP_MASTER. > > The last commit, admittedly works around userspace bugs. Although it's > far better than the "run as root" approach that people have been using. > > It has the extra side effect of allowing some userspace (but not all) > to use vgem without any modifications ;-) > > Would be great if this series is checked through the Intel GFX trybot > but I'm not sure how to do that. Just cc intel-gfx@lists.freedesktop.org. -Daniel > > Any comments, review or general ack's are appreciated. > > Thanks > Emil > > Emil Velikov (3): > drm: change DROP_MASTER permissions to allow DRM_MASTER > drm: annotate drm_core_check_feature() dev arg. as const > drm: allow render capable master with DRM_AUTH ioctls > > drivers/gpu/drm/drm_ioctl.c | 10 +++++++--- > include/drm/drm_drv.h | 2 +- > 2 files changed, 8 insertions(+), 4 deletions(-) > > -- > 2.19.2 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] drm: tweak permission handling 2018-12-19 20:30 ` [PATCH 0/3] drm: tweak permission handling Daniel Vetter @ 2018-12-19 20:37 ` Daniel Vetter 2018-12-20 12:56 ` Emil Velikov 0 siblings, 1 reply; 16+ messages in thread From: Daniel Vetter @ 2018-12-19 20:37 UTC (permalink / raw) To: Emil Velikov; +Cc: dri-devel On Wed, Dec 19, 2018 at 09:30:46PM +0100, Daniel Vetter wrote: > On Wed, Dec 19, 2018 at 07:22:44PM +0000, Emil Velikov wrote: > > Hi all, > > > > This series relaxes some permission handling we have in core. > > > > The first patch, swaps the DRM_ROOT_ONLY to DRM_MASTER on DROP_MASTER > > ioctls. Thus any application can drop privileges just after SET_MASTER > > and not worry about elevating them, solely for DROP_MASTER. > > > > The last commit, admittedly works around userspace bugs. Although it's > > far better than the "run as root" approach that people have been using. > > > > It has the extra side effect of allowing some userspace (but not all) > > to use vgem without any modifications ;-) > > > > Would be great if this series is checked through the Intel GFX trybot > > but I'm not sure how to do that. > > Just cc intel-gfx@lists.freedesktop.org. Even better would be a few igts to exercise this stuff. We have some basic auth tests, but not much, so running this through the intel CI won't test much at all. -Daniel > -Daniel > > > > > Any comments, review or general ack's are appreciated. > > > > Thanks > > Emil > > > > Emil Velikov (3): > > drm: change DROP_MASTER permissions to allow DRM_MASTER > > drm: annotate drm_core_check_feature() dev arg. as const > > drm: allow render capable master with DRM_AUTH ioctls > > > > drivers/gpu/drm/drm_ioctl.c | 10 +++++++--- > > include/drm/drm_drv.h | 2 +- > > 2 files changed, 8 insertions(+), 4 deletions(-) > > > > -- > > 2.19.2 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] drm: tweak permission handling 2018-12-19 20:37 ` Daniel Vetter @ 2018-12-20 12:56 ` Emil Velikov 2018-12-20 14:43 ` Daniel Vetter 0 siblings, 1 reply; 16+ messages in thread From: Emil Velikov @ 2018-12-20 12:56 UTC (permalink / raw) To: Daniel Vetter; +Cc: ML dri-devel On Wed, 19 Dec 2018 at 20:37, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Wed, Dec 19, 2018 at 09:30:46PM +0100, Daniel Vetter wrote: > > On Wed, Dec 19, 2018 at 07:22:44PM +0000, Emil Velikov wrote: > > > Hi all, > > > > > > This series relaxes some permission handling we have in core. > > > > > > The first patch, swaps the DRM_ROOT_ONLY to DRM_MASTER on DROP_MASTER > > > ioctls. Thus any application can drop privileges just after SET_MASTER > > > and not worry about elevating them, solely for DROP_MASTER. > > > > > > The last commit, admittedly works around userspace bugs. Although it's > > > far better than the "run as root" approach that people have been using. > > > > > > It has the extra side effect of allowing some userspace (but not all) > > > to use vgem without any modifications ;-) > > > > > > Would be great if this series is checked through the Intel GFX trybot > > > but I'm not sure how to do that. > > > > Just cc intel-gfx@lists.freedesktop.org. Thanks will do. > > Even better would be a few igts to exercise this stuff. We have some basic > auth tests, but not much, so running this through the intel CI won't test > much at all. Right, I was thinking about adding something like the following: - open the primary node - /dev/dri/cardX - ensure it's not authenticated - by default the first client (or one run as root) is - issue a trivial ioctl that's annotated as DRM_AUTH - fail if the ioctl returns with -EACCESS Since IGT is usually the first client (or sometimes ran as root), I'm not quite sure how to achieve the second point. Any ideas are greatly appreciated. Thanks Emil _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] drm: tweak permission handling 2018-12-20 12:56 ` Emil Velikov @ 2018-12-20 14:43 ` Daniel Vetter 0 siblings, 0 replies; 16+ messages in thread From: Daniel Vetter @ 2018-12-20 14:43 UTC (permalink / raw) To: Emil Velikov; +Cc: ML dri-devel On Thu, Dec 20, 2018 at 12:56:46PM +0000, Emil Velikov wrote: > On Wed, 19 Dec 2018 at 20:37, Daniel Vetter <daniel@ffwll.ch> wrote: > > > > On Wed, Dec 19, 2018 at 09:30:46PM +0100, Daniel Vetter wrote: > > > On Wed, Dec 19, 2018 at 07:22:44PM +0000, Emil Velikov wrote: > > > > Hi all, > > > > > > > > This series relaxes some permission handling we have in core. > > > > > > > > The first patch, swaps the DRM_ROOT_ONLY to DRM_MASTER on DROP_MASTER > > > > ioctls. Thus any application can drop privileges just after SET_MASTER > > > > and not worry about elevating them, solely for DROP_MASTER. > > > > > > > > The last commit, admittedly works around userspace bugs. Although it's > > > > far better than the "run as root" approach that people have been using. > > > > > > > > It has the extra side effect of allowing some userspace (but not all) > > > > to use vgem without any modifications ;-) > > > > > > > > Would be great if this series is checked through the Intel GFX trybot > > > > but I'm not sure how to do that. > > > > > > Just cc intel-gfx@lists.freedesktop.org. > Thanks will do. > > > > > Even better would be a few igts to exercise this stuff. We have some basic > > auth tests, but not much, so running this through the intel CI won't test > > much at all. > > Right, I was thinking about adding something like the following: > - open the primary node - /dev/dri/cardX > - ensure it's not authenticated - by default the first client (or one > run as root) is > - issue a trivial ioctl that's annotated as DRM_AUTH > - fail if the ioctl returns with -EACCESS > > Since IGT is usually the first client (or sometimes ran as root), I'm > not quite sure how to achieve the second point. > Any ideas are greatly appreciated. Open fd a 2nd time, before closing the first one. For examples see the various core_* tests, specically core_auth. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2018-12-20 19:12 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-12-19 19:22 [PATCH 0/3] drm: tweak permission handling Emil Velikov 2018-12-19 19:22 ` [PATCH 1/3] drm: change DROP_MASTER permissions to allow DRM_MASTER Emil Velikov 2018-12-19 20:36 ` Daniel Vetter 2018-12-20 13:50 ` Emil Velikov 2018-12-20 14:45 ` Daniel Vetter 2018-12-20 19:09 ` Emil Velikov 2018-12-19 19:22 ` [PATCH 2/3] drm: annotate drm_core_check_feature() dev arg. as const Emil Velikov 2018-12-19 20:35 ` Daniel Vetter 2018-12-19 19:22 ` [PATCH 3/3] drm: allow render capable master with DRM_AUTH ioctls Emil Velikov 2018-12-19 20:34 ` Daniel Vetter 2018-12-20 15:16 ` Emil Velikov 2018-12-20 15:34 ` Daniel Vetter 2018-12-19 20:30 ` [PATCH 0/3] drm: tweak permission handling Daniel Vetter 2018-12-19 20:37 ` Daniel Vetter 2018-12-20 12:56 ` Emil Velikov 2018-12-20 14:43 ` Daniel Vetter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.