Discussion:
[PATCH] drm: remove va_drm_is_authenticated check
(too old to reply)
Emil Velikov
2016-11-15 15:24:15 UTC
Permalink
If we do not use a render node we must authenticate. Doing the extra
GetClient calls/ioctls does not help much, so don't bother.

Cc: David Herrmann <***@gmail.com>
Cc: Daniel Vetter <***@ffwll.ch>
Signed-off-by: Emil Velikov <***@gmail.com>
---
David, Daniel, I believe things are perfectly reasonable on kernel side.
If not please shout.
---
va/drm/va_drm.c | 8 ++------
va/drm/va_drm_auth.c | 35 -----------------------------------
va/drm/va_drm_auth.h | 4 ----
3 files changed, 2 insertions(+), 45 deletions(-)

diff --git a/va/drm/va_drm.c b/va/drm/va_drm.c
index 08071cf..59e33fa 100644
--- a/va/drm/va_drm.c
+++ b/va/drm/va_drm.c
@@ -74,12 +74,8 @@ va_DisplayContextGetDriverName(
if (ret < 0)
return VA_STATUS_ERROR_OPERATION_FAILED;

- if (!va_drm_is_authenticated(drm_state->fd)) {
- if (!va_drm_authenticate(drm_state->fd, magic))
- return VA_STATUS_ERROR_OPERATION_FAILED;
- if (!va_drm_is_authenticated(drm_state->fd))
- return VA_STATUS_ERROR_OPERATION_FAILED;
- }
+ if (!va_drm_authenticate(drm_state->fd, magic))
+ return VA_STATUS_ERROR_OPERATION_FAILED;
}

drm_state->auth_type = VA_DRM_AUTH_CUSTOM;
diff --git a/va/drm/va_drm_auth.c b/va/drm/va_drm_auth.c
index 53794d3..592381d 100644
--- a/va/drm/va_drm_auth.c
+++ b/va/drm/va_drm_auth.c
@@ -28,41 +28,6 @@
#include "va_drm_auth.h"
#include "va_drm_auth_x11.h"

-#if defined __linux__
-# include <sys/syscall.h>
-#endif
-
-/* Checks whether the thread id is the current thread */
-static bool
-is_local_tid(pid_t tid)
-{
-#if defined __linux__
- /* On Linux systems, drmGetClient() would return the thread ID
- instead of the actual process ID */
- return syscall(SYS_gettid) == tid;
-#else
- return false;
-#endif
-}
-
-/* Checks whether DRM connection is authenticated */
-bool
-va_drm_is_authenticated(int fd)
-{
- pid_t client_pid;
- int i, auth, pid, uid;
- unsigned long magic, iocs;
- bool is_authenticated = false;
-
- client_pid = getpid();
- for (i = 0; !is_authenticated; i++) {
- if (drmGetClient(fd, i, &auth, &pid, &uid, &magic, &iocs) != 0)
- break;
- is_authenticated = auth && (pid == client_pid || is_local_tid(pid));
- }
- return is_authenticated;
-}
-
/* Try to authenticate the DRM connection with the supplied magic id */
bool
va_drm_authenticate(int fd, uint32_t magic)
diff --git a/va/drm/va_drm_auth.h b/va/drm/va_drm_auth.h
index 1aa6989..a8ca794 100644
--- a/va/drm/va_drm_auth.h
+++ b/va/drm/va_drm_auth.h
@@ -30,10 +30,6 @@

DLL_HIDDEN
bool
-va_drm_is_authenticated(int fd);
-
-DLL_HIDDEN
-bool
va_drm_authenticate(int fd, uint32_t magic);

#endif /* VA_DRM_AUTH_H */
--
2.10.2
Emil Velikov
2016-11-15 17:44:59 UTC
Permalink
Hi
Post by Emil Velikov
If we do not use a render node we must authenticate. Doing the extra
GetClient calls/ioctls does not help much, so don't bother.
---
David, Daniel, I believe things are perfectly reasonable on kernel side.
If not please shout.
---
va/drm/va_drm.c | 8 ++------
va/drm/va_drm_auth.c | 35 -----------------------------------
va/drm/va_drm_auth.h | 4 ----
3 files changed, 2 insertions(+), 45 deletions(-)
diff --git a/va/drm/va_drm.c b/va/drm/va_drm.c
index 08071cf..59e33fa 100644
--- a/va/drm/va_drm.c
+++ b/va/drm/va_drm.c
@@ -74,12 +74,8 @@ va_DisplayContextGetDriverName(
if (ret < 0)
return VA_STATUS_ERROR_OPERATION_FAILED;
- if (!va_drm_is_authenticated(drm_state->fd)) {
- if (!va_drm_authenticate(drm_state->fd, magic))
- return VA_STATUS_ERROR_OPERATION_FAILED;
- if (!va_drm_is_authenticated(drm_state->fd))
- return VA_STATUS_ERROR_OPERATION_FAILED;
- }
+ if (!va_drm_authenticate(drm_state->fd, magic))
+ return VA_STATUS_ERROR_OPERATION_FAILED;
va_drm_authenticate() on native DRM returns EINVAL (via
drmAuthMagic()) if already authenticated. Hence, this solution only
authenticated.
I don't know the VA internals, so cannot see whether this matters.
The API (vaGetDisplayDRM) isn't explicit if the device has to be auth.
yet I'm leaning towards no. Not to mention that every user of vaapi
[that I know of] does not do auth.
On the libva side, it's done once during vaInitialize and only when
using the DRM display (vaGetDisplayDRM).

-Emil
Sean V Kelley
2016-11-17 06:14:04 UTC
Permalink
Post by Emil Velikov
Hi
Post by Emil Velikov
If we do not use a render node we must authenticate. Doing the extra
GetClient calls/ioctls does not help much, so don't bother.
---
David, Daniel, I believe things are perfectly reasonable on kernel side.
If not please shout.
---
va/drm/va_drm.c | 8 ++------
va/drm/va_drm_auth.c | 35 -----------------------------------
va/drm/va_drm_auth.h | 4 ----
3 files changed, 2 insertions(+), 45 deletions(-)
diff --git a/va/drm/va_drm.c b/va/drm/va_drm.c
index 08071cf..59e33fa 100644
--- a/va/drm/va_drm.c
+++ b/va/drm/va_drm.c
@@ -74,12 +74,8 @@ va_DisplayContextGetDriverName(
if (ret < 0)
return VA_STATUS_ERROR_OPERATION_FAILED;
- if (!va_drm_is_authenticated(drm_state->fd)) {
- if (!va_drm_authenticate(drm_state->fd, magic))
- return VA_STATUS_ERROR_OPERATION_FAILED;
- if (!va_drm_is_authenticated(drm_state->fd))
- return VA_STATUS_ERROR_OPERATION_FAILED;
- }
+ if (!va_drm_authenticate(drm_state->fd, magic))
+ return VA_STATUS_ERROR_OPERATION_FAILED;
va_drm_authenticate() on native DRM returns EINVAL (via
drmAuthMagic()) if already authenticated. Hence, this solution only
authenticated.
I don't know the VA internals, so cannot see whether this matters.
The API (vaGetDisplayDRM) isn't explicit if the device has to be auth.
yet I'm leaning towards no. Not to mention that every user of vaapi
[that I know of] does not do auth.
Indeed the intent has been to allow vaGetDisplayDRM() to accept DRM Render-Nodes bypassing authentication to facilitate headless or remote based modes, e.g., ssh remotely with no auth on system display. We really don’t do auth at all. Auth has always been a byproduct of what ever backend we needed to support, or work around, e.g., Android. I’m inclined to be okay with this patch.

Thanks,

Sean
Post by Emil Velikov
On the libva side, it's done once during vaInitialize and only when
using the DRM display (vaGetDisplayDRM).
-Emil
_______________________________________________
Libva mailing list
https://lists.freedesktop.org/mailman/listinfo/libva <https://lists.freedesktop.org/mailman/listinfo/libva>
Xiang, Haihao
2016-11-17 15:38:22 UTC
Permalink
Post by Emil Velikov
Velikov
Sent: Tuesday, November 15, 2016 11:24 PM
Subject: [Libva] [PATCH] drm: remove va_drm_is_authenticated check
If we do not use a render node we must authenticate.
It is not true. A root or master user can access /dev/dri/card0 without authentication in drm.
va_drm_is_authenticated() is used to check for this cases.
Post by Emil Velikov
Doing the extra
GetClient calls/ioctls does not help much, so don't bother.
---
David, Daniel, I believe things are perfectly reasonable on kernel side.
If not please shout.
---
va/drm/va_drm.c | 8 ++------
va/drm/va_drm_auth.c | 35 -----------------------------------
va/drm/va_drm_auth.h | 4 ----
3 files changed, 2 insertions(+), 45 deletions(-)
diff --git a/va/drm/va_drm.c b/va/drm/va_drm.c
index 08071cf..59e33fa 100644
--- a/va/drm/va_drm.c
+++ b/va/drm/va_drm.c
@@ -74,12 +74,8 @@ va_DisplayContextGetDriverName(
if (ret < 0)
return VA_STATUS_ERROR_OPERATION_FAILED;
- if (!va_drm_is_authenticated(drm_state->fd)) {
- if (!va_drm_authenticate(drm_state->fd, magic))
- return VA_STATUS_ERROR_OPERATION_FAILED;
- if (!va_drm_is_authenticated(drm_state->fd))
- return VA_STATUS_ERROR_OPERATION_FAILED;
- }
+ if (!va_drm_authenticate(drm_state->fd, magic))
+ return VA_STATUS_ERROR_OPERATION_FAILED;
}
drm_state->auth_type = VA_DRM_AUTH_CUSTOM;
diff --git a/va/drm/va_drm_auth.c b/va/drm/va_drm_auth.c
index 53794d3..592381d 100644
--- a/va/drm/va_drm_auth.c
+++ b/va/drm/va_drm_auth.c
@@ -28,41 +28,6 @@
#include "va_drm_auth.h"
#include "va_drm_auth_x11.h"
-#if defined __linux__
-# include <sys/syscall.h>
-#endif
-
-/* Checks whether the thread id is the current thread */
-static bool
-is_local_tid(pid_t tid)
-{
-#if defined __linux__
- /* On Linux systems, drmGetClient() would return the thread ID
- instead of the actual process ID */
- return syscall(SYS_gettid) == tid;
-#else
- return false;
-#endif
-}
-
-/* Checks whether DRM connection is authenticated */
-bool
-va_drm_is_authenticated(int fd)
-{
- pid_t client_pid;
- int i, auth, pid, uid;
- unsigned long magic, iocs;
- bool is_authenticated = false;
-
- client_pid = getpid();
- for (i = 0; !is_authenticated; i++) {
- if (drmGetClient(fd, i, &auth, &pid, &uid, &magic, &iocs) != 0)
- break;
- is_authenticated = auth && (pid == client_pid || is_local_tid(pid));
- }
- return is_authenticated;
-}
-
/* Try to authenticate the DRM connection with the supplied magic id */
bool
va_drm_authenticate(int fd, uint32_t magic)
diff --git a/va/drm/va_drm_auth.h b/va/drm/va_drm_auth.h
index 1aa6989..a8ca794 100644
--- a/va/drm/va_drm_auth.h
+++ b/va/drm/va_drm_auth.h
@@ -30,10 +30,6 @@
DLL_HIDDEN
bool
-va_drm_is_authenticated(int fd);
-
-DLL_HIDDEN
-bool
va_drm_authenticate(int fd, uint32_t magic);
#endif /* VA_DRM_AUTH_H */
--
2.10.2
_______________________________________________
Libva mailing list
https://lists.freedesktop.org/mailman/listinfo/libva
Sean V Kelley
2016-11-17 16:39:40 UTC
Permalink
Post by Emil Velikov
Velikov
Sent: Tuesday, November 15, 2016 11:24 PM
Subject: [Libva] [PATCH] drm: remove va_drm_is_authenticated check
If we do not use a render node we must authenticate. 
It is not true.  A root or master user can access /dev/dri/card0
without authentication in drm. 
va_drm_is_authenticated() is used to check for this cases.
That's true but that is also not a recommended use case, root user
accessing card0 for video playback/encode.

Sean
Post by Emil Velikov
Doing the extra
GetClient calls/ioctls does not help much, so don't bother.
---
David, Daniel, I believe things are perfectly reasonable on kernel side.
If not please shout.
---
va/drm/va_drm.c      |  8 ++------
va/drm/va_drm_auth.c | 35 -----------------------------------
va/drm/va_drm_auth.h |  4 ----
3 files changed, 2 insertions(+), 45 deletions(-)
diff --git a/va/drm/va_drm.c b/va/drm/va_drm.c
index 08071cf..59e33fa 100644
--- a/va/drm/va_drm.c
+++ b/va/drm/va_drm.c
@@ -74,12 +74,8 @@ va_DisplayContextGetDriverName(
        if (ret < 0)
            return VA_STATUS_ERROR_OPERATION_FAILED;
-        if (!va_drm_is_authenticated(drm_state->fd)) {
-            if (!va_drm_authenticate(drm_state->fd, magic))
-                return VA_STATUS_ERROR_OPERATION_FAILED;
-            if (!va_drm_is_authenticated(drm_state->fd))
-                return VA_STATUS_ERROR_OPERATION_FAILED;
-        }
+        if (!va_drm_authenticate(drm_state->fd, magic))
+            return VA_STATUS_ERROR_OPERATION_FAILED;
    }
    drm_state->auth_type = VA_DRM_AUTH_CUSTOM;
diff --git a/va/drm/va_drm_auth.c b/va/drm/va_drm_auth.c
index 53794d3..592381d 100644
--- a/va/drm/va_drm_auth.c
+++ b/va/drm/va_drm_auth.c
@@ -28,41 +28,6 @@
#include "va_drm_auth.h"
#include "va_drm_auth_x11.h"
-#if defined __linux__
-# include <sys/syscall.h>
-#endif
-
-/* Checks whether the thread id is the current thread */
-static bool
-is_local_tid(pid_t tid)
-{
-#if defined __linux__
-    /* On Linux systems, drmGetClient() would return the thread ID
-       instead of the actual process ID */
-    return syscall(SYS_gettid) == tid;
-#else
-    return false;
-#endif
-}
-
-/* Checks whether DRM connection is authenticated */
-bool
-va_drm_is_authenticated(int fd)
-{
-    pid_t client_pid;
-    int i, auth, pid, uid;
-    unsigned long magic, iocs;
-    bool is_authenticated = false;
-
-    client_pid = getpid();
-    for (i = 0; !is_authenticated; i++) {
-        if (drmGetClient(fd, i, &auth, &pid, &uid, &magic, &iocs)
!= 0)
-            break;
-        is_authenticated = auth && (pid == client_pid ||
is_local_tid(pid));
-    }
-    return is_authenticated;
-}
-
/* Try to authenticate the DRM connection with the supplied magic id */
bool
va_drm_authenticate(int fd, uint32_t magic)
diff --git a/va/drm/va_drm_auth.h b/va/drm/va_drm_auth.h
index 1aa6989..a8ca794 100644
--- a/va/drm/va_drm_auth.h
+++ b/va/drm/va_drm_auth.h
@@ -30,10 +30,6 @@
DLL_HIDDEN
bool
-va_drm_is_authenticated(int fd);
-
-DLL_HIDDEN
-bool
va_drm_authenticate(int fd, uint32_t magic);
#endif /* VA_DRM_AUTH_H */
--
2.10.2
_______________________________________________
Libva mailing list
https://lists.freedesktop.org/mailman/listinfo/libva
_______________________________________________
Libva mailing list
https://lists.freedesktop.org/mailman/listinfo/libva
Emil Velikov
2016-11-17 16:42:28 UTC
Permalink
Post by Xiang, Haihao
Post by Emil Velikov
Velikov
Sent: Tuesday, November 15, 2016 11:24 PM
Subject: [Libva] [PATCH] drm: remove va_drm_is_authenticated check
If we do not use a render node we must authenticate.
It is not true. A root or master user can access /dev/dri/card0 without authentication in drm.
va_drm_is_authenticated() is used to check for this cases.
Surely you're not suggesting that running your video player as root is
a wise idea ;-)

Regardless, there's something _really_ fishy with libva if it needs
va_drm_is_authenticated.
Any other software out there does not need to (or do so) - some
examples include Xorg, Wayland and other Weston compositors, Mesa, IGT
(a ton of GPU tests) ... be that as root, user or otherwise.

Emil
Sean V Kelley
2016-11-17 16:46:39 UTC
Permalink
Post by Emil Velikov
Post by Emil Velikov
Velikov
Sent: Tuesday, November 15, 2016 11:24 PM
Subject: [Libva] [PATCH] drm: remove va_drm_is_authenticated check
If we do not use a render node we must authenticate.
It is not true.  A root or master user can access /dev/dri/card0
without authentication in drm.
va_drm_is_authenticated() is used to check for this cases.
Surely you're not suggesting that running your video player as root is
a wise idea ;-)
Agreed.
Post by Emil Velikov
Regardless, there's something _really_ fishy with libva if it needs
va_drm_is_authenticated.
Any other software out there does not need to (or do so) - some
examples include Xorg, Wayland and other Weston compositors, Mesa, IGT
(a ton of GPU tests) ... be that as root, user or otherwise.
We don't need it, in my opinion.

Sean
Post by Emil Velikov
Emil
_______________________________________________
Libva mailing list
https://lists.freedesktop.org/mailman/listinfo/libva
Xiang, Haihao
2016-11-17 18:17:15 UTC
Permalink
-----Original Message-----
Sent: Friday, November 18, 2016 12:47 AM
Subject: Re: [Libva] [PATCH] drm: remove va_drm_is_authenticated check
Post by Emil Velikov
Of Emil Velikov
Sent: Tuesday, November 15, 2016 11:24 PM
Subject: [Libva] [PATCH] drm: remove va_drm_is_authenticated check
If we do not use a render node we must authenticate.
It is not true.  A root or master user can access /dev/dri/card0
without authentication in drm.
va_drm_is_authenticated() is used to check for this cases.
Surely you're not suggesting that running your video player as root is
a wise idea ;-)
Agreed.
I also agree we should run video player as a normal user, however some customer prefers root.
Post by Emil Velikov
Regardless, there's something _really_ fishy with libva if it needs
va_drm_is_authenticated.
Any other software out there does not need to (or do so) - some
examples include Xorg, Wayland and other Weston compositors, Mesa, IGT
(a ton of GPU tests) ... be that as root, user or otherwise.
We don't need it, in my opinion.
We can still run VAAPI application as root to access /dev/dri/card0 with this patch, but cannot
run 2+ VAAPI applications at the same time. We can suggest customer to use render node first because
authentication isn't needed for render node.

I am fine to apply the patch.
Sean
Post by Emil Velikov
Emil
_______________________________________________
Libva mailing list
https://lists.freedesktop.org/mailman/listinfo/libva
Sean V Kelley
2016-11-17 22:44:48 UTC
Permalink
Post by Xiang, Haihao
-----Original Message-----
Sent: Friday, November 18, 2016 12:47 AM
.org; David
Subject: Re: [Libva] [PATCH] drm: remove va_drm_is_authenticated check
Post by Emil Velikov
om>
Of Emil Velikov
Sent: Tuesday, November 15, 2016 11:24 PM
Subject: [Libva] [PATCH] drm: remove va_drm_is_authenticated check
If we do not use a render node we must authenticate.
It is not true.  A root or master user can access
/dev/dri/card0
without authentication in drm.
va_drm_is_authenticated() is used to check for this cases.
Surely you're not suggesting that running your video player as root is
a wise idea ;-)
Agreed.
I also agree we should run video player as a normal user, however
some customer prefers root.
Post by Emil Velikov
Regardless, there's something _really_ fishy with libva if it needs
va_drm_is_authenticated.
Any other software out there does not need to (or do so) - some
examples include Xorg, Wayland and other Weston compositors, Mesa, IGT
(a ton of GPU tests) ... be that as root, user or otherwise.
We don't need it, in my opinion.
We can still run VAAPI application as root to access /dev/dri/card0
with this patch, but cannot
run 2+ VAAPI applications at the same time.  We can suggest customer
to use render node first because
authentication isn't needed for render node.
I am fine to apply the patch.
Good. I'm glad we are aligned.

Thanks,

Sean
Post by Xiang, Haihao
Sean
Post by Emil Velikov
Emil
_______________________________________________
Libva mailing list
https://lists.freedesktop.org/mailman/listinfo/libva
Sean V Kelley
2016-11-17 22:46:14 UTC
Permalink
Post by Emil Velikov
If we do not use a render node we must authenticate. Doing the extra
GetClient calls/ioctls does not help much, so don't bother.
After some discussion in this thread, lgtm, applied.

Thanks,

Sean
Post by Emil Velikov
---
David, Daniel, I believe things are perfectly reasonable on kernel side.
If not please shout.
---
 va/drm/va_drm.c      |  8 ++------
 va/drm/va_drm_auth.c | 35 -----------------------------------
 va/drm/va_drm_auth.h |  4 ----
 3 files changed, 2 insertions(+), 45 deletions(-)
diff --git a/va/drm/va_drm.c b/va/drm/va_drm.c
index 08071cf..59e33fa 100644
--- a/va/drm/va_drm.c
+++ b/va/drm/va_drm.c
@@ -74,12 +74,8 @@ va_DisplayContextGetDriverName(
         if (ret < 0)
             return VA_STATUS_ERROR_OPERATION_FAILED;
 
-        if (!va_drm_is_authenticated(drm_state->fd)) {
-            if (!va_drm_authenticate(drm_state->fd, magic))
-                return VA_STATUS_ERROR_OPERATION_FAILED;
-            if (!va_drm_is_authenticated(drm_state->fd))
-                return VA_STATUS_ERROR_OPERATION_FAILED;
-        }
+        if (!va_drm_authenticate(drm_state->fd, magic))
+            return VA_STATUS_ERROR_OPERATION_FAILED;
     }
 
     drm_state->auth_type = VA_DRM_AUTH_CUSTOM;
diff --git a/va/drm/va_drm_auth.c b/va/drm/va_drm_auth.c
index 53794d3..592381d 100644
--- a/va/drm/va_drm_auth.c
+++ b/va/drm/va_drm_auth.c
@@ -28,41 +28,6 @@
 #include "va_drm_auth.h"
 #include "va_drm_auth_x11.h"
 
-#if defined __linux__
-# include <sys/syscall.h>
-#endif
-
-/* Checks whether the thread id is the current thread */
-static bool
-is_local_tid(pid_t tid)
-{
-#if defined __linux__
-    /* On Linux systems, drmGetClient() would return the thread ID
-       instead of the actual process ID */
-    return syscall(SYS_gettid) == tid;
-#else
-    return false;
-#endif
-}
-
-/* Checks whether DRM connection is authenticated */
-bool
-va_drm_is_authenticated(int fd)
-{
-    pid_t client_pid;
-    int i, auth, pid, uid;
-    unsigned long magic, iocs;
-    bool is_authenticated = false;
-
-    client_pid = getpid();
-    for (i = 0; !is_authenticated; i++) {
-        if (drmGetClient(fd, i, &auth, &pid, &uid, &magic, &iocs) !=
0)
-            break;
-        is_authenticated = auth && (pid == client_pid ||
is_local_tid(pid));
-    }
-    return is_authenticated;
-}
-
 /* Try to authenticate the DRM connection with the supplied magic id
*/
 bool
 va_drm_authenticate(int fd, uint32_t magic)
diff --git a/va/drm/va_drm_auth.h b/va/drm/va_drm_auth.h
index 1aa6989..a8ca794 100644
--- a/va/drm/va_drm_auth.h
+++ b/va/drm/va_drm_auth.h
@@ -30,10 +30,6 @@
 
 DLL_HIDDEN
 bool
-va_drm_is_authenticated(int fd);
-
-DLL_HIDDEN
-bool
 va_drm_authenticate(int fd, uint32_t magic);
 
 #endif /* VA_DRM_AUTH_H */
Loading...