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

Daniel Vetter
2015-05-13T07:12:09Z

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