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.anto...@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
