[PATCH 0/3] drm: tweak permission handling

[PATCH 0/3] drm: tweak permission handling

[Emil 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

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

[Emil 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

[PATCH 2/3] drm: annotate drm_core_check_feature() dev arg. as const

[Emil 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

[PATCH 3/3] drm: allow render capable master with DRM_AUTH ioctls

[Emil 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

Re: [PATCH 0/3] drm: tweak permission handling

[Daniel Vetter]

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

Re: [PATCH 3/3] drm: allow render capable master with DRM_AUTH ioctls

[Daniel Vetter]

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

Re: [PATCH 2/3] drm: annotate drm_core_check_feature() dev arg. as const

[Daniel Vetter]

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

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

[Daniel Vetter]

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

Re: [PATCH 0/3] drm: tweak permission handling

[Daniel Vetter]

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

Re: [PATCH 0/3] drm: tweak permission handling

[Emil Velikov]

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

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

[Emil Velikov]

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

Re: [PATCH 0/3] drm: tweak permission handling

[Daniel Vetter]

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

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

[Daniel Vetter]

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

Re: [PATCH 3/3] drm: allow render capable master with DRM_AUTH ioctls

[Emil Velikov]

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

Re: [PATCH 3/3] drm: allow render capable master with DRM_AUTH ioctls

[Daniel Vetter]

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

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

[Emil Velikov]

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

[PATCH 3/3] drm: allow render capable master with DRM_AUTH ioctls

[Emil 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.

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)

v4:
- Reapply patch, use DRIVER_FORCE_AUTH w/a for amdgpu/radeon.

[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
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_ioctl.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 09f7f8e33fa3..c30feb5e97e3 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -517,6 +517,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
  *
@@ -531,6 +538,8 @@ 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;
@@ -538,7 +547,14 @@ int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
 	/* AUTH is only for authenticated or render client */
 	if (unlikely((flags & DRM_AUTH) && !drm_is_render_client(file_priv) &&
 		     !file_priv->authenticated))
-		return -EACCES;
+		/*
+		 * Although we allow:
+		 *  - render drivers with DRM_RENDER_ALLOW ioctls, when
+		 *  - drivers do not explicitly mandate authentication.
+		 */
+		if (!drm_render_driver_and_ioctl(dev, flags) ||
+		    drm_core_check_feature(dev, DRIVER_FORCE_AUTH))
+			return -EACCES;
 
 	/* MASTER is only for master or control clients */
 	if (unlikely((flags & DRM_MASTER) &&
-- 
2.21.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx