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

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

[Rob Herring]

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),
 	DRM_IOCTL_DEF_DRV(VC4_GET_HANG_STATE, vc4_get_hang_state_ioctl,
 			  DRM_ROOT_ONLY),
 };
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm: vc4: enable XBGR8888 and ABGR8888 pixel formats

[Rob Herring]

DRM_FORMAT_XBGR8888 and DRM_FORMAT_ABGR8888 are 2 of the native formats
used in Android, so enable them for VC4. There seems to be no logic behind
HVS_PIXEL_ORDER_xxxx naming, but HVS_PIXEL_ORDER_ARGB seems to work
correctly.

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

diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index 4037b52..b12deef 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -94,6 +94,14 @@ static const struct hvs_format {
 		.pixel_order = HVS_PIXEL_ORDER_ABGR, .has_alpha = true,
 	},
 	{
+		.drm = DRM_FORMAT_ABGR8888, .hvs = HVS_PIXEL_FORMAT_RGBA8888,
+		.pixel_order = HVS_PIXEL_ORDER_ARGB, .has_alpha = true,
+	},
+	{
+		.drm = DRM_FORMAT_XBGR8888, .hvs = HVS_PIXEL_FORMAT_RGBA8888,
+		.pixel_order = HVS_PIXEL_ORDER_ARGB, .has_alpha = false,
+	},
+	{
 		.drm = DRM_FORMAT_RGB565, .hvs = HVS_PIXEL_FORMAT_RGB565,
 		.pixel_order = HVS_PIXEL_ORDER_XRGB, .has_alpha = false,
 	},
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm: vc4: enable XBGR8888 and ABGR8888 pixel formats

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 415 bytes --]

Rob Herring <robh@kernel.org> writes:

> DRM_FORMAT_XBGR8888 and DRM_FORMAT_ABGR8888 are 2 of the native formats
> used in Android, so enable them for VC4. There seems to be no logic behind
> HVS_PIXEL_ORDER_xxxx naming, but HVS_PIXEL_ORDER_ARGB seems to work
> correctly.

Spent a little bit digging into how the pixel orderings work and didn't
come up with an answer.  I'll try to look into this one again later.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm: vc4: enable XBGR8888 and ABGR8888 pixel formats

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 601 bytes --]

Eric Anholt <eric@anholt.net> writes:

> Rob Herring <robh@kernel.org> writes:
>
>> DRM_FORMAT_XBGR8888 and DRM_FORMAT_ABGR8888 are 2 of the native formats
>> used in Android, so enable them for VC4. There seems to be no logic behind
>> HVS_PIXEL_ORDER_xxxx naming, but HVS_PIXEL_ORDER_ARGB seems to work
>> correctly.
>
> Spent a little bit digging into how the pixel orderings work and didn't
> come up with an answer.  I'll try to look into this one again later.

Never came up with a good answer, but it's rendering correctly in
modetest, so I've reviewed and applied it to drm-vc4-next.

Thanks!

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Emil Velikov]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 356 bytes --]

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.


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Emil Velikov]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 1282 bytes --]

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!"

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Emil Velikov]

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.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 2052 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel