diff mbox

[v2,1/2] drm: Make HW_LOCK access functions optional.

Message ID 1431500088-15278-2-git-send-email-peter.antoine@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Peter Antoine May 13, 2015, 6:54 a.m. UTC 
As these functions are only used by one driver and there are security holes
in these functions. Make the functions optional.

Issue: VIZ-5485
Signed-off-by: Peter Antoine <peter.antoine@intel.com>
---
 drivers/gpu/drm/drm_lock.c            |  6 ++++++
 drivers/gpu/drm/nouveau/nouveau_drm.c |  3 ++-
 include/drm/drmP.h                    | 23 ++++++++++++-----------
 3 files changed, 20 insertions(+), 12 deletions(-)

Comments

Daniel Vetter May 13, 2015, 7:14 a.m. UTC | #1 
On Wed, May 13, 2015 at 07:54:47AM +0100, Peter Antoine wrote:
> As these functions are only used by one driver and there are security holes
> in these functions. Make the functions optional.

Is there a reference for why nouveau needs hw locks too? Also have you
done an audit of mesa history and X history to make sure there's no other
driver accidentally using it with a modern kms driver?

> Issue: VIZ-5485
> Signed-off-by: Peter Antoine <peter.antoine@intel.com>
> ---
>  drivers/gpu/drm/drm_lock.c            |  6 ++++++
>  drivers/gpu/drm/nouveau/nouveau_drm.c |  3 ++-
>  include/drm/drmP.h                    | 23 ++++++++++++-----------
>  3 files changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
> index f861361..21eb180 100644
> --- a/drivers/gpu/drm/drm_lock.c
> +++ b/drivers/gpu/drm/drm_lock.c
> @@ -61,6 +61,9 @@ int drm_legacy_lock(struct drm_device *dev, void *data,
>  	struct drm_master *master = file_priv->master;
>  	int ret = 0;
>  
> +	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))

You also need to allow these for all legacy drivers, i.e. without
DRIVER_MODESET.
-Daniel

> +		return -EINVAL;
> +
>  	++file_priv->lock_count;
>  
>  	if (lock->context == DRM_KERNEL_CONTEXT) {
> @@ -153,6 +156,9 @@ int drm_legacy_unlock(struct drm_device *dev, void *data, struct drm_file *file_
>  	struct drm_lock *lock = data;
>  	struct drm_master *master = file_priv->master;
>  
> +	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> +		return -EINVAL;
> +
>  	if (lock->context == DRM_KERNEL_CONTEXT) {
>  		DRM_ERROR("Process %d using kernel context %d\n",
>  			  task_pid_nr(current), lock->context);
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index 8904933..9624b38 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -941,7 +941,8 @@ static struct drm_driver
>  driver_stub = {
>  	.driver_features =
>  		DRIVER_USE_AGP |
> -		DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER,
> +		DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER |
> +		DRIVER_KMS_LEGACY_CONTEXT,
>  
>  	.load = nouveau_drm_load,
>  	.unload = nouveau_drm_unload,
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index df6d997..3874942 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -137,17 +137,18 @@ void drm_err(const char *format, ...);
>  /*@{*/
>  
>  /* driver capabilities and requirements mask */
> -#define DRIVER_USE_AGP     0x1
> -#define DRIVER_PCI_DMA     0x8
> -#define DRIVER_SG          0x10
> -#define DRIVER_HAVE_DMA    0x20
> -#define DRIVER_HAVE_IRQ    0x40
> -#define DRIVER_IRQ_SHARED  0x80
> -#define DRIVER_GEM         0x1000
> -#define DRIVER_MODESET     0x2000
> -#define DRIVER_PRIME       0x4000
> -#define DRIVER_RENDER      0x8000
> -#define DRIVER_ATOMIC      0x10000
> +#define DRIVER_USE_AGP			0x1
> +#define DRIVER_PCI_DMA			0x8
> +#define DRIVER_SG			0x10
> +#define DRIVER_HAVE_DMA			0x20
> +#define DRIVER_HAVE_IRQ			0x40
> +#define DRIVER_IRQ_SHARED		0x80
> +#define DRIVER_GEM			0x1000
> +#define DRIVER_MODESET			0x2000
> +#define DRIVER_PRIME			0x4000
> +#define DRIVER_RENDER			0x8000
> +#define DRIVER_ATOMIC			0x10000
> +#define DRIVER_KMS_LEGACY_CONTEXT	0x20000
>  
>  /***********************************************************************/
>  /** \name Macros to make printk easier */
> -- 
> 1.9.1
>
Daniel Vetter May 13, 2015, 7:24 a.m. UTC | #2 
Adding dri-devel, I've forgotten to do that ...
-Daniel

On Wed, May 13, 2015 at 09:14:29AM +0200, Daniel Vetter wrote:
> On Wed, May 13, 2015 at 07:54:47AM +0100, Peter Antoine wrote:
> > As these functions are only used by one driver and there are security holes
> > in these functions. Make the functions optional.
> 
> Is there a reference for why nouveau needs hw locks too? Also have you
> done an audit of mesa history and X history to make sure there's no other
> driver accidentally using it with a modern kms driver?
> 
> > Issue: VIZ-5485
> > Signed-off-by: Peter Antoine <peter.antoine@intel.com>
> > ---
> >  drivers/gpu/drm/drm_lock.c            |  6 ++++++
> >  drivers/gpu/drm/nouveau/nouveau_drm.c |  3 ++-
> >  include/drm/drmP.h                    | 23 ++++++++++++-----------
> >  3 files changed, 20 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
> > index f861361..21eb180 100644
> > --- a/drivers/gpu/drm/drm_lock.c
> > +++ b/drivers/gpu/drm/drm_lock.c
> > @@ -61,6 +61,9 @@ int drm_legacy_lock(struct drm_device *dev, void *data,
> >  	struct drm_master *master = file_priv->master;
> >  	int ret = 0;
> >  
> > +	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> 
> You also need to allow these for all legacy drivers, i.e. without
> DRIVER_MODESET.
> -Daniel
> 
> > +		return -EINVAL;
> > +
> >  	++file_priv->lock_count;
> >  
> >  	if (lock->context == DRM_KERNEL_CONTEXT) {
> > @@ -153,6 +156,9 @@ int drm_legacy_unlock(struct drm_device *dev, void *data, struct drm_file *file_
> >  	struct drm_lock *lock = data;
> >  	struct drm_master *master = file_priv->master;
> >  
> > +	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
> > +		return -EINVAL;
> > +
> >  	if (lock->context == DRM_KERNEL_CONTEXT) {
> >  		DRM_ERROR("Process %d using kernel context %d\n",
> >  			  task_pid_nr(current), lock->context);
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > index 8904933..9624b38 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > @@ -941,7 +941,8 @@ static struct drm_driver
> >  driver_stub = {
> >  	.driver_features =
> >  		DRIVER_USE_AGP |
> > -		DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER,
> > +		DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER |
> > +		DRIVER_KMS_LEGACY_CONTEXT,
> >  
> >  	.load = nouveau_drm_load,
> >  	.unload = nouveau_drm_unload,
> > diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> > index df6d997..3874942 100644
> > --- a/include/drm/drmP.h
> > +++ b/include/drm/drmP.h
> > @@ -137,17 +137,18 @@ void drm_err(const char *format, ...);
> >  /*@{*/
> >  
> >  /* driver capabilities and requirements mask */
> > -#define DRIVER_USE_AGP     0x1
> > -#define DRIVER_PCI_DMA     0x8
> > -#define DRIVER_SG          0x10
> > -#define DRIVER_HAVE_DMA    0x20
> > -#define DRIVER_HAVE_IRQ    0x40
> > -#define DRIVER_IRQ_SHARED  0x80
> > -#define DRIVER_GEM         0x1000
> > -#define DRIVER_MODESET     0x2000
> > -#define DRIVER_PRIME       0x4000
> > -#define DRIVER_RENDER      0x8000
> > -#define DRIVER_ATOMIC      0x10000
> > +#define DRIVER_USE_AGP			0x1
> > +#define DRIVER_PCI_DMA			0x8
> > +#define DRIVER_SG			0x10
> > +#define DRIVER_HAVE_DMA			0x20
> > +#define DRIVER_HAVE_IRQ			0x40
> > +#define DRIVER_IRQ_SHARED		0x80
> > +#define DRIVER_GEM			0x1000
> > +#define DRIVER_MODESET			0x2000
> > +#define DRIVER_PRIME			0x4000
> > +#define DRIVER_RENDER			0x8000
> > +#define DRIVER_ATOMIC			0x10000
> > +#define DRIVER_KMS_LEGACY_CONTEXT	0x20000
> >  
> >  /***********************************************************************/
> >  /** \name Macros to make printk easier */
> > -- 
> > 1.9.1
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
index f861361..21eb180 100644
--- a/drivers/gpu/drm/drm_lock.c
+++ b/drivers/gpu/drm/drm_lock.c
@@ -61,6 +61,9 @@  int drm_legacy_lock(struct drm_device *dev, void *data,
 	struct drm_master *master = file_priv->master;
 	int ret = 0;
 
+	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
+		return -EINVAL;
+
 	++file_priv->lock_count;
 
 	if (lock->context == DRM_KERNEL_CONTEXT) {
@@ -153,6 +156,9 @@  int drm_legacy_unlock(struct drm_device *dev, void *data, struct drm_file *file_
 	struct drm_lock *lock = data;
 	struct drm_master *master = file_priv->master;
 
+	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT))
+		return -EINVAL;
+
 	if (lock->context == DRM_KERNEL_CONTEXT) {
 		DRM_ERROR("Process %d using kernel context %d\n",
 			  task_pid_nr(current), lock->context);
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 8904933..9624b38 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -941,7 +941,8 @@  static struct drm_driver
 driver_stub = {
 	.driver_features =
 		DRIVER_USE_AGP |
-		DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER,
+		DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_RENDER |
+		DRIVER_KMS_LEGACY_CONTEXT,
 
 	.load = nouveau_drm_load,
 	.unload = nouveau_drm_unload,
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index df6d997..3874942 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -137,17 +137,18 @@  void drm_err(const char *format, ...);
 /*@{*/
 
 /* driver capabilities and requirements mask */
-#define DRIVER_USE_AGP     0x1
-#define DRIVER_PCI_DMA     0x8
-#define DRIVER_SG          0x10
-#define DRIVER_HAVE_DMA    0x20
-#define DRIVER_HAVE_IRQ    0x40
-#define DRIVER_IRQ_SHARED  0x80
-#define DRIVER_GEM         0x1000
-#define DRIVER_MODESET     0x2000
-#define DRIVER_PRIME       0x4000
-#define DRIVER_RENDER      0x8000
-#define DRIVER_ATOMIC      0x10000
+#define DRIVER_USE_AGP			0x1
+#define DRIVER_PCI_DMA			0x8
+#define DRIVER_SG			0x10
+#define DRIVER_HAVE_DMA			0x20
+#define DRIVER_HAVE_IRQ			0x40
+#define DRIVER_IRQ_SHARED		0x80
+#define DRIVER_GEM			0x1000
+#define DRIVER_MODESET			0x2000
+#define DRIVER_PRIME			0x4000
+#define DRIVER_RENDER			0x8000
+#define DRIVER_ATOMIC			0x10000
+#define DRIVER_KMS_LEGACY_CONTEXT	0x20000
 
 /***********************************************************************/
 /** \name Macros to make printk easier */