| Message ID | 1465507165-16345-1-git-send-email-robh@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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), };
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(-)