From: Emil Velikov emil.velikov@collabora.com
Move the render_client hunk for require_exist alongside the rest. Keeping all the reasons why an existing object is needed, in a single place makes it easier to follow.
Cc: VMware Graphics linux-graphics-maintainer@vmware.com Cc: Thomas Hellstrom thellstrom@vmware.com Signed-off-by: Emil Velikov emil.velikov@collabora.com --- drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c index 29d8794f0421..1f989f3605c8 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c @@ -909,16 +909,12 @@ vmw_surface_handle_reference(struct vmw_private *dev_priv, uint32_t handle; struct ttm_base_object *base; int ret; - bool require_exist = false;
if (handle_type == DRM_VMW_HANDLE_PRIME) { ret = ttm_prime_fd_to_handle(tfile, u_handle, &handle); if (unlikely(ret != 0)) return ret; } else { - if (unlikely(drm_is_render_client(file_priv))) - require_exist = true; - handle = u_handle; }
@@ -935,6 +931,8 @@ vmw_surface_handle_reference(struct vmw_private *dev_priv, }
if (handle_type != DRM_VMW_HANDLE_PRIME) { + bool require_exist = false; + user_srf = container_of(base, struct vmw_user_surface, prime.base);
@@ -946,6 +944,9 @@ vmw_surface_handle_reference(struct vmw_private *dev_priv, user_srf->master != file_priv->master) require_exist = true;
+ if (unlikely(drm_is_render_client(file_priv))) + require_exist = true; + ret = ttm_ref_object_add(tfile, base, TTM_REF_USAGE, NULL, require_exist); if (unlikely(ret != 0)) {
From: Emil Velikov emil.velikov@collabora.com
With later commit we'll rework DRM authentication handling. Namely DRM_AUTH will not be a requirement for DRM_RENDER_ALLOW ioctls.
Since vmwgfx does isolation for primary clients in different master realms, the DRM_AUTH can be dropped.
The only place where authentication matters, is surface_reference ioctls whenever a legacy (non-prime) handle is used. For those ioctls we call vmw_surface_handle_reference(), where we explicitly check if the client is both a) master and b) unauthenticated - bailing out as result.
Otherwise the usual isolation path kicks in and we're all good.
v2: Reword commit message, since the isolation work has landed.
Cc: VMware Graphics linux-graphics-maintainer@vmware.com Cc: Thomas Hellstrom thellstrom@vmware.com Signed-off-by: Emil Velikov emil.velikov@collabora.com --- drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c index 1f989f3605c8..596e5c1bc2c1 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c @@ -936,6 +936,13 @@ vmw_surface_handle_reference(struct vmw_private *dev_priv, user_srf = container_of(base, struct vmw_user_surface, prime.base);
+ /* Error out if we are unauthenticated master */ + if (drm_is_primary_client(file_priv) && + !file_priv->authenticated) { + ret = -EACCES; + goto out_bad_resource; + } + /* * Make sure the surface creator has the same * authenticating master, or is already registered with us.
On 11/1/19 2:05 PM, Emil Velikov wrote:
From: Emil Velikov emil.velikov@collabora.com
With later commit we'll rework DRM authentication handling. Namely DRM_AUTH will not be a requirement for DRM_RENDER_ALLOW ioctls.
Since vmwgfx does isolation for primary clients in different master realms, the DRM_AUTH can be dropped.
The only place where authentication matters, is surface_reference ioctls whenever a legacy (non-prime) handle is used. For those ioctls we call vmw_surface_handle_reference(), where we explicitly check if the client is both a) master and b) unauthenticated - bailing out as result.
Otherwise the usual isolation path kicks in and we're all good.
v2: Reword commit message, since the isolation work has landed.
Cc: VMware Graphics linux-graphics-maintainer@vmware.com Cc: Thomas Hellstrom thellstrom@vmware.com Signed-off-by: Emil Velikov emil.velikov@collabora.com
drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c index 1f989f3605c8..596e5c1bc2c1 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c @@ -936,6 +936,13 @@ vmw_surface_handle_reference(struct vmw_private *dev_priv, user_srf = container_of(base, struct vmw_user_surface, prime.base);
/* Error out if we are unauthenticated master */
Shouldn't this be "Error out if we are unauthenticated primary" ?
Otherwise
Reviewed-by: Thomas Hellstrom thellstrom@vmware.com
if (drm_is_primary_client(file_priv) &&!file_priv->authenticated) {ret = -EACCES;goto out_bad_resource;}- /*
- Make sure the surface creator has the same
- authenticating master, or is already registered with us.
From: Emil Velikov emil.velikov@collabora.com
With earlier commit 9c84aeba67cc ("drm/vmwgfx: Kill unneeded legacy security features") we removed the no longer applicable validation, as we now have isolation of primary clients from different master realms.
As of last commit, we're explicitly checking for authentication in the only render ioctls which care about one.
With those in place, the DRM_AUTH token serves no real purpose. Let's drop it.
Cc: VMware Graphics linux-graphics-maintainer@vmware.com Cc: Thomas Hellstrom thellstrom@vmware.com Signed-off-by: Emil Velikov emil.velikov@collabora.com --- drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c index 81a95651643f..253fae160175 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c @@ -165,9 +165,9 @@
static const struct drm_ioctl_desc vmw_ioctls[] = { VMW_IOCTL_DEF(VMW_GET_PARAM, vmw_getparam_ioctl, - DRM_AUTH | DRM_RENDER_ALLOW), + DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_ALLOC_DMABUF, vmw_bo_alloc_ioctl, - DRM_AUTH | DRM_RENDER_ALLOW), + DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_UNREF_DMABUF, vmw_bo_unref_ioctl, DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_CURSOR_BYPASS, @@ -182,16 +182,16 @@ static const struct drm_ioctl_desc vmw_ioctls[] = { DRM_MASTER),
VMW_IOCTL_DEF(VMW_CREATE_CONTEXT, vmw_context_define_ioctl, - DRM_AUTH | DRM_RENDER_ALLOW), + DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_UNREF_CONTEXT, vmw_context_destroy_ioctl, DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_CREATE_SURFACE, vmw_surface_define_ioctl, - DRM_AUTH | DRM_RENDER_ALLOW), + DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_UNREF_SURFACE, vmw_surface_destroy_ioctl, DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_REF_SURFACE, vmw_surface_reference_ioctl, - DRM_AUTH | DRM_RENDER_ALLOW), - VMW_IOCTL_DEF(VMW_EXECBUF, vmw_execbuf_ioctl, DRM_AUTH | + DRM_RENDER_ALLOW), + VMW_IOCTL_DEF(VMW_EXECBUF, vmw_execbuf_ioctl, DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_FENCE_WAIT, vmw_fence_obj_wait_ioctl, DRM_RENDER_ALLOW), @@ -201,9 +201,9 @@ static const struct drm_ioctl_desc vmw_ioctls[] = { VMW_IOCTL_DEF(VMW_FENCE_UNREF, vmw_fence_obj_unref_ioctl, DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_FENCE_EVENT, vmw_fence_event_ioctl, - DRM_AUTH | DRM_RENDER_ALLOW), + DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_GET_3D_CAP, vmw_get_cap_3d_ioctl, - DRM_AUTH | DRM_RENDER_ALLOW), + DRM_RENDER_ALLOW),
/* these allow direct access to the framebuffers mark as master only */ VMW_IOCTL_DEF(VMW_PRESENT, vmw_present_ioctl, @@ -221,28 +221,28 @@ static const struct drm_ioctl_desc vmw_ioctls[] = { DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_CREATE_SHADER, vmw_shader_define_ioctl, - DRM_AUTH | DRM_RENDER_ALLOW), + DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_UNREF_SHADER, vmw_shader_destroy_ioctl, DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_GB_SURFACE_CREATE, vmw_gb_surface_define_ioctl, - DRM_AUTH | DRM_RENDER_ALLOW), + DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_GB_SURFACE_REF, vmw_gb_surface_reference_ioctl, - DRM_AUTH | DRM_RENDER_ALLOW), + DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_SYNCCPU, vmw_user_bo_synccpu_ioctl, DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_CREATE_EXTENDED_CONTEXT, vmw_extended_context_define_ioctl, - DRM_AUTH | DRM_RENDER_ALLOW), + DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_GB_SURFACE_CREATE_EXT, vmw_gb_surface_define_ext_ioctl, - DRM_AUTH | DRM_RENDER_ALLOW), + DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_GB_SURFACE_REF_EXT, vmw_gb_surface_reference_ext_ioctl, - DRM_AUTH | DRM_RENDER_ALLOW), + DRM_RENDER_ALLOW), };
static const struct pci_device_id vmw_pci_id_list[] = {
On 11/1/19 2:05 PM, Emil Velikov wrote:
From: Emil Velikov emil.velikov@collabora.com
With earlier commit 9c84aeba67cc ("drm/vmwgfx: Kill unneeded legacy security features") we removed the no longer applicable validation, as we now have isolation of primary clients from different master realms.
As of last commit, we're explicitly checking for authentication in the only render ioctls which care about one.
With those in place, the DRM_AUTH token serves no real purpose. Let's drop it.
Cc: VMware Graphics linux-graphics-maintainer@vmware.com Cc: Thomas Hellstrom thellstrom@vmware.com Signed-off-by: Emil Velikov emil.velikov@collabora.com
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c index 81a95651643f..253fae160175 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c @@ -165,9 +165,9 @@
static const struct drm_ioctl_desc vmw_ioctls[] = { VMW_IOCTL_DEF(VMW_GET_PARAM, vmw_getparam_ioctl,
DRM_AUTH | DRM_RENDER_ALLOW),
DRM_RENDER_ALLOW),
DRM_AUTH | DRM_RENDER_ALLOW),
DRM_RENDER_ALLOW),@@ -182,16 +182,16 @@ static const struct drm_ioctl_desc vmw_ioctls[] = { DRM_MASTER),
VMW_IOCTL_DEF(VMW_CREATE_CONTEXT, vmw_context_define_ioctl,
DRM_AUTH | DRM_RENDER_ALLOW),
DRM_RENDER_ALLOW),
DRM_AUTH | DRM_RENDER_ALLOW),
DRM_RENDER_ALLOW),
DRM_AUTH | DRM_RENDER_ALLOW),- VMW_IOCTL_DEF(VMW_EXECBUF, vmw_execbuf_ioctl, DRM_AUTH |
DRM_RENDER_ALLOW),- VMW_IOCTL_DEF(VMW_EXECBUF, vmw_execbuf_ioctl, DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_FENCE_WAIT, vmw_fence_obj_wait_ioctl, DRM_RENDER_ALLOW),
@@ -201,9 +201,9 @@ static const struct drm_ioctl_desc vmw_ioctls[] = { VMW_IOCTL_DEF(VMW_FENCE_UNREF, vmw_fence_obj_unref_ioctl, DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_FENCE_EVENT, vmw_fence_event_ioctl,
DRM_AUTH | DRM_RENDER_ALLOW),
DRM_RENDER_ALLOW),
DRM_AUTH | DRM_RENDER_ALLOW),
DRM_RENDER_ALLOW),/* these allow direct access to the framebuffers mark as master only */ VMW_IOCTL_DEF(VMW_PRESENT, vmw_present_ioctl,
@@ -221,28 +221,28 @@ static const struct drm_ioctl_desc vmw_ioctls[] = { DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_CREATE_SHADER, vmw_shader_define_ioctl,
DRM_AUTH | DRM_RENDER_ALLOW),
DRM_RENDER_ALLOW),
DRM_AUTH | DRM_RENDER_ALLOW),
DRM_RENDER_ALLOW),
DRM_AUTH | DRM_RENDER_ALLOW),
DRM_RENDER_ALLOW),
DRM_AUTH | DRM_RENDER_ALLOW),
DRM_RENDER_ALLOW),
DRM_AUTH | DRM_RENDER_ALLOW),
DRM_RENDER_ALLOW),
DRM_AUTH | DRM_RENDER_ALLOW),
DRM_RENDER_ALLOW),};
static const struct pci_device_id vmw_pci_id_list[] = {
Reviewed-by: Thomas Hellstrom thellstrom@vmware.com
Do you want me to take this through the vmwgfx tree?
Thanks,
Thomas
On Tue, 12 Nov 2019 at 12:54, Thomas Hellstrom thellstrom@vmware.com wrote:
On 11/1/19 2:05 PM, Emil Velikov wrote:
From: Emil Velikov emil.velikov@collabora.com
With earlier commit 9c84aeba67cc ("drm/vmwgfx: Kill unneeded legacy security features") we removed the no longer applicable validation, as we now have isolation of primary clients from different master realms.
As of last commit, we're explicitly checking for authentication in the only render ioctls which care about one.
With those in place, the DRM_AUTH token serves no real purpose. Let's drop it.
Cc: VMware Graphics linux-graphics-maintainer@vmware.com Cc: Thomas Hellstrom thellstrom@vmware.com Signed-off-by: Emil Velikov emil.velikov@collabora.com
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c index 81a95651643f..253fae160175 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c @@ -165,9 +165,9 @@
static const struct drm_ioctl_desc vmw_ioctls[] = { VMW_IOCTL_DEF(VMW_GET_PARAM, vmw_getparam_ioctl,
DRM_AUTH | DRM_RENDER_ALLOW),
DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_ALLOC_DMABUF, vmw_bo_alloc_ioctl,
DRM_AUTH | DRM_RENDER_ALLOW),
DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_UNREF_DMABUF, vmw_bo_unref_ioctl, DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_CURSOR_BYPASS,@@ -182,16 +182,16 @@ static const struct drm_ioctl_desc vmw_ioctls[] = { DRM_MASTER),
VMW_IOCTL_DEF(VMW_CREATE_CONTEXT, vmw_context_define_ioctl,
DRM_AUTH | DRM_RENDER_ALLOW),
DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_UNREF_CONTEXT, vmw_context_destroy_ioctl, DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_CREATE_SURFACE, vmw_surface_define_ioctl,
DRM_AUTH | DRM_RENDER_ALLOW),
DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_UNREF_SURFACE, vmw_surface_destroy_ioctl, DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_REF_SURFACE, vmw_surface_reference_ioctl,
DRM_AUTH | DRM_RENDER_ALLOW),VMW_IOCTL_DEF(VMW_EXECBUF, vmw_execbuf_ioctl, DRM_AUTH |
DRM_RENDER_ALLOW),VMW_IOCTL_DEF(VMW_EXECBUF, vmw_execbuf_ioctl, DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_FENCE_WAIT, vmw_fence_obj_wait_ioctl, DRM_RENDER_ALLOW),@@ -201,9 +201,9 @@ static const struct drm_ioctl_desc vmw_ioctls[] = { VMW_IOCTL_DEF(VMW_FENCE_UNREF, vmw_fence_obj_unref_ioctl, DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_FENCE_EVENT, vmw_fence_event_ioctl,
DRM_AUTH | DRM_RENDER_ALLOW),
DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_GET_3D_CAP, vmw_get_cap_3d_ioctl,
DRM_AUTH | DRM_RENDER_ALLOW),
DRM_RENDER_ALLOW), /* these allow direct access to the framebuffers mark as master only */ VMW_IOCTL_DEF(VMW_PRESENT, vmw_present_ioctl,@@ -221,28 +221,28 @@ static const struct drm_ioctl_desc vmw_ioctls[] = { DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_CREATE_SHADER, vmw_shader_define_ioctl,
DRM_AUTH | DRM_RENDER_ALLOW),
DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_UNREF_SHADER, vmw_shader_destroy_ioctl, DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_GB_SURFACE_CREATE, vmw_gb_surface_define_ioctl,
DRM_AUTH | DRM_RENDER_ALLOW),
DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_GB_SURFACE_REF, vmw_gb_surface_reference_ioctl,
DRM_AUTH | DRM_RENDER_ALLOW),
DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_SYNCCPU, vmw_user_bo_synccpu_ioctl, DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_CREATE_EXTENDED_CONTEXT, vmw_extended_context_define_ioctl,
DRM_AUTH | DRM_RENDER_ALLOW),
DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_GB_SURFACE_CREATE_EXT, vmw_gb_surface_define_ext_ioctl,
DRM_AUTH | DRM_RENDER_ALLOW),
DRM_RENDER_ALLOW), VMW_IOCTL_DEF(VMW_GB_SURFACE_REF_EXT, vmw_gb_surface_reference_ext_ioctl,
DRM_AUTH | DRM_RENDER_ALLOW),
DRM_RENDER_ALLOW),};
static const struct pci_device_id vmw_pci_id_list[] = {
Reviewed-by: Thomas Hellstrom thellstrom@vmware.com
Great thanks.
Do you want me to take this through the vmwgfx tree?
Getting the patches through your tree sounds better. This way the VMware team can do extra testing, just in case :-) I'd imagine you will squash the typo in 2/5 when applying, correct?
May i interest you in reviewing the final patch of this series [1]?
Thanks Emil [1] https://patchwork.freedesktop.org/patch/338585/?series=68863&rev=1
From: Emil Velikov emil.velikov@collabora.com
As of earlier commit we have address space separation. Yet we forgot to remove the respective comment and DRM_AUTH in the ioctl declaration.
Cc: Tomeu Vizoso tomeu.vizoso@collabora.com Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: Robin Murphy robin.murphy@arm.com Cc: Steven Price steven.price@arm.com Fixes: 7282f7645d06 ("drm/panfrost: Implement per FD address spaces") Signed-off-by: Emil Velikov emil.velikov@collabora.com --- drivers/gpu/drm/panfrost/panfrost_drv.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c index bc2ddeb55f5d..c677b2c9e409 100644 --- a/drivers/gpu/drm/panfrost/panfrost_drv.c +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c @@ -451,15 +451,11 @@ panfrost_postclose(struct drm_device *dev, struct drm_file *file) kfree(panfrost_priv); }
-/* DRM_AUTH is required on SUBMIT for now, while all clients share a single - * address space. Note that render nodes would be able to submit jobs that - * could access BOs from clients authenticated with the master node. - */ static const struct drm_ioctl_desc panfrost_drm_driver_ioctls[] = { #define PANFROST_IOCTL(n, func, flags) \ DRM_IOCTL_DEF_DRV(PANFROST_##n, panfrost_ioctl_##func, flags)
- PANFROST_IOCTL(SUBMIT, submit, DRM_RENDER_ALLOW | DRM_AUTH), + PANFROST_IOCTL(SUBMIT, submit, DRM_RENDER_ALLOW), PANFROST_IOCTL(WAIT_BO, wait_bo, DRM_RENDER_ALLOW), PANFROST_IOCTL(CREATE_BO, create_bo, DRM_RENDER_ALLOW), PANFROST_IOCTL(MMAP_BO, mmap_bo, DRM_RENDER_ALLOW),
On 01/11/2019 13:03, Emil Velikov wrote:
From: Emil Velikov emil.velikov@collabora.com
As of earlier commit we have address space separation. Yet we forgot to remove the respective comment and DRM_AUTH in the ioctl declaration.
Cc: Tomeu Vizoso tomeu.vizoso@collabora.com Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: Robin Murphy robin.murphy@arm.com Cc: Steven Price steven.price@arm.com Fixes: 7282f7645d06 ("drm/panfrost: Implement per FD address spaces") Signed-off-by: Emil Velikov emil.velikov@collabora.com
Reviewed-by: Steven Price steven.price@arm.com
I'm not sure DRM_AUTH provided us with much in the first place (because render nodes could snoop/affect the primary node), but since we have address space separation it's clearly not required now.
Steve
drivers/gpu/drm/panfrost/panfrost_drv.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c index bc2ddeb55f5d..c677b2c9e409 100644 --- a/drivers/gpu/drm/panfrost/panfrost_drv.c +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c @@ -451,15 +451,11 @@ panfrost_postclose(struct drm_device *dev, struct drm_file *file) kfree(panfrost_priv); }
-/* DRM_AUTH is required on SUBMIT for now, while all clients share a single
- address space. Note that render nodes would be able to submit jobs that
- could access BOs from clients authenticated with the master node.
- */
static const struct drm_ioctl_desc panfrost_drm_driver_ioctls[] = { #define PANFROST_IOCTL(n, func, flags) \ DRM_IOCTL_DEF_DRV(PANFROST_##n, panfrost_ioctl_##func, flags)
- PANFROST_IOCTL(SUBMIT, submit, DRM_RENDER_ALLOW | DRM_AUTH),
- PANFROST_IOCTL(SUBMIT, submit, DRM_RENDER_ALLOW), PANFROST_IOCTL(WAIT_BO, wait_bo, DRM_RENDER_ALLOW), PANFROST_IOCTL(CREATE_BO, create_bo, DRM_RENDER_ALLOW), PANFROST_IOCTL(MMAP_BO, mmap_bo, DRM_RENDER_ALLOW),
On Fri, 1 Nov 2019 at 13:34, Steven Price steven.price@arm.com wrote:
On 01/11/2019 13:03, Emil Velikov wrote:
From: Emil Velikov emil.velikov@collabora.com
As of earlier commit we have address space separation. Yet we forgot to remove the respective comment and DRM_AUTH in the ioctl declaration.
Cc: Tomeu Vizoso tomeu.vizoso@collabora.com Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: Robin Murphy robin.murphy@arm.com Cc: Steven Price steven.price@arm.com Fixes: 7282f7645d06 ("drm/panfrost: Implement per FD address spaces") Signed-off-by: Emil Velikov emil.velikov@collabora.com
Reviewed-by: Steven Price steven.price@arm.com
I'm not sure DRM_AUTH provided us with much in the first place (because render nodes could snoop/affect the primary node), but since we have address space separation it's clearly not required now.
Thanks Steve. This is exactly the reason why I removed it from most other drivers. There are equivalent vmwgfx changes and a DRM core patch in this series.
Do you think you'll have some time to check those over? Would be amazing if I can apply the lot in one go to drm-misc.
Thanks Emil
On 08/11/2019 13:10, Emil Velikov wrote:
On Fri, 1 Nov 2019 at 13:34, Steven Price steven.price@arm.com wrote:
On 01/11/2019 13:03, Emil Velikov wrote:
From: Emil Velikov emil.velikov@collabora.com
As of earlier commit we have address space separation. Yet we forgot to remove the respective comment and DRM_AUTH in the ioctl declaration.
Cc: Tomeu Vizoso tomeu.vizoso@collabora.com Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: Robin Murphy robin.murphy@arm.com Cc: Steven Price steven.price@arm.com Fixes: 7282f7645d06 ("drm/panfrost: Implement per FD address spaces") Signed-off-by: Emil Velikov emil.velikov@collabora.com
Reviewed-by: Steven Price steven.price@arm.com
I'm not sure DRM_AUTH provided us with much in the first place (because render nodes could snoop/affect the primary node), but since we have address space separation it's clearly not required now.
Thanks Steve. This is exactly the reason why I removed it from most other drivers. There are equivalent vmwgfx changes and a DRM core patch in this series.
Do you think you'll have some time to check those over? Would be amazing if I can apply the lot in one go to drm-misc.
I'm afraid I don't know enough about the security model of vmwgfx to meaningfully comment on those changes. On the surface they look fine, but it really needs someone who understands whether this exposes an attack surface.
The DRM core patch concerns me slightly (although again I'm not completely up to speed on the security mode here). For a device which doesn't have address space separation (and doesn't support render nodes), is there anything stopping a process which hasn't authenticated converting another process's handle to a prime fd? (or injecting dmabufs into the address space used by the authenticated process - which might cause address space exhaustion). If that's not a concern then I'm not sure why the ioctls were originally added with DRM_AUTH...
Steve
On Fri, 8 Nov 2019 at 15:55, Steven Price steven.price@arm.com wrote:
On 08/11/2019 13:10, Emil Velikov wrote:
On Fri, 1 Nov 2019 at 13:34, Steven Price steven.price@arm.com wrote:
On 01/11/2019 13:03, Emil Velikov wrote:
From: Emil Velikov emil.velikov@collabora.com
As of earlier commit we have address space separation. Yet we forgot to remove the respective comment and DRM_AUTH in the ioctl declaration.
Cc: Tomeu Vizoso tomeu.vizoso@collabora.com Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: Robin Murphy robin.murphy@arm.com Cc: Steven Price steven.price@arm.com Fixes: 7282f7645d06 ("drm/panfrost: Implement per FD address spaces") Signed-off-by: Emil Velikov emil.velikov@collabora.com
Reviewed-by: Steven Price steven.price@arm.com
I'm not sure DRM_AUTH provided us with much in the first place (because render nodes could snoop/affect the primary node), but since we have address space separation it's clearly not required now.
Thanks Steve. This is exactly the reason why I removed it from most other drivers. There are equivalent vmwgfx changes and a DRM core patch in this series.
Do you think you'll have some time to check those over? Would be amazing if I can apply the lot in one go to drm-misc.
I'm afraid I don't know enough about the security model of vmwgfx to meaningfully comment on those changes. On the surface they look fine, but it really needs someone who understands whether this exposes an attack surface.
I understand, thanks for having a look.
The DRM core patch concerns me slightly (although again I'm not completely up to speed on the security mode here). For a device which doesn't have address space separation (and doesn't support render nodes), is there anything stopping a process which hasn't authenticated converting another process's handle to a prime fd? (or injecting dmabufs into the address space used by the authenticated process - which might cause address space exhaustion). If that's not a concern then I'm not sure why the ioctls were originally added with DRM_AUTH...
Thanks for raising this up.
Let's start with the short question: Why was DRM_AUTH added? I would expect either cargo-cult - we have DRM_AUTH even for get_param ioctl.
In order for a handle to be exported as fd, the driver must support render nodes. Which implicitly mandates address space separation. If that assumption is broken, then we have deeper problems. On the other hand, V3D does expose render nodes and uses the same quirk as panfrost before the address space separation patch.
So overall, it doesn't seem like it would cause any problems, which don't exist already.
Thanks Emil
From: Emil Velikov emil.velikov@collabora.com
As mentioned by Christian, for drivers which support only primary nodes this changes the returned error from -EACCES into -EOPNOTSUPP/-ENOSYS.
For others, this check in particular will be a noop. So let's remove it as suggested by Christian.
Cc: Alex Deucher alexander.deucher@amd.com Cc: amd-gfx@lists.freedesktop.org Cc: Daniel Vetter daniel@ffwll.ch Cc: Sean Paul sean@poorly.run Acked-by: Christian König christian.koenig@amd.com Signed-off-by: Emil Velikov emil.velikov@collabora.com --- drivers/gpu/drm/drm_ioctl.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index fcd728d7cf72..5afb39688b55 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -652,8 +652,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETRESOURCES, drm_mode_getresources, 0),
- DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, drm_prime_handle_to_fd_ioctl, DRM_AUTH|DRM_RENDER_ALLOW), - DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, drm_prime_fd_to_handle_ioctl, DRM_AUTH|DRM_RENDER_ALLOW), + DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, drm_prime_handle_to_fd_ioctl, DRM_RENDER_ALLOW), + DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, drm_prime_fd_to_handle_ioctl, DRM_RENDER_ALLOW),
DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPLANERESOURCES, drm_mode_getplane_res, 0), DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETCRTC, drm_mode_getcrtc, 0),
On Fri, 1 Nov 2019 at 13:05, Emil Velikov emil.l.velikov@gmail.com wrote:
From: Emil Velikov emil.velikov@collabora.com
As mentioned by Christian, for drivers which support only primary nodes this changes the returned error from -EACCES into -EOPNOTSUPP/-ENOSYS.
For others, this check in particular will be a noop. So let's remove it as suggested by Christian.
Cc: Alex Deucher alexander.deucher@amd.com Cc: amd-gfx@lists.freedesktop.org Cc: Daniel Vetter daniel@ffwll.ch Cc: Sean Paul sean@poorly.run Acked-by: Christian König christian.koenig@amd.com Signed-off-by: Emil Velikov emil.velikov@collabora.com
drivers/gpu/drm/drm_ioctl.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index fcd728d7cf72..5afb39688b55 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -652,8 +652,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETRESOURCES, drm_mode_getresources, 0),
DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, drm_prime_handle_to_fd_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, drm_prime_fd_to_handle_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, drm_prime_handle_to_fd_ioctl, DRM_RENDER_ALLOW),DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, drm_prime_fd_to_handle_ioctl, DRM_RENDER_ALLOW), DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPLANERESOURCES, drm_mode_getplane_res, 0), DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETCRTC, drm_mode_getcrtc, 0),--
Humble poke? Sean, others?
Thanks Emil
Hi Emil,
On Fri, 1 Nov 2019 13:03:13 +0000 Emil Velikov emil.l.velikov@gmail.com wrote:
From: Emil Velikov emil.velikov@collabora.com
As mentioned by Christian, for drivers which support only primary nodes this changes the returned error from -EACCES into -EOPNOTSUPP/-ENOSYS.
Are you sure this is true for MODESET-only nodes (those that do not have the RENDER cap set) implementing ->{fd_to_handle,handle_to_fd}()? Shouldn't the is_authenticated() check still be done in that case?
Regards,
Boris
For others, this check in particular will be a noop. So let's remove it as suggested by Christian.
Cc: Alex Deucher alexander.deucher@amd.com Cc: amd-gfx@lists.freedesktop.org Cc: Daniel Vetter daniel@ffwll.ch Cc: Sean Paul sean@poorly.run Acked-by: Christian König christian.koenig@amd.com Signed-off-by: Emil Velikov emil.velikov@collabora.com
drivers/gpu/drm/drm_ioctl.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index fcd728d7cf72..5afb39688b55 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -652,8 +652,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETRESOURCES, drm_mode_getresources, 0),
- DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, drm_prime_handle_to_fd_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
- DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, drm_prime_fd_to_handle_ioctl, DRM_AUTH|DRM_RENDER_ALLOW),
DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, drm_prime_handle_to_fd_ioctl, DRM_RENDER_ALLOW),
DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, drm_prime_fd_to_handle_ioctl, DRM_RENDER_ALLOW),
DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPLANERESOURCES, drm_mode_getplane_res, 0), DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETCRTC, drm_mode_getcrtc, 0),
On Wed, 27 Nov 2019 at 07:41, Boris Brezillon boris.brezillon@collabora.com wrote:
Hi Emil,
On Fri, 1 Nov 2019 13:03:13 +0000 Emil Velikov emil.l.velikov@gmail.com wrote:
From: Emil Velikov emil.velikov@collabora.com
As mentioned by Christian, for drivers which support only primary nodes this changes the returned error from -EACCES into -EOPNOTSUPP/-ENOSYS.
Are you sure this is true for MODESET-only nodes (those that do not have the RENDER cap set) implementing ->{fd_to_handle,handle_to_fd}()? Shouldn't the is_authenticated() check still be done in that case?
Thanks for catching this. Just sent out v2, which I should address the concern.
-Emil
On Wed, Nov 27, 2019 at 04:27:29PM +0000, Emil Velikov wrote:
On Wed, 27 Nov 2019 at 07:41, Boris Brezillon boris.brezillon@collabora.com wrote:
Hi Emil,
On Fri, 1 Nov 2019 13:03:13 +0000 Emil Velikov emil.l.velikov@gmail.com wrote:
From: Emil Velikov emil.velikov@collabora.com
As mentioned by Christian, for drivers which support only primary nodes this changes the returned error from -EACCES into -EOPNOTSUPP/-ENOSYS.
Are you sure this is true for MODESET-only nodes (those that do not have the RENDER cap set) implementing ->{fd_to_handle,handle_to_fd}()? Shouldn't the is_authenticated() check still be done in that case?
Thanks for catching this. Just sent out v2, which I should address the concern.
Why do we need this additional check in v2? What can go wrong on modeset drivers if non-authenticated legacy things can use this? modeset-only drivers have all their resources segregated by the drm core (drm_fb, mmaps, buffer lists), so there's really no access limitations that can go wrong here. -Daniel
On Wed, 27 Nov 2019 at 18:04, Daniel Vetter daniel@ffwll.ch wrote:
On Wed, Nov 27, 2019 at 04:27:29PM +0000, Emil Velikov wrote:
On Wed, 27 Nov 2019 at 07:41, Boris Brezillon boris.brezillon@collabora.com wrote:
Hi Emil,
On Fri, 1 Nov 2019 13:03:13 +0000 Emil Velikov emil.l.velikov@gmail.com wrote:
From: Emil Velikov emil.velikov@collabora.com
As mentioned by Christian, for drivers which support only primary nodes this changes the returned error from -EACCES into -EOPNOTSUPP/-ENOSYS.
Are you sure this is true for MODESET-only nodes (those that do not have the RENDER cap set) implementing ->{fd_to_handle,handle_to_fd}()? Shouldn't the is_authenticated() check still be done in that case?
Thanks for catching this. Just sent out v2, which I should address the concern.
Why do we need this additional check in v2? What can go wrong on modeset drivers if non-authenticated legacy things can use this? modeset-only drivers have all their resources segregated by the drm core (drm_fb, mmaps, buffer lists), so there's really no access limitations that can go wrong here.
Welcome back Daniel.
I haven't audited the core drm code, so wasn't sure if there's any issues that may arise. Hence the conservative approach in v2.
If you think this is fine as-is a formal Reviewed-by would be highly appreciated.
Thanks Emil
On Wed, Nov 27, 2019 at 06:32:56PM +0000, Emil Velikov wrote:
On Wed, 27 Nov 2019 at 18:04, Daniel Vetter daniel@ffwll.ch wrote:
On Wed, Nov 27, 2019 at 04:27:29PM +0000, Emil Velikov wrote:
On Wed, 27 Nov 2019 at 07:41, Boris Brezillon boris.brezillon@collabora.com wrote:
Hi Emil,
On Fri, 1 Nov 2019 13:03:13 +0000 Emil Velikov emil.l.velikov@gmail.com wrote:
From: Emil Velikov emil.velikov@collabora.com
As mentioned by Christian, for drivers which support only primary nodes this changes the returned error from -EACCES into -EOPNOTSUPP/-ENOSYS.
Are you sure this is true for MODESET-only nodes (those that do not have the RENDER cap set) implementing ->{fd_to_handle,handle_to_fd}()? Shouldn't the is_authenticated() check still be done in that case?
Thanks for catching this. Just sent out v2, which I should address the concern.
Why do we need this additional check in v2? What can go wrong on modeset drivers if non-authenticated legacy things can use this? modeset-only drivers have all their resources segregated by the drm core (drm_fb, mmaps, buffer lists), so there's really no access limitations that can go wrong here.
Welcome back Daniel.
I haven't audited the core drm code, so wasn't sure if there's any issues that may arise. Hence the conservative approach in v2.
If you think this is fine as-is a formal Reviewed-by would be highly appreciated.
I think there's a non-zero chance I'll have to eat a few hats on this, but I think v1 is solid.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
Thanks Emil
On Wed, 27 Nov 2019 at 18:37, Daniel Vetter daniel@ffwll.ch wrote:
On Wed, Nov 27, 2019 at 06:32:56PM +0000, Emil Velikov wrote:
On Wed, 27 Nov 2019 at 18:04, Daniel Vetter daniel@ffwll.ch wrote:
On Wed, Nov 27, 2019 at 04:27:29PM +0000, Emil Velikov wrote:
On Wed, 27 Nov 2019 at 07:41, Boris Brezillon boris.brezillon@collabora.com wrote:
Hi Emil,
On Fri, 1 Nov 2019 13:03:13 +0000 Emil Velikov emil.l.velikov@gmail.com wrote:
From: Emil Velikov emil.velikov@collabora.com
As mentioned by Christian, for drivers which support only primary nodes this changes the returned error from -EACCES into -EOPNOTSUPP/-ENOSYS.
Are you sure this is true for MODESET-only nodes (those that do not have the RENDER cap set) implementing ->{fd_to_handle,handle_to_fd}()? Shouldn't the is_authenticated() check still be done in that case?
Thanks for catching this. Just sent out v2, which I should address the concern.
Why do we need this additional check in v2? What can go wrong on modeset drivers if non-authenticated legacy things can use this? modeset-only drivers have all their resources segregated by the drm core (drm_fb, mmaps, buffer lists), so there's really no access limitations that can go wrong here.
Welcome back Daniel.
I haven't audited the core drm code, so wasn't sure if there's any issues that may arise. Hence the conservative approach in v2.
If you think this is fine as-is a formal Reviewed-by would be highly appreciated.
I think there's a non-zero chance I'll have to eat a few hats on this, but I think v1 is solid.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
Thanks. I've just re-read the DIM instructions and pushed this to drm-misc-next. Fingers crossed, I did not butcher it this time around.
-Emil
From: Emil Velikov emil.velikov@collabora.com
Current validation requires that we're authenticated, even though we can bypass (by design) the authentication when using a render node.
Let's address the former by following the design decision.
v2: Add simpler validation in the ioctls themselves (Boris)
Cc: Alex Deucher alexander.deucher@amd.com Cc: amd-gfx@lists.freedesktop.org Cc: Boris Brezillon boris.brezillon@collabora.com Cc: Daniel Vetter daniel@ffwll.ch Cc: Sean Paul sean@poorly.run Acked-by: Christian König christian.koenig@amd.com Signed-off-by: Emil Velikov emil.velikov@collabora.com --- drivers/gpu/drm/drm_ioctl.c | 4 ++-- drivers/gpu/drm/drm_prime.c | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index fcd728d7cf72..5afb39688b55 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -652,8 +652,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETRESOURCES, drm_mode_getresources, 0),
- DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, drm_prime_handle_to_fd_ioctl, DRM_AUTH|DRM_RENDER_ALLOW), - DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, drm_prime_fd_to_handle_ioctl, DRM_AUTH|DRM_RENDER_ALLOW), + DRM_IOCTL_DEF(DRM_IOCTL_PRIME_HANDLE_TO_FD, drm_prime_handle_to_fd_ioctl, DRM_RENDER_ALLOW), + DRM_IOCTL_DEF(DRM_IOCTL_PRIME_FD_TO_HANDLE, drm_prime_fd_to_handle_ioctl, DRM_RENDER_ALLOW),
DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPLANERESOURCES, drm_mode_getplane_res, 0), DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETCRTC, drm_mode_getcrtc, 0), diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 0a2316e0e812..dab166c860ec 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -358,11 +358,27 @@ int drm_gem_prime_fd_to_handle(struct drm_device *dev, } EXPORT_SYMBOL(drm_gem_prime_fd_to_handle);
+static inline bool +allowed_ioctl(struct drm_device *dev, struct drm_file *file_priv) +{ + /* Unauthenticated master is allowed, for render capable devices */ + if (drm_is_primary_client(file_priv)) { + if (!file_priv->authenticated && + !drm_core_check_feature(dev, DRIVER_RENDER)) + return false; + } + + return true; +} + int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_prime_handle *args = data;
+ if (!allowed_ioctl(dev, file_priv)) + return -EACCES; + if (!dev->driver->prime_fd_to_handle) return -ENOSYS;
@@ -511,6 +527,9 @@ int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data, { struct drm_prime_handle *args = data;
+ if (!allowed_ioctl(dev, file_priv)) + return -EACCES; + if (!dev->driver->prime_handle_to_fd) return -ENOSYS;
On Fri, 1 Nov 2019 at 13:05, Emil Velikov emil.l.velikov@gmail.com wrote:
From: Emil Velikov emil.velikov@collabora.com
Move the render_client hunk for require_exist alongside the rest. Keeping all the reasons why an existing object is needed, in a single place makes it easier to follow.
Cc: VMware Graphics linux-graphics-maintainer@vmware.com Cc: Thomas Hellstrom thellstrom@vmware.com Signed-off-by: Emil Velikov emil.velikov@collabora.com
drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c index 29d8794f0421..1f989f3605c8 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c @@ -909,16 +909,12 @@ vmw_surface_handle_reference(struct vmw_private *dev_priv, uint32_t handle; struct ttm_base_object *base; int ret;
bool require_exist = false; if (handle_type == DRM_VMW_HANDLE_PRIME) { ret = ttm_prime_fd_to_handle(tfile, u_handle, &handle); if (unlikely(ret != 0)) return ret; } else {if (unlikely(drm_is_render_client(file_priv)))require_exist = true;handle = u_handle; }@@ -935,6 +931,8 @@ vmw_surface_handle_reference(struct vmw_private *dev_priv, }
if (handle_type != DRM_VMW_HANDLE_PRIME) {
bool require_exist = false;user_srf = container_of(base, struct vmw_user_surface, prime.base);@@ -946,6 +944,9 @@ vmw_surface_handle_reference(struct vmw_private *dev_priv, user_srf->master != file_priv->master) require_exist = true;
if (unlikely(drm_is_render_client(file_priv)))require_exist = true;ret = ttm_ref_object_add(tfile, base, TTM_REF_USAGE, NULL, require_exist); if (unlikely(ret != 0)) {--
Thomas, VMware devs, humble poke? Any comments and review would be appreciated.
Thanks Emil
Hi, Emil!
On 11/8/19 2:14 PM, Emil Velikov wrote:
On Fri, 1 Nov 2019 at 13:05, Emil Velikov emil.l.velikov@gmail.com wrote:
From: Emil Velikov emil.velikov@collabora.com
Move the render_client hunk for require_exist alongside the rest. Keeping all the reasons why an existing object is needed, in a single place makes it easier to follow.
Cc: VMware Graphics linux-graphics-maintainer@vmware.com Cc: Thomas Hellstrom thellstrom@vmware.com Signed-off-by: Emil Velikov emil.velikov@collabora.com
drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c index 29d8794f0421..1f989f3605c8 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c @@ -909,16 +909,12 @@ vmw_surface_handle_reference(struct vmw_private *dev_priv, uint32_t handle; struct ttm_base_object *base; int ret;
bool require_exist = false; if (handle_type == DRM_VMW_HANDLE_PRIME) { ret = ttm_prime_fd_to_handle(tfile, u_handle, &handle); if (unlikely(ret != 0)) return ret; } else {if (unlikely(drm_is_render_client(file_priv)))require_exist = true;handle = u_handle; }@@ -935,6 +931,8 @@ vmw_surface_handle_reference(struct vmw_private *dev_priv, }
if (handle_type != DRM_VMW_HANDLE_PRIME) {
bool require_exist = false;user_srf = container_of(base, struct vmw_user_surface, prime.base);@@ -946,6 +944,9 @@ vmw_surface_handle_reference(struct vmw_private *dev_priv, user_srf->master != file_priv->master) require_exist = true;
if (unlikely(drm_is_render_client(file_priv)))require_exist = true;ret = ttm_ref_object_add(tfile, base, TTM_REF_USAGE, NULL, require_exist); if (unlikely(ret != 0)) {--
Thomas, VMware devs, humble poke? Any comments and review would be appreciated.
Thanks Emil
Sorry, I'll look at this early monday.
Thanks,
Thomas
On 11/1/19 2:05 PM, Emil Velikov wrote:
From: Emil Velikov emil.velikov@collabora.com
Move the render_client hunk for require_exist alongside the rest. Keeping all the reasons why an existing object is needed, in a single place makes it easier to follow.
Cc: VMware Graphics linux-graphics-maintainer@vmware.com Cc: Thomas Hellstrom thellstrom@vmware.com Signed-off-by: Emil Velikov emil.velikov@collabora.com
drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c index 29d8794f0421..1f989f3605c8 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c @@ -909,16 +909,12 @@ vmw_surface_handle_reference(struct vmw_private *dev_priv, uint32_t handle; struct ttm_base_object *base; int ret;
bool require_exist = false;
if (handle_type == DRM_VMW_HANDLE_PRIME) { ret = ttm_prime_fd_to_handle(tfile, u_handle, &handle); if (unlikely(ret != 0)) return ret; } else {
if (unlikely(drm_is_render_client(file_priv)))require_exist = true;handle = u_handle; }
@@ -935,6 +931,8 @@ vmw_surface_handle_reference(struct vmw_private *dev_priv, }
if (handle_type != DRM_VMW_HANDLE_PRIME) {
bool require_exist = false;- user_srf = container_of(base, struct vmw_user_surface, prime.base);
@@ -946,6 +944,9 @@ vmw_surface_handle_reference(struct vmw_private *dev_priv, user_srf->master != file_priv->master) require_exist = true;
if (unlikely(drm_is_render_client(file_priv)))require_exist = true;- ret = ttm_ref_object_add(tfile, base, TTM_REF_USAGE, NULL, require_exist); if (unlikely(ret != 0)) {
Reviewed-by: Thomas Hellstrom thellstrom@vmware.com
dri-devel@lists.freedesktop.org
-
Boris Brezillon -
Daniel Vetter -
Emil Velikov -
Steven Price -
Thomas Hellstrom