Re: [PATCH 4/5] drm: Make HW_LOCK access functions optional.

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Apr 23, 2015 at 03:07:57PM +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.
> 
> Issue: VIZ-5485
> Signed-off-by: Peter Antoine <peter.antoine@intel.com>
> ---
>  drivers/gpu/drm/drm_lock.c            |  6 ++++++
>  drivers/gpu/drm/i915/i915_dma.c       |  3 +++
>  drivers/gpu/drm/nouveau/nouveau_drm.c |  3 ++-
>  include/drm/drmP.h                    | 23 ++++++++++++-----------
>  include/uapi/drm/i915_drm.h           |  1 +
>  5 files changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_lock.c b/drivers/gpu/drm/drm_lock.c
> index 94500930..b61d4c7 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 (_DRM_LOCKING_CONTEXT(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 (_DRM_LOCKING_CONTEXT(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/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index e44116f..c771ef0 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -163,6 +163,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
>  		if (!value)
>  			return -ENODEV;
>  		break;
> +	case I915_PARAM_HAS_LEGACY_CONTEXT:
> +		value = drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT);
> +		break;

Seems pointless to add a parameter that'll always be false.

>  	default:
>  		DRM_DEBUG("Unknown parameter %d\n", param->param);
>  		return -EINVAL;
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index 8763deb..936b423 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -940,7 +940,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,

Why is this here? AFAICS only the via driver cares about legacy
contexts, and only dri1 drivers care about the hw lock.

>  
>  	.load = nouveau_drm_load,
>  	.unload = nouveau_drm_unload,
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 62c40777..367e42f 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

Why is there KMS in the name?

I was thinking just checking for GEM, but I think there was some
gem+dri1 userland for i915 at some point in time. ums and dri1 are
now dead as far as i915 is concerned, so in theory it should be fine.
But I'm not sure if some other driver might have the same baggage.

I suppose one option would be to check for MODESET instead. kms+dri1
doesn't sound like an entirely sane combination to me.

>  
>  /***********************************************************************/
>  /** \name Macros to make printk easier */
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 4851d66..0ad611a2 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -350,6 +350,7 @@ typedef struct drm_i915_irq_wait {
>  #define I915_PARAM_REVISION              32
>  #define I915_PARAM_SUBSLICE_TOTAL	 33
>  #define I915_PARAM_EU_TOTAL		 34
> +#define I915_PARAM_HAS_LEGACY_CONTEXT	 35
>  
>  typedef struct drm_i915_getparam {
>  	int param;
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx