diff mbox

[1/2] drm: vc4: set permissions for ioctls

Message ID 1465507165-16345-1-git-send-email-robh@kernel.org (mailing list archive)
State New, archived
Headers show

Commit Message

Rob Herring (Arm) June 9, 2016, 9:19 p.m. UTC
Ioctls generally have DRM_AUTH and DRM_RENDER_ALLOW set to restrict them
to authorized clients and render nodes. Without this, access from render
nodes fails.

Signed-off-by: Rob Herring <robh@kernel.org>
---
 drivers/gpu/drm/vc4/vc4_drv.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

Comments

Emil Velikov June 9, 2016, 11:15 p.m. UTC | #1
Hi Rob,

On 9 June 2016 at 22:19, Rob Herring <robh@kernel.org> wrote:
> Ioctls generally have DRM_AUTH and DRM_RENDER_ALLOW set to restrict them
> to authorized clients and render nodes. Without this, access from render
> nodes fails.
>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
>  drivers/gpu/drm/vc4/vc4_drv.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
> index 3446ece..2077589 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.c
> +++ b/drivers/gpu/drm/vc4/vc4_drv.c
> @@ -66,12 +66,18 @@ static const struct file_operations vc4_drm_fops = {
>  };
>
>  static const struct drm_ioctl_desc vc4_drm_ioctls[] = {
> -       DRM_IOCTL_DEF_DRV(VC4_SUBMIT_CL, vc4_submit_cl_ioctl, 0),
> -       DRM_IOCTL_DEF_DRV(VC4_WAIT_SEQNO, vc4_wait_seqno_ioctl, 0),
> -       DRM_IOCTL_DEF_DRV(VC4_WAIT_BO, vc4_wait_bo_ioctl, 0),
> -       DRM_IOCTL_DEF_DRV(VC4_CREATE_BO, vc4_create_bo_ioctl, 0),
> -       DRM_IOCTL_DEF_DRV(VC4_MMAP_BO, vc4_mmap_bo_ioctl, 0),
> -       DRM_IOCTL_DEF_DRV(VC4_CREATE_SHADER_BO, vc4_create_shader_bo_ioctl, 0),
> +       DRM_IOCTL_DEF_DRV(VC4_SUBMIT_CL, vc4_submit_cl_ioctl,
> +                         DRM_AUTH | DRM_RENDER_ALLOW),
> +       DRM_IOCTL_DEF_DRV(VC4_WAIT_SEQNO, vc4_wait_seqno_ioctl,
> +                         DRM_AUTH | DRM_RENDER_ALLOW),
> +       DRM_IOCTL_DEF_DRV(VC4_WAIT_BO, vc4_wait_bo_ioctl,
> +                         DRM_AUTH | DRM_RENDER_ALLOW),
> +       DRM_IOCTL_DEF_DRV(VC4_CREATE_BO, vc4_create_bo_ioctl,
> +                         DRM_AUTH | DRM_RENDER_ALLOW),
> +       DRM_IOCTL_DEF_DRV(VC4_MMAP_BO, vc4_mmap_bo_ioctl,
> +                         DRM_AUTH | DRM_RENDER_ALLOW),
> +       DRM_IOCTL_DEF_DRV(VC4_CREATE_SHADER_BO, vc4_create_shader_bo_ioctl,
> +                         DRM_AUTH | DRM_RENDER_ALLOW),
I believe that the DRM_RENDER_ALLOW bits landed recently. Although the
DRM_AUTH are still missing afaict.

Regards,
Emil
Eric Anholt June 9, 2016, 11:42 p.m. UTC | #2
Rob Herring <robh@kernel.org> writes:

> Ioctls generally have DRM_AUTH and DRM_RENDER_ALLOW set to restrict them
> to authorized clients and render nodes. Without this, access from render
> nodes fails.

We've already got a fix to add RENDER_ALLOW submitted in the latest
drm-vc4-fixes.  There's no reason to require auth on this
implementation, though.
Emil Velikov June 9, 2016, 11:50 p.m. UTC | #3
On 10 June 2016 at 00:42, Eric Anholt <eric@anholt.net> wrote:
> Rob Herring <robh@kernel.org> writes:
>
>> Ioctls generally have DRM_AUTH and DRM_RENDER_ALLOW set to restrict them
>> to authorized clients and render nodes. Without this, access from render
>> nodes fails.
>
> We've already got a fix to add RENDER_ALLOW submitted in the latest
> drm-vc4-fixes.  There's no reason to require auth on this
> implementation, though.
>
Not 100% sure but I think you do. At least every other driver does...

Why: I'm thinking that without DRM_AUTH one will be able to open the
card# node and issue the said IOCTLs even if the client is not
authenticated. Which, obviously isn't a huge deal, but doesn't sound
right.

Then again, my knowledge of vc4 is virtually non-existent, so there
might be something special happening here ?

Regards,
Emil
Eric Anholt June 10, 2016, 8:08 p.m. UTC | #4
Emil Velikov <emil.l.velikov@gmail.com> writes:

> On 10 June 2016 at 00:42, Eric Anholt <eric@anholt.net> wrote:
>> Rob Herring <robh@kernel.org> writes:
>>
>>> Ioctls generally have DRM_AUTH and DRM_RENDER_ALLOW set to restrict them
>>> to authorized clients and render nodes. Without this, access from render
>>> nodes fails.
>>
>> We've already got a fix to add RENDER_ALLOW submitted in the latest
>> drm-vc4-fixes.  There's no reason to require auth on this
>> implementation, though.
>>
> Not 100% sure but I think you do. At least every other driver does...
>
> Why: I'm thinking that without DRM_AUTH one will be able to open the
> card# node and issue the said IOCTLs even if the client is not
> authenticated. Which, obviously isn't a huge deal, but doesn't sound
> right.
>
> Then again, my knowledge of vc4 is virtually non-existent, so there
> might be something special happening here ?

Let's flip this around: What is the problem you see with calling any of
the ioctls without having gone through the auth dance?  I don't believe
there's any reason to require auth, since you only have access to the
buffers you create or import.

Basically, auth was created a stopgap solution for "but if anyone had
access to the DRM device, they could scrape the X frontbuffer!"
Emil Velikov June 14, 2016, 12:31 p.m. UTC | #5
On 10 June 2016 at 21:08, Eric Anholt <eric@anholt.net> wrote:
> Emil Velikov <emil.l.velikov@gmail.com> writes:
>
>> On 10 June 2016 at 00:42, Eric Anholt <eric@anholt.net> wrote:
>>> Rob Herring <robh@kernel.org> writes:
>>>
>>>> Ioctls generally have DRM_AUTH and DRM_RENDER_ALLOW set to restrict them
>>>> to authorized clients and render nodes. Without this, access from render
>>>> nodes fails.
>>>
>>> We've already got a fix to add RENDER_ALLOW submitted in the latest
>>> drm-vc4-fixes.  There's no reason to require auth on this
>>> implementation, though.
>>>
>> Not 100% sure but I think you do. At least every other driver does...
>>
>> Why: I'm thinking that without DRM_AUTH one will be able to open the
>> card# node and issue the said IOCTLs even if the client is not
>> authenticated. Which, obviously isn't a huge deal, but doesn't sound
>> right.
>>
>> Then again, my knowledge of vc4 is virtually non-existent, so there
>> might be something special happening here ?
>
> Let's flip this around: What is the problem you see with calling any of
> the ioctls without having gone through the auth dance?  I don't believe
> there's any reason to require auth, since you only have access to the
> buffers you create or import.
>
> Basically, auth was created a stopgap solution for "but if anyone had
> access to the DRM device, they could scrape the X frontbuffer!"

Personally I don't see any serious issues* with keeping DRM_AUTH out
of these. Although one could argue that the lack of it up-to recently
one was using non-auth access to the card node. The latter of which
lead to the DRM_RENDER_ALLOW going unnoticed.

That aside, I would urge that we have consistency on the topic.
Whether adding DRM_AUTH to the said VC4 ioctls, dropping DRM_AUTH
everywhere (if DRM_RENDER_ALLOW is present on the said ioclt) or
something else.

I believe Daniel V was wondering about the second at some stage.

Regards,
Emil

* Barring buggy user-space tailored for this behaviour.
Eric Anholt June 14, 2016, 3:53 p.m. UTC | #6
Emil Velikov <emil.l.velikov@gmail.com> writes:

> On 10 June 2016 at 21:08, Eric Anholt <eric@anholt.net> wrote:
>> Emil Velikov <emil.l.velikov@gmail.com> writes:
>>
>>> On 10 June 2016 at 00:42, Eric Anholt <eric@anholt.net> wrote:
>>>> Rob Herring <robh@kernel.org> writes:
>>>>
>>>>> Ioctls generally have DRM_AUTH and DRM_RENDER_ALLOW set to restrict them
>>>>> to authorized clients and render nodes. Without this, access from render
>>>>> nodes fails.
>>>>
>>>> We've already got a fix to add RENDER_ALLOW submitted in the latest
>>>> drm-vc4-fixes.  There's no reason to require auth on this
>>>> implementation, though.
>>>>
>>> Not 100% sure but I think you do. At least every other driver does...
>>>
>>> Why: I'm thinking that without DRM_AUTH one will be able to open the
>>> card# node and issue the said IOCTLs even if the client is not
>>> authenticated. Which, obviously isn't a huge deal, but doesn't sound
>>> right.
>>>
>>> Then again, my knowledge of vc4 is virtually non-existent, so there
>>> might be something special happening here ?
>>
>> Let's flip this around: What is the problem you see with calling any of
>> the ioctls without having gone through the auth dance?  I don't believe
>> there's any reason to require auth, since you only have access to the
>> buffers you create or import.
>>
>> Basically, auth was created a stopgap solution for "but if anyone had
>> access to the DRM device, they could scrape the X frontbuffer!"
>
> Personally I don't see any serious issues* with keeping DRM_AUTH out
> of these. Although one could argue that the lack of it up-to recently
> one was using non-auth access to the card node. The latter of which
> lead to the DRM_RENDER_ALLOW going unnoticed.
>
> That aside, I would urge that we have consistency on the topic.
> Whether adding DRM_AUTH to the said VC4 ioctls, dropping DRM_AUTH
> everywhere (if DRM_RENDER_ALLOW is present on the said ioclt) or
> something else.

DRM_AUTH is not safe to remove from other drivers, unless they enforce
access control to their buffers.
diff mbox

Patch

diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index 3446ece..2077589 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -66,12 +66,18 @@  static const struct file_operations vc4_drm_fops = {
 };
 
 static const struct drm_ioctl_desc vc4_drm_ioctls[] = {
-	DRM_IOCTL_DEF_DRV(VC4_SUBMIT_CL, vc4_submit_cl_ioctl, 0),
-	DRM_IOCTL_DEF_DRV(VC4_WAIT_SEQNO, vc4_wait_seqno_ioctl, 0),
-	DRM_IOCTL_DEF_DRV(VC4_WAIT_BO, vc4_wait_bo_ioctl, 0),
-	DRM_IOCTL_DEF_DRV(VC4_CREATE_BO, vc4_create_bo_ioctl, 0),
-	DRM_IOCTL_DEF_DRV(VC4_MMAP_BO, vc4_mmap_bo_ioctl, 0),
-	DRM_IOCTL_DEF_DRV(VC4_CREATE_SHADER_BO, vc4_create_shader_bo_ioctl, 0),
+	DRM_IOCTL_DEF_DRV(VC4_SUBMIT_CL, vc4_submit_cl_ioctl,
+			  DRM_AUTH | DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF_DRV(VC4_WAIT_SEQNO, vc4_wait_seqno_ioctl,
+			  DRM_AUTH | DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF_DRV(VC4_WAIT_BO, vc4_wait_bo_ioctl,
+			  DRM_AUTH | DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF_DRV(VC4_CREATE_BO, vc4_create_bo_ioctl,
+			  DRM_AUTH | DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF_DRV(VC4_MMAP_BO, vc4_mmap_bo_ioctl,
+			  DRM_AUTH | DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF_DRV(VC4_CREATE_SHADER_BO, vc4_create_shader_bo_ioctl,
+			  DRM_AUTH | DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(VC4_GET_HANG_STATE, vc4_get_hang_state_ioctl,
 			  DRM_ROOT_ONLY),
 };