[PATCH/RFC 0/3] Constify drm_driver

[PATCH/RFC 0/3] Constify drm_driver

[Laurent Pinchart]

Hello,

This patch series makes it possible for DRM drivers to declare their
struct drm_driver as a static const. This improves security by avoiding
function pointers in writable memory.

The change turned out to be fairly easy, with preparation in patch 1/3
that moves the only non-const field out of drm_driver, and patch 2/3
that performs the constification in the DRM core. Patch 3/3 is an
example of driver conversion to const drm_driver. If the series is
accepted, I'll write a coccinelle patch that handles all drivers.

Laurent Pinchart (3):
  drm: Move legacy device list out of drm_driver
  drm: Use a const drm_driver through the DRM core
  drm: rcar_du: Constify drm_driver

 drivers/gpu/drm/drm_drv.c                | 10 +++----
 drivers/gpu/drm/drm_pci.c                | 38 +++++++++++++++++-------
 drivers/gpu/drm/drm_vram_helper_common.c |  4 +--
 drivers/gpu/drm/rcar-du/rcar_du_drv.c    |  2 +-
 include/drm/drm_device.h                 |  4 ++-
 include/drm/drm_drv.h                    |  8 ++---
 include/drm/drm_legacy.h                 | 10 ++++---
 include/drm/drm_pci.h                    |  4 +--
 8 files changed, 49 insertions(+), 31 deletions(-)

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH/RFC 1/3] drm: Move legacy device list out of drm_driver

[Laurent Pinchart]

The drm_driver structure contains a single field (legacy_dev_list) that
is modified by the DRM core, used to store a linked list of legacy DRM
devices associated with the driver. In order to make the structure
const, move the field out to a global variable. This requires locking
access to the global where the local field didn't require serialization,
but this only affects legacy drivers, and isn't in any hot path.

While at it, compile-out the legacy_dev_list field when DRM_LEGACY isn't
defined.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/drm_pci.c | 30 ++++++++++++++++++++++--------
 include/drm/drm_device.h  |  2 ++
 include/drm/drm_drv.h     |  2 --
 3 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
index c6bb98729a26..44805ac3177c 100644
--- a/drivers/gpu/drm/drm_pci.c
+++ b/drivers/gpu/drm/drm_pci.c
@@ -24,6 +24,8 @@
 
 #include <linux/dma-mapping.h>
 #include <linux/export.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
 #include <linux/pci.h>
 #include <linux/slab.h>
 
@@ -36,6 +38,12 @@
 #include "drm_internal.h"
 #include "drm_legacy.h"
 
+#ifdef CONFIG_DRM_LEGACY
+/* List of devices hanging off drivers with stealth attach. */
+static LIST_HEAD(legacy_dev_list);
+static DEFINE_MUTEX(legacy_dev_list_lock);
+#endif
+
 /**
  * drm_pci_alloc - Allocate a PCI consistent memory block, for DMA.
  * @dev: DRM device
@@ -236,10 +244,13 @@ int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent,
 	if (ret)
 		goto err_agp;
 
-	/* No locking needed since shadow-attach is single-threaded since it may
-	 * only be called from the per-driver module init hook. */
-	if (drm_core_check_feature(dev, DRIVER_LEGACY))
-		list_add_tail(&dev->legacy_dev_list, &driver->legacy_dev_list);
+#ifdef CONFIG_DRM_LEGACY
+	if (drm_core_check_feature(dev, DRIVER_LEGACY)) {
+		mutex_lock(&legacy_dev_list_lock);
+		list_add_tail(&dev->legacy_dev_list, &legacy_dev_list);
+		mutex_unlock(&legacy_dev_list_lock);
+	}
+#endif
 
 	return 0;
 
@@ -275,7 +286,6 @@ int drm_legacy_pci_init(struct drm_driver *driver, struct pci_driver *pdriver)
 		return -EINVAL;
 
 	/* If not using KMS, fall back to stealth mode manual scanning. */
-	INIT_LIST_HEAD(&driver->legacy_dev_list);
 	for (i = 0; pdriver->id_table[i].vendor != 0; i++) {
 		pid = &pdriver->id_table[i];
 
@@ -317,11 +327,15 @@ void drm_legacy_pci_exit(struct drm_driver *driver, struct pci_driver *pdriver)
 	if (!(driver->driver_features & DRIVER_LEGACY)) {
 		WARN_ON(1);
 	} else {
-		list_for_each_entry_safe(dev, tmp, &driver->legacy_dev_list,
+		mutex_lock(&legacy_dev_list_lock);
+		list_for_each_entry_safe(dev, tmp, &legacy_dev_list,
 					 legacy_dev_list) {
-			list_del(&dev->legacy_dev_list);
-			drm_put_dev(dev);
+			if (dev->driver == driver) {
+				list_del(&dev->legacy_dev_list);
+				drm_put_dev(dev);
+			}
 		}
+		mutex_unlock(&legacy_dev_list_lock);
 	}
 	DRM_INFO("Module unloaded\n");
 }
diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
index bb60a949f416..215b3472c773 100644
--- a/include/drm/drm_device.h
+++ b/include/drm/drm_device.h
@@ -51,12 +51,14 @@ enum switch_power_state {
  * may contain multiple heads.
  */
 struct drm_device {
+#ifdef CONFIG_DRM_LEGACY
 	/**
 	 * @legacy_dev_list:
 	 *
 	 * List of devices per driver for stealth attach cleanup
 	 */
 	struct list_head legacy_dev_list;
+#endif
 
 	/** @if_version: Highest interface version set */
 	int if_version;
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index 97109df5beac..7dcf3b7bb5e6 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -601,8 +601,6 @@ struct drm_driver {
 	/* Everything below here is for legacy driver, never use! */
 	/* private: */
 
-	/* List of devices hanging off this driver with stealth attach. */
-	struct list_head legacy_dev_list;
 	int (*firstopen) (struct drm_device *);
 	void (*preclose) (struct drm_device *, struct drm_file *file_priv);
 	int (*dma_ioctl) (struct drm_device *dev, void *data, struct drm_file *file_priv);
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH/RFC 2/3] drm: Use a const drm_driver through the DRM core

[Laurent Pinchart]

The drm_driver structure contains pointers to functions, which can be an
attack vector if an attacker can corrupt the structure. The DRM core
however never modifies the structure, so it could be declared as const
in drivers. Modify the DRM core to take const struct drm_driver
pointers in all APIs.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/drm_drv.c                | 10 +++++-----
 drivers/gpu/drm/drm_pci.c                |  8 +++++---
 drivers/gpu/drm/drm_vram_helper_common.c |  4 ++--
 include/drm/drm_device.h                 |  2 +-
 include/drm/drm_drv.h                    |  6 +++---
 include/drm/drm_legacy.h                 | 10 ++++++----
 include/drm/drm_pci.h                    |  4 ++--
 7 files changed, 24 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 7b1a628d1f6e..41654427d258 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -300,7 +300,7 @@ void drm_minor_release(struct drm_minor *minor)
  *		kfree(priv);
  *	}
  *
- *	static struct drm_driver driver_drm_driver = {
+ *	static const struct drm_driver driver_drm_driver = {
  *		[...]
  *		.release = driver_drm_release,
  *	};
@@ -612,7 +612,7 @@ static void drm_fs_inode_free(struct inode *inode)
  * 0 on success, or error code on failure.
  */
 int drm_dev_init(struct drm_device *dev,
-		 struct drm_driver *driver,
+		 const struct drm_driver *driver,
 		 struct device *parent)
 {
 	int ret;
@@ -722,7 +722,7 @@ static void devm_drm_dev_init_release(void *data)
  */
 int devm_drm_dev_init(struct device *parent,
 		      struct drm_device *dev,
-		      struct drm_driver *driver)
+		      const struct drm_driver *driver)
 {
 	int ret;
 
@@ -800,7 +800,7 @@ EXPORT_SYMBOL(drm_dev_fini);
  * RETURNS:
  * Pointer to new DRM device, or ERR_PTR on failure.
  */
-struct drm_device *drm_dev_alloc(struct drm_driver *driver,
+struct drm_device *drm_dev_alloc(const struct drm_driver *driver,
 				 struct device *parent)
 {
 	struct drm_device *dev;
@@ -943,7 +943,7 @@ static void remove_compat_control_link(struct drm_device *dev)
  */
 int drm_dev_register(struct drm_device *dev, unsigned long flags)
 {
-	struct drm_driver *driver = dev->driver;
+	const struct drm_driver *driver = dev->driver;
 	int ret;
 
 	if (drm_dev_needs_global_mutex(dev))
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
index 44805ac3177c..2ca7adf270c6 100644
--- a/drivers/gpu/drm/drm_pci.c
+++ b/drivers/gpu/drm/drm_pci.c
@@ -215,7 +215,7 @@ void drm_pci_agp_destroy(struct drm_device *dev)
  * Return: 0 on success or a negative error code on failure.
  */
 int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent,
-		    struct drm_driver *driver)
+		    const struct drm_driver *driver)
 {
 	struct drm_device *dev;
 	int ret;
@@ -274,7 +274,8 @@ EXPORT_SYMBOL(drm_get_pci_dev);
  *
  * Return: 0 on success or a negative error code on failure.
  */
-int drm_legacy_pci_init(struct drm_driver *driver, struct pci_driver *pdriver)
+int drm_legacy_pci_init(const struct drm_driver *driver,
+			struct pci_driver *pdriver)
 {
 	struct pci_dev *pdev = NULL;
 	const struct pci_device_id *pid;
@@ -319,7 +320,8 @@ EXPORT_SYMBOL(drm_legacy_pci_init);
  * Unregister a DRM driver shadow-attached through drm_legacy_pci_init(). This
  * is deprecated and only used by dri1 drivers.
  */
-void drm_legacy_pci_exit(struct drm_driver *driver, struct pci_driver *pdriver)
+void drm_legacy_pci_exit(const struct drm_driver *driver,
+			 struct pci_driver *pdriver)
 {
 	struct drm_device *dev, *tmp;
 	DRM_DEBUG("\n");
diff --git a/drivers/gpu/drm/drm_vram_helper_common.c b/drivers/gpu/drm/drm_vram_helper_common.c
index 2000d9b33fd5..e93b04bbe2de 100644
--- a/drivers/gpu/drm/drm_vram_helper_common.c
+++ b/drivers/gpu/drm/drm_vram_helper_common.c
@@ -29,11 +29,11 @@
  *
  * .. code-block:: c
  *
- *	struct file_operations fops ={
+ *	const struct file_operations fops ={
  *		.owner = THIS_MODULE,
  *		DRM_VRAM_MM_FILE_OPERATION
  *	};
- *	struct drm_driver drv = {
+ *	const struct drm_driver drv = {
  *		.driver_feature = DRM_ ... ,
  *		.fops = &fops,
  *		DRM_GEM_VRAM_DRIVER
diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
index 215b3472c773..6ed5d84e5f5d 100644
--- a/include/drm/drm_device.h
+++ b/include/drm/drm_device.h
@@ -70,7 +70,7 @@ struct drm_device {
 	struct device *dev;
 
 	/** @driver: DRM driver managing the device */
-	struct drm_driver *driver;
+	const struct drm_driver *driver;
 
 	/**
 	 * @dev_private:
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index 7dcf3b7bb5e6..02c9915a9244 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -613,14 +613,14 @@ struct drm_driver {
 };
 
 int drm_dev_init(struct drm_device *dev,
-		 struct drm_driver *driver,
+		 const struct drm_driver *driver,
 		 struct device *parent);
 int devm_drm_dev_init(struct device *parent,
 		      struct drm_device *dev,
-		      struct drm_driver *driver);
+		      const struct drm_driver *driver);
 void drm_dev_fini(struct drm_device *dev);
 
-struct drm_device *drm_dev_alloc(struct drm_driver *driver,
+struct drm_device *drm_dev_alloc(const struct drm_driver *driver,
 				 struct device *parent);
 int drm_dev_register(struct drm_device *dev, unsigned long flags);
 void drm_dev_unregister(struct drm_device *dev);
diff --git a/include/drm/drm_legacy.h b/include/drm/drm_legacy.h
index dcef3598f49e..49f2fd963871 100644
--- a/include/drm/drm_legacy.h
+++ b/include/drm/drm_legacy.h
@@ -194,18 +194,20 @@ void drm_legacy_idlelock_release(struct drm_lock_data *lock);
 
 #ifdef CONFIG_PCI
 
-int drm_legacy_pci_init(struct drm_driver *driver, struct pci_driver *pdriver);
-void drm_legacy_pci_exit(struct drm_driver *driver, struct pci_driver *pdriver);
+int drm_legacy_pci_init(const struct drm_driver *driver,
+			struct pci_driver *pdriver);
+void drm_legacy_pci_exit(const struct drm_driver *driver,
+			 struct pci_driver *pdriver);
 
 #else
 
-static inline int drm_legacy_pci_init(struct drm_driver *driver,
+static inline int drm_legacy_pci_init(const struct drm_driver *driver,
 				      struct pci_driver *pdriver)
 {
 	return -EINVAL;
 }
 
-static inline void drm_legacy_pci_exit(struct drm_driver *driver,
+static inline void drm_legacy_pci_exit(const struct drm_driver *driver,
 				       struct pci_driver *pdriver)
 {
 }
diff --git a/include/drm/drm_pci.h b/include/drm/drm_pci.h
index 9031e217b506..8f98ae8384c2 100644
--- a/include/drm/drm_pci.h
+++ b/include/drm/drm_pci.h
@@ -47,7 +47,7 @@ void drm_pci_free(struct drm_device *dev, struct drm_dma_handle * dmah);
 
 int drm_get_pci_dev(struct pci_dev *pdev,
 		    const struct pci_device_id *ent,
-		    struct drm_driver *driver);
+		    const struct drm_driver *driver);
 
 #else
 
@@ -64,7 +64,7 @@ static inline void drm_pci_free(struct drm_device *dev,
 
 static inline int drm_get_pci_dev(struct pci_dev *pdev,
 				  const struct pci_device_id *ent,
-				  struct drm_driver *driver)
+				  const struct drm_driver *driver)
 {
 	return -ENOSYS;
 }
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH/RFC 3/3] drm: rcar_du: Constify drm_driver

[Laurent Pinchart]

The drm_driver structure is never modified, make it const. The improves
security by avoiding writable function pointers.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
index 654e2dd08146..039eee3ef661 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
@@ -474,7 +474,7 @@ MODULE_DEVICE_TABLE(of, rcar_du_of_table);
 
 DEFINE_DRM_GEM_CMA_FOPS(rcar_du_fops);
 
-static struct drm_driver rcar_du_driver = {
+static const struct drm_driver rcar_du_driver = {
 	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
 	.gem_free_object_unlocked = drm_gem_cma_free_object,
 	.gem_vm_ops		= &drm_gem_cma_vm_ops,
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH/RFC 1/3] drm: Move legacy device list out of drm_driver

[Daniel Vetter]

On Sat, Feb 22, 2020 at 05:24:28PM +0200, Laurent Pinchart wrote:
> The drm_driver structure contains a single field (legacy_dev_list) that
> is modified by the DRM core, used to store a linked list of legacy DRM
> devices associated with the driver. In order to make the structure
> const, move the field out to a global variable. This requires locking
> access to the global where the local field didn't require serialization,
> but this only affects legacy drivers, and isn't in any hot path.
> 
> While at it, compile-out the legacy_dev_list field when DRM_LEGACY isn't
> defined.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/drm_pci.c | 30 ++++++++++++++++++++++--------
>  include/drm/drm_device.h  |  2 ++
>  include/drm/drm_drv.h     |  2 --
>  3 files changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
> index c6bb98729a26..44805ac3177c 100644
> --- a/drivers/gpu/drm/drm_pci.c
> +++ b/drivers/gpu/drm/drm_pci.c
> @@ -24,6 +24,8 @@
>  
>  #include <linux/dma-mapping.h>
>  #include <linux/export.h>
> +#include <linux/list.h>
> +#include <linux/mutex.h>
>  #include <linux/pci.h>
>  #include <linux/slab.h>
>  
> @@ -36,6 +38,12 @@
>  #include "drm_internal.h"
>  #include "drm_legacy.h"
>  
> +#ifdef CONFIG_DRM_LEGACY
> +/* List of devices hanging off drivers with stealth attach. */
> +static LIST_HEAD(legacy_dev_list);
> +static DEFINE_MUTEX(legacy_dev_list_lock);
> +#endif
> +
>  /**
>   * drm_pci_alloc - Allocate a PCI consistent memory block, for DMA.
>   * @dev: DRM device
> @@ -236,10 +244,13 @@ int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent,
>  	if (ret)
>  		goto err_agp;
>  
> -	/* No locking needed since shadow-attach is single-threaded since it may
> -	 * only be called from the per-driver module init hook. */
> -	if (drm_core_check_feature(dev, DRIVER_LEGACY))
> -		list_add_tail(&dev->legacy_dev_list, &driver->legacy_dev_list);
> +#ifdef CONFIG_DRM_LEGACY
> +	if (drm_core_check_feature(dev, DRIVER_LEGACY)) {
> +		mutex_lock(&legacy_dev_list_lock);
> +		list_add_tail(&dev->legacy_dev_list, &legacy_dev_list);
> +		mutex_unlock(&legacy_dev_list_lock);
> +	}
> +#endif
>  
>  	return 0;
>  
> @@ -275,7 +286,6 @@ int drm_legacy_pci_init(struct drm_driver *driver, struct pci_driver *pdriver)
>  		return -EINVAL;
>  
>  	/* If not using KMS, fall back to stealth mode manual scanning. */
> -	INIT_LIST_HEAD(&driver->legacy_dev_list);
>  	for (i = 0; pdriver->id_table[i].vendor != 0; i++) {
>  		pid = &pdriver->id_table[i];
>  
> @@ -317,11 +327,15 @@ void drm_legacy_pci_exit(struct drm_driver *driver, struct pci_driver *pdriver)
>  	if (!(driver->driver_features & DRIVER_LEGACY)) {
>  		WARN_ON(1);
>  	} else {
> -		list_for_each_entry_safe(dev, tmp, &driver->legacy_dev_list,
> +		mutex_lock(&legacy_dev_list_lock);
> +		list_for_each_entry_safe(dev, tmp, &legacy_dev_list,
>  					 legacy_dev_list) {
> -			list_del(&dev->legacy_dev_list);
> -			drm_put_dev(dev);
> +			if (dev->driver == driver) {
> +				list_del(&dev->legacy_dev_list);
> +				drm_put_dev(dev);

I checked whether this would result in any issues with the new mutex_lock,
but with the legacy model there's no hotunplug or anything like that, so
we never need to remove ourselves from this list coming from the other
direction. We just oops :-)

> +			}
>  		}
> +		mutex_unlock(&legacy_dev_list_lock);
>  	}
>  	DRM_INFO("Module unloaded\n");
>  }
> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> index bb60a949f416..215b3472c773 100644
> --- a/include/drm/drm_device.h
> +++ b/include/drm/drm_device.h
> @@ -51,12 +51,14 @@ enum switch_power_state {
>   * may contain multiple heads.
>   */
>  struct drm_device {
> +#ifdef CONFIG_DRM_LEGACY
>  	/**
>  	 * @legacy_dev_list:
>  	 *
>  	 * List of devices per driver for stealth attach cleanup
>  	 */
>  	struct list_head legacy_dev_list;
> +#endif

We have a CONFIG_DRM_LEGACY dungeon already at the end of this struct, can
you pls move it there? Also drop the kerneldoc comment, we want to hide
this for good :-)

With that tiny bikeshed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>  
>  	/** @if_version: Highest interface version set */
>  	int if_version;
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index 97109df5beac..7dcf3b7bb5e6 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -601,8 +601,6 @@ struct drm_driver {
>  	/* Everything below here is for legacy driver, never use! */
>  	/* private: */
>  
> -	/* List of devices hanging off this driver with stealth attach. */
> -	struct list_head legacy_dev_list;
>  	int (*firstopen) (struct drm_device *);
>  	void (*preclose) (struct drm_device *, struct drm_file *file_priv);
>  	int (*dma_ioctl) (struct drm_device *dev, void *data, struct drm_file *file_priv);
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH/RFC 2/3] drm: Use a const drm_driver through the DRM core

[Daniel Vetter]

On Sat, Feb 22, 2020 at 05:24:29PM +0200, Laurent Pinchart wrote:
> The drm_driver structure contains pointers to functions, which can be an
> attack vector if an attacker can corrupt the structure. The DRM core
> however never modifies the structure, so it could be declared as const
> in drivers. Modify the DRM core to take const struct drm_driver
> pointers in all APIs.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Assuming everything still compiles properly:

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_drv.c                | 10 +++++-----
>  drivers/gpu/drm/drm_pci.c                |  8 +++++---
>  drivers/gpu/drm/drm_vram_helper_common.c |  4 ++--
>  include/drm/drm_device.h                 |  2 +-
>  include/drm/drm_drv.h                    |  6 +++---
>  include/drm/drm_legacy.h                 | 10 ++++++----
>  include/drm/drm_pci.h                    |  4 ++--
>  7 files changed, 24 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 7b1a628d1f6e..41654427d258 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -300,7 +300,7 @@ void drm_minor_release(struct drm_minor *minor)
>   *		kfree(priv);
>   *	}
>   *
> - *	static struct drm_driver driver_drm_driver = {
> + *	static const struct drm_driver driver_drm_driver = {
>   *		[...]
>   *		.release = driver_drm_release,
>   *	};
> @@ -612,7 +612,7 @@ static void drm_fs_inode_free(struct inode *inode)
>   * 0 on success, or error code on failure.
>   */
>  int drm_dev_init(struct drm_device *dev,
> -		 struct drm_driver *driver,
> +		 const struct drm_driver *driver,
>  		 struct device *parent)
>  {
>  	int ret;
> @@ -722,7 +722,7 @@ static void devm_drm_dev_init_release(void *data)
>   */
>  int devm_drm_dev_init(struct device *parent,
>  		      struct drm_device *dev,
> -		      struct drm_driver *driver)
> +		      const struct drm_driver *driver)
>  {
>  	int ret;
>  
> @@ -800,7 +800,7 @@ EXPORT_SYMBOL(drm_dev_fini);
>   * RETURNS:
>   * Pointer to new DRM device, or ERR_PTR on failure.
>   */
> -struct drm_device *drm_dev_alloc(struct drm_driver *driver,
> +struct drm_device *drm_dev_alloc(const struct drm_driver *driver,
>  				 struct device *parent)
>  {
>  	struct drm_device *dev;
> @@ -943,7 +943,7 @@ static void remove_compat_control_link(struct drm_device *dev)
>   */
>  int drm_dev_register(struct drm_device *dev, unsigned long flags)
>  {
> -	struct drm_driver *driver = dev->driver;
> +	const struct drm_driver *driver = dev->driver;
>  	int ret;
>  
>  	if (drm_dev_needs_global_mutex(dev))
> diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
> index 44805ac3177c..2ca7adf270c6 100644
> --- a/drivers/gpu/drm/drm_pci.c
> +++ b/drivers/gpu/drm/drm_pci.c
> @@ -215,7 +215,7 @@ void drm_pci_agp_destroy(struct drm_device *dev)
>   * Return: 0 on success or a negative error code on failure.
>   */
>  int drm_get_pci_dev(struct pci_dev *pdev, const struct pci_device_id *ent,
> -		    struct drm_driver *driver)
> +		    const struct drm_driver *driver)
>  {
>  	struct drm_device *dev;
>  	int ret;
> @@ -274,7 +274,8 @@ EXPORT_SYMBOL(drm_get_pci_dev);
>   *
>   * Return: 0 on success or a negative error code on failure.
>   */
> -int drm_legacy_pci_init(struct drm_driver *driver, struct pci_driver *pdriver)
> +int drm_legacy_pci_init(const struct drm_driver *driver,
> +			struct pci_driver *pdriver)
>  {
>  	struct pci_dev *pdev = NULL;
>  	const struct pci_device_id *pid;
> @@ -319,7 +320,8 @@ EXPORT_SYMBOL(drm_legacy_pci_init);
>   * Unregister a DRM driver shadow-attached through drm_legacy_pci_init(). This
>   * is deprecated and only used by dri1 drivers.
>   */
> -void drm_legacy_pci_exit(struct drm_driver *driver, struct pci_driver *pdriver)
> +void drm_legacy_pci_exit(const struct drm_driver *driver,
> +			 struct pci_driver *pdriver)
>  {
>  	struct drm_device *dev, *tmp;
>  	DRM_DEBUG("\n");
> diff --git a/drivers/gpu/drm/drm_vram_helper_common.c b/drivers/gpu/drm/drm_vram_helper_common.c
> index 2000d9b33fd5..e93b04bbe2de 100644
> --- a/drivers/gpu/drm/drm_vram_helper_common.c
> +++ b/drivers/gpu/drm/drm_vram_helper_common.c
> @@ -29,11 +29,11 @@
>   *
>   * .. code-block:: c
>   *
> - *	struct file_operations fops ={
> + *	const struct file_operations fops ={
>   *		.owner = THIS_MODULE,
>   *		DRM_VRAM_MM_FILE_OPERATION
>   *	};
> - *	struct drm_driver drv = {
> + *	const struct drm_driver drv = {
>   *		.driver_feature = DRM_ ... ,
>   *		.fops = &fops,
>   *		DRM_GEM_VRAM_DRIVER
> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> index 215b3472c773..6ed5d84e5f5d 100644
> --- a/include/drm/drm_device.h
> +++ b/include/drm/drm_device.h
> @@ -70,7 +70,7 @@ struct drm_device {
>  	struct device *dev;
>  
>  	/** @driver: DRM driver managing the device */
> -	struct drm_driver *driver;
> +	const struct drm_driver *driver;
>  
>  	/**
>  	 * @dev_private:
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index 7dcf3b7bb5e6..02c9915a9244 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -613,14 +613,14 @@ struct drm_driver {
>  };
>  
>  int drm_dev_init(struct drm_device *dev,
> -		 struct drm_driver *driver,
> +		 const struct drm_driver *driver,
>  		 struct device *parent);
>  int devm_drm_dev_init(struct device *parent,
>  		      struct drm_device *dev,
> -		      struct drm_driver *driver);
> +		      const struct drm_driver *driver);
>  void drm_dev_fini(struct drm_device *dev);
>  
> -struct drm_device *drm_dev_alloc(struct drm_driver *driver,
> +struct drm_device *drm_dev_alloc(const struct drm_driver *driver,
>  				 struct device *parent);
>  int drm_dev_register(struct drm_device *dev, unsigned long flags);
>  void drm_dev_unregister(struct drm_device *dev);
> diff --git a/include/drm/drm_legacy.h b/include/drm/drm_legacy.h
> index dcef3598f49e..49f2fd963871 100644
> --- a/include/drm/drm_legacy.h
> +++ b/include/drm/drm_legacy.h
> @@ -194,18 +194,20 @@ void drm_legacy_idlelock_release(struct drm_lock_data *lock);
>  
>  #ifdef CONFIG_PCI
>  
> -int drm_legacy_pci_init(struct drm_driver *driver, struct pci_driver *pdriver);
> -void drm_legacy_pci_exit(struct drm_driver *driver, struct pci_driver *pdriver);
> +int drm_legacy_pci_init(const struct drm_driver *driver,
> +			struct pci_driver *pdriver);
> +void drm_legacy_pci_exit(const struct drm_driver *driver,
> +			 struct pci_driver *pdriver);
>  
>  #else
>  
> -static inline int drm_legacy_pci_init(struct drm_driver *driver,
> +static inline int drm_legacy_pci_init(const struct drm_driver *driver,
>  				      struct pci_driver *pdriver)
>  {
>  	return -EINVAL;
>  }
>  
> -static inline void drm_legacy_pci_exit(struct drm_driver *driver,
> +static inline void drm_legacy_pci_exit(const struct drm_driver *driver,
>  				       struct pci_driver *pdriver)
>  {
>  }
> diff --git a/include/drm/drm_pci.h b/include/drm/drm_pci.h
> index 9031e217b506..8f98ae8384c2 100644
> --- a/include/drm/drm_pci.h
> +++ b/include/drm/drm_pci.h
> @@ -47,7 +47,7 @@ void drm_pci_free(struct drm_device *dev, struct drm_dma_handle * dmah);
>  
>  int drm_get_pci_dev(struct pci_dev *pdev,
>  		    const struct pci_device_id *ent,
> -		    struct drm_driver *driver);
> +		    const struct drm_driver *driver);
>  
>  #else
>  
> @@ -64,7 +64,7 @@ static inline void drm_pci_free(struct drm_device *dev,
>  
>  static inline int drm_get_pci_dev(struct pci_dev *pdev,
>  				  const struct pci_device_id *ent,
> -				  struct drm_driver *driver)
> +				  const struct drm_driver *driver)
>  {
>  	return -ENOSYS;
>  }
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH/RFC 3/3] drm: rcar_du: Constify drm_driver

[Daniel Vetter]

On Sat, Feb 22, 2020 at 05:24:30PM +0200, Laurent Pinchart wrote:
> The drm_driver structure is never modified, make it const. The improves
> security by avoiding writable function pointers.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

I wonder whether there's some magic somewhere we could do to enlist the
cocci army to create the constify patches for us ...


Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/rcar-du/rcar_du_drv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> index 654e2dd08146..039eee3ef661 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> @@ -474,7 +474,7 @@ MODULE_DEVICE_TABLE(of, rcar_du_of_table);
>  
>  DEFINE_DRM_GEM_CMA_FOPS(rcar_du_fops);
>  
> -static struct drm_driver rcar_du_driver = {
> +static const struct drm_driver rcar_du_driver = {
>  	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
>  	.gem_free_object_unlocked = drm_gem_cma_free_object,
>  	.gem_vm_ops		= &drm_gem_cma_vm_ops,
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH/RFC 3/3] drm: rcar_du: Constify drm_driver

[Emil Velikov]

Thanks Laurent for sorting this out.

On Sat, 22 Feb 2020 at 17:59, Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Sat, Feb 22, 2020 at 05:24:30PM +0200, Laurent Pinchart wrote:
> > The drm_driver structure is never modified, make it const. The improves
> > security by avoiding writable function pointers.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
> I wonder whether there's some magic somewhere we could do to enlist the
> cocci army to create the constify patches for us ...
>
IIRC some drivers still manually thinker with their struct drm_driver ;-)

That said, the series looks good:
Reviewed-by: Emil Velikov <emil.velikov@collabora.com>

-Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH/RFC 3/3] drm: rcar_du: Constify drm_driver

[Laurent Pinchart]

Hi Emil,

On Mon, Feb 24, 2020 at 03:26:05PM +0000, Emil Velikov wrote:
> Thanks Laurent for sorting this out.

You're welcome. It's been bothering me for some time :-)

> On Sat, 22 Feb 2020 at 17:59, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Sat, Feb 22, 2020 at 05:24:30PM +0200, Laurent Pinchart wrote:
> > > The drm_driver structure is never modified, make it const. The improves
> > > security by avoiding writable function pointers.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >
> > I wonder whether there's some magic somewhere we could do to enlist the
> > cocci army to create the constify patches for us ...
> >
> IIRC some drivers still manually thinker with their struct drm_driver ;-)
> 
> That said, the series looks good:
> Reviewed-by: Emil Velikov <emil.velikov@collabora.com>

Does this apply to the whole series, or to this patch only ?

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel