| Message ID | 20191101130313.8862-4-emil.l.velikov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] drm/vmwgfx: move the require_exist handling together | expand |
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
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),