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), };
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, },
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.
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!
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
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.
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
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!"
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.
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.
dri-devel@lists.freedesktop.org
-
Emil Velikov -
Eric Anholt -
Rob Herring