[PATCH 1/2] drm: make drm_dev_set_unique() not use a format string

[PATCH 1/2] drm: make drm_dev_set_unique() not use a format string

[Nicolas Iooss]

drm_dev_set_unique() uses a format string to define the unique name of a
device.  This feature is not used as currently all the calls to this
function either use "%s" as a format string or directly use
dev_name().

Even though this second kind of call does not introduce security
problems, because there cannot be "%" characters in dev_name() results,
gcc issues a warning when building with -Wformat-security flag
("warning: format string is not a string literal (potentially
insecure)").  This warning is useful to find real bugs like the one
fixed by commit 3958b79266b1 ("configfs: fix kernel infoleak through
user-controlled format string").  False positives which do not bring
an extra value make the work of finding real bugs harder.

Therefore remove the format-string feature from drm_dev_set_unique().

Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
---
 drivers/gpu/drm/drm_drv.c                   | 11 +++--------
 drivers/gpu/drm/nouveau/nouveau_drm.c       |  2 +-
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c |  2 +-
 include/drm/drmP.h                          |  2 +-
 4 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 7dd6728dd092..20eaa0aae205 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -797,7 +797,7 @@ EXPORT_SYMBOL(drm_dev_unregister);
 /**
  * drm_dev_set_unique - Set the unique name of a DRM device
  * @dev: device of which to set the unique name
- * @fmt: format string for unique name
+ * @name: unique name
  *
  * Sets the unique name of a DRM device using the specified format string and
  * a variable list of arguments. Drivers can use this at driver probe time if
@@ -805,15 +805,10 @@ EXPORT_SYMBOL(drm_dev_unregister);
  *
  * Return: 0 on success or a negative error code on failure.
  */
-int drm_dev_set_unique(struct drm_device *dev, const char *fmt, ...)
+int drm_dev_set_unique(struct drm_device *dev, const char *name)
 {
-	va_list ap;
-
 	kfree(dev->unique);
-
-	va_start(ap, fmt);
-	dev->unique = kvasprintf(GFP_KERNEL, fmt, ap);
-	va_end(ap);
+	dev->unique = kstrdup(name, GFP_KERNEL);
 
 	return dev->unique ? 0 : -ENOMEM;
 }
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 1d3ee5179ab8..2d23f95f17ce 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -1046,7 +1046,7 @@ nouveau_platform_device_create(const struct nvkm_device_tegra_func *func,
 		goto err_free;
 	}
 
-	err = drm_dev_set_unique(drm, "%s", dev_name(&pdev->dev));
+	err = drm_dev_set_unique(drm, dev_name(&pdev->dev));
 	if (err < 0)
 		goto err_free;
 
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index f22e1e1ee64a..215d6c44af55 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -450,7 +450,7 @@ static int rockchip_drm_bind(struct device *dev)
 	if (!drm)
 		return -ENOMEM;
 
-	ret = drm_dev_set_unique(drm, "%s", dev_name(dev));
+	ret = drm_dev_set_unique(drm, dev_name(dev));
 	if (ret)
 		goto err_free;
 
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 0a271ca1f7c7..f14c25a6bbf2 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1059,7 +1059,7 @@ void drm_dev_ref(struct drm_device *dev);
 void drm_dev_unref(struct drm_device *dev);
 int drm_dev_register(struct drm_device *dev, unsigned long flags);
 void drm_dev_unregister(struct drm_device *dev);
-int drm_dev_set_unique(struct drm_device *dev, const char *fmt, ...);
+int drm_dev_set_unique(struct drm_device *dev, const char *name);
 
 struct drm_minor *drm_minor_acquire(unsigned int minor_id);
 void drm_minor_release(struct drm_minor *minor);
-- 
2.6.3

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

[PATCH 2/2] drm: use dev_name as default unique name in drm_dev_alloc()

[Nicolas Iooss]

The following code pattern exists in some DRM drivers:

    ddev = drm_dev_alloc(&driver, parent_dev);
    drm_dev_set_unique(ddev, dev_name(parent_dev));

(Sometimes dev_name(ddev->dev) is used, which is the same.)

As suggested in
http://lists.freedesktop.org/archives/dri-devel/2015-December/096441.html,
the unique name of a new DRM device can be set as dev_name(parent_dev)
when parent_dev is not NULL (vgem is a special case).

Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 4 ----
 drivers/gpu/drm/drm_drv.c                    | 9 +++++++++
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c    | 1 -
 drivers/gpu/drm/nouveau/nouveau_drm.c        | 4 ----
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c  | 4 ----
 drivers/gpu/drm/tegra/drm.c                  | 1 -
 drivers/gpu/drm/vc4/vc4_drv.c                | 2 --
 7 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
index 244df0a440b7..fba4f72e7ae1 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
@@ -733,10 +733,6 @@ static int atmel_hlcdc_dc_drm_probe(struct platform_device *pdev)
 	if (!ddev)
 		return -ENOMEM;
 
-	ret = drm_dev_set_unique(ddev, dev_name(ddev->dev));
-	if (ret)
-		goto err_unref;
-
 	ret = atmel_hlcdc_dc_load(ddev);
 	if (ret)
 		goto err_unref;
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 20eaa0aae205..df749a6156e0 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -633,8 +633,17 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver,
 		}
 	}
 
+	if (parent) {
+		ret = drm_dev_set_unique(dev, dev_name(parent));
+		if (ret)
+			goto err_setunique;
+	}
+
 	return dev;
 
+err_setunique:
+	if (drm_core_check_feature(dev, DRIVER_GEM))
+		drm_gem_destroy(dev);
 err_ctxbitmap:
 	drm_legacy_ctxbitmap_cleanup(dev);
 	drm_ht_remove(&dev->map_hash);
diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
index 1930234ba5f1..fca97d3fc846 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
@@ -363,7 +363,6 @@ static int fsl_dcu_drm_probe(struct platform_device *pdev)
 	fsl_dev->np = dev->of_node;
 	drm->dev_private = fsl_dev;
 	dev_set_drvdata(dev, fsl_dev);
-	drm_dev_set_unique(drm, dev_name(dev));
 
 	ret = drm_dev_register(drm, 0);
 	if (ret < 0)
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 2d23f95f17ce..b3a563c44bcd 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -1046,10 +1046,6 @@ nouveau_platform_device_create(const struct nvkm_device_tegra_func *func,
 		goto err_free;
 	}
 
-	err = drm_dev_set_unique(drm, dev_name(&pdev->dev));
-	if (err < 0)
-		goto err_free;
-
 	drm->platformdev = pdev;
 	platform_set_drvdata(pdev, drm);
 
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index 215d6c44af55..afbb7407c44f 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -450,10 +450,6 @@ static int rockchip_drm_bind(struct device *dev)
 	if (!drm)
 		return -ENOMEM;
 
-	ret = drm_dev_set_unique(drm, dev_name(dev));
-	if (ret)
-		goto err_free;
-
 	ret = drm_dev_register(drm, 0);
 	if (ret)
 		goto err_free;
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 159ef515cab1..12e2d3ccbc9d 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -991,7 +991,6 @@ static int host1x_drm_probe(struct host1x_device *dev)
 	if (!drm)
 		return -ENOMEM;
 
-	drm_dev_set_unique(drm, dev_name(&dev->dev));
 	dev_set_drvdata(&dev->dev, drm);
 
 	err = drm_dev_register(drm, 0);
diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index d5db9e0f3b73..647772305e8f 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -168,8 +168,6 @@ static int vc4_drm_bind(struct device *dev)
 	vc4->dev = drm;
 	drm->dev_private = vc4;
 
-	drm_dev_set_unique(drm, dev_name(dev));
-
 	drm_mode_config_init(drm);
 	if (ret)
 		goto unref;
-- 
2.6.3

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

Re: [PATCH 1/2] drm: make drm_dev_set_unique() not use a format string

[Emil Velikov]

On 8 December 2015 at 22:12, Nicolas Iooss <nicolas.iooss_linux@m4x.org> wrote:
> drm_dev_set_unique() uses a format string to define the unique name of a
> device.  This feature is not used as currently all the calls to this
> function either use "%s" as a format string or directly use
> dev_name().
>
> Even though this second kind of call does not introduce security
> problems, because there cannot be "%" characters in dev_name() results,
> gcc issues a warning when building with -Wformat-security flag
> ("warning: format string is not a string literal (potentially
> insecure)").  This warning is useful to find real bugs like the one
> fixed by commit 3958b79266b1 ("configfs: fix kernel infoleak through
> user-controlled format string").  False positives which do not bring
> an extra value make the work of finding real bugs harder.
>
> Therefore remove the format-string feature from drm_dev_set_unique().
>
> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
> ---
>  drivers/gpu/drm/drm_drv.c                   | 11 +++--------
>  drivers/gpu/drm/nouveau/nouveau_drm.c       |  2 +-
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c |  2 +-
>  include/drm/drmP.h                          |  2 +-
>  4 files changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 7dd6728dd092..20eaa0aae205 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -797,7 +797,7 @@ EXPORT_SYMBOL(drm_dev_unregister);
>  /**
>   * drm_dev_set_unique - Set the unique name of a DRM device
>   * @dev: device of which to set the unique name
> - * @fmt: format string for unique name
> + * @name: unique name
>   *
>   * Sets the unique name of a DRM device using the specified format string and
>   * a variable list of arguments. Drivers can use this at driver probe time if
You might want to also update the above hunk :-)

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

Re: [PATCH 1/2] drm: make drm_dev_set_unique() not use a format string

[Nicolas Iooss]

On 12/09/2015 12:28 AM, Emil Velikov wrote:
> On 8 December 2015 at 22:12, Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org> wrote:
>> drm_dev_set_unique() uses a format string to define the unique name of a
>> device.  This feature is not used as currently all the calls to this
>> function either use "%s" as a format string or directly use
>> dev_name().
>>
>> Even though this second kind of call does not introduce security
>> problems, because there cannot be "%" characters in dev_name() results,
>> gcc issues a warning when building with -Wformat-security flag
>> ("warning: format string is not a string literal (potentially
>> insecure)").  This warning is useful to find real bugs like the one
>> fixed by commit 3958b79266b1 ("configfs: fix kernel infoleak through
>> user-controlled format string").  False positives which do not bring
>> an extra value make the work of finding real bugs harder.
>>
>> Therefore remove the format-string feature from drm_dev_set_unique().
>>
>> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org>
>> ---
>>  drivers/gpu/drm/drm_drv.c                   | 11 +++--------
>>  drivers/gpu/drm/nouveau/nouveau_drm.c       |  2 +-
>>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c |  2 +-
>>  include/drm/drmP.h                          |  2 +-
>>  4 files changed, 6 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index 7dd6728dd092..20eaa0aae205 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -797,7 +797,7 @@ EXPORT_SYMBOL(drm_dev_unregister);
>>  /**
>>   * drm_dev_set_unique - Set the unique name of a DRM device
>>   * @dev: device of which to set the unique name
>> - * @fmt: format string for unique name
>> + * @name: unique name
>>   *
>>   * Sets the unique name of a DRM device using the specified format string and
>>   * a variable list of arguments. Drivers can use this at driver probe time if
> You might want to also update the above hunk :-)

Indeed, thanks! I will wait a little bit for other feedbacks, read all
the comments/documentation to see if anything else needs an update and
submit a v2.

Nicolas

Re: [PATCH 2/2] drm: use dev_name as default unique name in drm_dev_alloc()

[Boris Brezillon]

On Tue,  8 Dec 2015 23:12:12 +0100
Nicolas Iooss <nicolas.iooss_linux@m4x.org> wrote:

> The following code pattern exists in some DRM drivers:
> 
>     ddev = drm_dev_alloc(&driver, parent_dev);
>     drm_dev_set_unique(ddev, dev_name(parent_dev));
> 
> (Sometimes dev_name(ddev->dev) is used, which is the same.)
> 
> As suggested in
> http://lists.freedesktop.org/archives/dri-devel/2015-December/096441.html,
> the unique name of a new DRM device can be set as dev_name(parent_dev)
> when parent_dev is not NULL (vgem is a special case).
> 
> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>

[for atmel-hlcdc]
Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 4 ----
>  drivers/gpu/drm/drm_drv.c                    | 9 +++++++++
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c    | 1 -
>  drivers/gpu/drm/nouveau/nouveau_drm.c        | 4 ----
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c  | 4 ----
>  drivers/gpu/drm/tegra/drm.c                  | 1 -
>  drivers/gpu/drm/vc4/vc4_drv.c                | 2 --
>  7 files changed, 9 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> index 244df0a440b7..fba4f72e7ae1 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> @@ -733,10 +733,6 @@ static int atmel_hlcdc_dc_drm_probe(struct platform_device *pdev)
>  	if (!ddev)
>  		return -ENOMEM;
>  
> -	ret = drm_dev_set_unique(ddev, dev_name(ddev->dev));
> -	if (ret)
> -		goto err_unref;
> -
>  	ret = atmel_hlcdc_dc_load(ddev);
>  	if (ret)
>  		goto err_unref;
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 20eaa0aae205..df749a6156e0 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -633,8 +633,17 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver,
>  		}
>  	}
>  
> +	if (parent) {
> +		ret = drm_dev_set_unique(dev, dev_name(parent));
> +		if (ret)
> +			goto err_setunique;
> +	}
> +
>  	return dev;
>  
> +err_setunique:
> +	if (drm_core_check_feature(dev, DRIVER_GEM))
> +		drm_gem_destroy(dev);
>  err_ctxbitmap:
>  	drm_legacy_ctxbitmap_cleanup(dev);
>  	drm_ht_remove(&dev->map_hash);
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> index 1930234ba5f1..fca97d3fc846 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> @@ -363,7 +363,6 @@ static int fsl_dcu_drm_probe(struct platform_device *pdev)
>  	fsl_dev->np = dev->of_node;
>  	drm->dev_private = fsl_dev;
>  	dev_set_drvdata(dev, fsl_dev);
> -	drm_dev_set_unique(drm, dev_name(dev));
>  
>  	ret = drm_dev_register(drm, 0);
>  	if (ret < 0)
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index 2d23f95f17ce..b3a563c44bcd 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -1046,10 +1046,6 @@ nouveau_platform_device_create(const struct nvkm_device_tegra_func *func,
>  		goto err_free;
>  	}
>  
> -	err = drm_dev_set_unique(drm, dev_name(&pdev->dev));
> -	if (err < 0)
> -		goto err_free;
> -
>  	drm->platformdev = pdev;
>  	platform_set_drvdata(pdev, drm);
>  
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> index 215d6c44af55..afbb7407c44f 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> @@ -450,10 +450,6 @@ static int rockchip_drm_bind(struct device *dev)
>  	if (!drm)
>  		return -ENOMEM;
>  
> -	ret = drm_dev_set_unique(drm, dev_name(dev));
> -	if (ret)
> -		goto err_free;
> -
>  	ret = drm_dev_register(drm, 0);
>  	if (ret)
>  		goto err_free;
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index 159ef515cab1..12e2d3ccbc9d 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -991,7 +991,6 @@ static int host1x_drm_probe(struct host1x_device *dev)
>  	if (!drm)
>  		return -ENOMEM;
>  
> -	drm_dev_set_unique(drm, dev_name(&dev->dev));
>  	dev_set_drvdata(&dev->dev, drm);
>  
>  	err = drm_dev_register(drm, 0);
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
> index d5db9e0f3b73..647772305e8f 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.c
> +++ b/drivers/gpu/drm/vc4/vc4_drv.c
> @@ -168,8 +168,6 @@ static int vc4_drm_bind(struct device *dev)
>  	vc4->dev = drm;
>  	drm->dev_private = vc4;
>  
> -	drm_dev_set_unique(drm, dev_name(dev));
> -
>  	drm_mode_config_init(drm);
>  	if (ret)
>  		goto unref;



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm: make drm_dev_set_unique() not use a format string

[Daniel Vetter]

On Wed, Dec 09, 2015 at 12:52:58AM +0100, Nicolas Iooss wrote:
> On 12/09/2015 12:28 AM, Emil Velikov wrote:
> > On 8 December 2015 at 22:12, Nicolas Iooss <nicolas.iooss_linux@m4x.org> wrote:
> >> drm_dev_set_unique() uses a format string to define the unique name of a
> >> device.  This feature is not used as currently all the calls to this
> >> function either use "%s" as a format string or directly use
> >> dev_name().
> >>
> >> Even though this second kind of call does not introduce security
> >> problems, because there cannot be "%" characters in dev_name() results,
> >> gcc issues a warning when building with -Wformat-security flag
> >> ("warning: format string is not a string literal (potentially
> >> insecure)").  This warning is useful to find real bugs like the one
> >> fixed by commit 3958b79266b1 ("configfs: fix kernel infoleak through
> >> user-controlled format string").  False positives which do not bring
> >> an extra value make the work of finding real bugs harder.
> >>
> >> Therefore remove the format-string feature from drm_dev_set_unique().
> >>
> >> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
> >> ---
> >>  drivers/gpu/drm/drm_drv.c                   | 11 +++--------
> >>  drivers/gpu/drm/nouveau/nouveau_drm.c       |  2 +-
> >>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c |  2 +-
> >>  include/drm/drmP.h                          |  2 +-
> >>  4 files changed, 6 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> >> index 7dd6728dd092..20eaa0aae205 100644
> >> --- a/drivers/gpu/drm/drm_drv.c
> >> +++ b/drivers/gpu/drm/drm_drv.c
> >> @@ -797,7 +797,7 @@ EXPORT_SYMBOL(drm_dev_unregister);
> >>  /**
> >>   * drm_dev_set_unique - Set the unique name of a DRM device
> >>   * @dev: device of which to set the unique name
> >> - * @fmt: format string for unique name
> >> + * @name: unique name
> >>   *
> >>   * Sets the unique name of a DRM device using the specified format string and
> >>   * a variable list of arguments. Drivers can use this at driver probe time if
> > You might want to also update the above hunk :-)
> 
> Indeed, thanks! I will wait a little bit for other feedbacks, read all
> the comments/documentation to see if anything else needs an update and
> submit a v2.

fyi 4.5 window for drm is closing in the next few days (because holidays
and all that). Please resend soon, otherwise it might miss and get delayed
to 4.6.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH v2 1/2] drm: make drm_dev_set_unique() not use a format string

[Nicolas Iooss]

drm_dev_set_unique() uses a format string to define the unique name of a
device.  This feature is not used as currently all the calls to this
function either use "%s" as a format string or directly use
dev_name().

Even though this second kind of call does not introduce security
problems, because there cannot be "%" characters in dev_name() results,
gcc issues a warning when building with -Wformat-security flag
("warning: format string is not a string literal (potentially
insecure)").  This warning is useful to find real bugs like the one
fixed by commit 3958b79266b1 ("configfs: fix kernel infoleak through
user-controlled format string").  False positives which do not bring
an extra value make the work of finding real bugs harder.

Therefore remove the format-string feature from drm_dev_set_unique().

Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
---
v2: update drm_dev_set_unique() comment

 drivers/gpu/drm/drm_drv.c                   | 17 ++++++-----------
 drivers/gpu/drm/nouveau/nouveau_drm.c       |  2 +-
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c |  2 +-
 include/drm/drmP.h                          |  2 +-
 4 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 7dd6728dd092..eaa4316f3c45 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -797,23 +797,18 @@ EXPORT_SYMBOL(drm_dev_unregister);
 /**
  * drm_dev_set_unique - Set the unique name of a DRM device
  * @dev: device of which to set the unique name
- * @fmt: format string for unique name
+ * @name: unique name
  *
- * Sets the unique name of a DRM device using the specified format string and
- * a variable list of arguments. Drivers can use this at driver probe time if
- * the unique name of the devices they drive is static.
+ * Sets the unique name of a DRM device using the specified string. Drivers
+ * can use this at driver probe time if the unique name of the devices they
+ * drive is static.
  *
  * Return: 0 on success or a negative error code on failure.
  */
-int drm_dev_set_unique(struct drm_device *dev, const char *fmt, ...)
+int drm_dev_set_unique(struct drm_device *dev, const char *name)
 {
-	va_list ap;
-
 	kfree(dev->unique);
-
-	va_start(ap, fmt);
-	dev->unique = kvasprintf(GFP_KERNEL, fmt, ap);
-	va_end(ap);
+	dev->unique = kstrdup(name, GFP_KERNEL);
 
 	return dev->unique ? 0 : -ENOMEM;
 }
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 1d3ee5179ab8..2d23f95f17ce 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -1046,7 +1046,7 @@ nouveau_platform_device_create(const struct nvkm_device_tegra_func *func,
 		goto err_free;
 	}
 
-	err = drm_dev_set_unique(drm, "%s", dev_name(&pdev->dev));
+	err = drm_dev_set_unique(drm, dev_name(&pdev->dev));
 	if (err < 0)
 		goto err_free;
 
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index f22e1e1ee64a..215d6c44af55 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -450,7 +450,7 @@ static int rockchip_drm_bind(struct device *dev)
 	if (!drm)
 		return -ENOMEM;
 
-	ret = drm_dev_set_unique(drm, "%s", dev_name(dev));
+	ret = drm_dev_set_unique(drm, dev_name(dev));
 	if (ret)
 		goto err_free;
 
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 0a271ca1f7c7..f14c25a6bbf2 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1059,7 +1059,7 @@ void drm_dev_ref(struct drm_device *dev);
 void drm_dev_unref(struct drm_device *dev);
 int drm_dev_register(struct drm_device *dev, unsigned long flags);
 void drm_dev_unregister(struct drm_device *dev);
-int drm_dev_set_unique(struct drm_device *dev, const char *fmt, ...);
+int drm_dev_set_unique(struct drm_device *dev, const char *name);
 
 struct drm_minor *drm_minor_acquire(unsigned int minor_id);
 void drm_minor_release(struct drm_minor *minor);
-- 
2.6.3

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

[PATCH v2 2/2] drm: use dev_name as default unique name in drm_dev_alloc()

[Nicolas Iooss]

The following code pattern exists in some DRM drivers:

    ddev = drm_dev_alloc(&driver, parent_dev);
    drm_dev_set_unique(ddev, dev_name(parent_dev));

(Sometimes dev_name(ddev->dev) is used, which is the same.)

As suggested in
http://lists.freedesktop.org/archives/dri-devel/2015-December/096441.html,
the unique name of a new DRM device can be set as dev_name(parent_dev)
when parent_dev is not NULL (vgem is a special case).

Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 4 ----
 drivers/gpu/drm/drm_drv.c                    | 9 +++++++++
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c    | 1 -
 drivers/gpu/drm/nouveau/nouveau_drm.c        | 4 ----
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c  | 4 ----
 drivers/gpu/drm/tegra/drm.c                  | 1 -
 drivers/gpu/drm/vc4/vc4_drv.c                | 2 --
 7 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
index 244df0a440b7..fba4f72e7ae1 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
@@ -733,10 +733,6 @@ static int atmel_hlcdc_dc_drm_probe(struct platform_device *pdev)
 	if (!ddev)
 		return -ENOMEM;
 
-	ret = drm_dev_set_unique(ddev, dev_name(ddev->dev));
-	if (ret)
-		goto err_unref;
-
 	ret = atmel_hlcdc_dc_load(ddev);
 	if (ret)
 		goto err_unref;
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index eaa4316f3c45..bf934cdea21c 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -633,8 +633,17 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver,
 		}
 	}
 
+	if (parent) {
+		ret = drm_dev_set_unique(dev, dev_name(parent));
+		if (ret)
+			goto err_setunique;
+	}
+
 	return dev;
 
+err_setunique:
+	if (drm_core_check_feature(dev, DRIVER_GEM))
+		drm_gem_destroy(dev);
 err_ctxbitmap:
 	drm_legacy_ctxbitmap_cleanup(dev);
 	drm_ht_remove(&dev->map_hash);
diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
index 1930234ba5f1..fca97d3fc846 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
@@ -363,7 +363,6 @@ static int fsl_dcu_drm_probe(struct platform_device *pdev)
 	fsl_dev->np = dev->of_node;
 	drm->dev_private = fsl_dev;
 	dev_set_drvdata(dev, fsl_dev);
-	drm_dev_set_unique(drm, dev_name(dev));
 
 	ret = drm_dev_register(drm, 0);
 	if (ret < 0)
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 2d23f95f17ce..b3a563c44bcd 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -1046,10 +1046,6 @@ nouveau_platform_device_create(const struct nvkm_device_tegra_func *func,
 		goto err_free;
 	}
 
-	err = drm_dev_set_unique(drm, dev_name(&pdev->dev));
-	if (err < 0)
-		goto err_free;
-
 	drm->platformdev = pdev;
 	platform_set_drvdata(pdev, drm);
 
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index 215d6c44af55..afbb7407c44f 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -450,10 +450,6 @@ static int rockchip_drm_bind(struct device *dev)
 	if (!drm)
 		return -ENOMEM;
 
-	ret = drm_dev_set_unique(drm, dev_name(dev));
-	if (ret)
-		goto err_free;
-
 	ret = drm_dev_register(drm, 0);
 	if (ret)
 		goto err_free;
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 159ef515cab1..12e2d3ccbc9d 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -991,7 +991,6 @@ static int host1x_drm_probe(struct host1x_device *dev)
 	if (!drm)
 		return -ENOMEM;
 
-	drm_dev_set_unique(drm, dev_name(&dev->dev));
 	dev_set_drvdata(&dev->dev, drm);
 
 	err = drm_dev_register(drm, 0);
diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index d5db9e0f3b73..647772305e8f 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -168,8 +168,6 @@ static int vc4_drm_bind(struct device *dev)
 	vc4->dev = drm;
 	drm->dev_private = vc4;
 
-	drm_dev_set_unique(drm, dev_name(dev));
-
 	drm_mode_config_init(drm);
 	if (ret)
 		goto unref;
-- 
2.6.3

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

Re: [PATCH v2 2/2] drm: use dev_name as default unique name in drm_dev_alloc()

[Daniel Vetter]

On Fri, Dec 11, 2015 at 11:20:28AM +0100, Nicolas Iooss wrote:
> The following code pattern exists in some DRM drivers:
> 
>     ddev = drm_dev_alloc(&driver, parent_dev);
>     drm_dev_set_unique(ddev, dev_name(parent_dev));
> 
> (Sometimes dev_name(ddev->dev) is used, which is the same.)
> 
> As suggested in
> http://lists.freedesktop.org/archives/dri-devel/2015-December/096441.html,
> the unique name of a new DRM device can be set as dev_name(parent_dev)
> when parent_dev is not NULL (vgem is a special case).
> 
> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
> Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>

Pulled these two in, but there's some other drivers floating around and
heading towards 4.5 with which it will conflict. If it's causing too much
trouble I'll drop it again and we have to get it in right around 4.5-rc1
to avoid trouble with new drivers.

Thanks, Daniel

> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 4 ----
>  drivers/gpu/drm/drm_drv.c                    | 9 +++++++++
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c    | 1 -
>  drivers/gpu/drm/nouveau/nouveau_drm.c        | 4 ----
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c  | 4 ----
>  drivers/gpu/drm/tegra/drm.c                  | 1 -
>  drivers/gpu/drm/vc4/vc4_drv.c                | 2 --
>  7 files changed, 9 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> index 244df0a440b7..fba4f72e7ae1 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> @@ -733,10 +733,6 @@ static int atmel_hlcdc_dc_drm_probe(struct platform_device *pdev)
>  	if (!ddev)
>  		return -ENOMEM;
>  
> -	ret = drm_dev_set_unique(ddev, dev_name(ddev->dev));
> -	if (ret)
> -		goto err_unref;
> -
>  	ret = atmel_hlcdc_dc_load(ddev);
>  	if (ret)
>  		goto err_unref;
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index eaa4316f3c45..bf934cdea21c 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -633,8 +633,17 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver,
>  		}
>  	}
>  
> +	if (parent) {
> +		ret = drm_dev_set_unique(dev, dev_name(parent));
> +		if (ret)
> +			goto err_setunique;
> +	}
> +
>  	return dev;
>  
> +err_setunique:
> +	if (drm_core_check_feature(dev, DRIVER_GEM))
> +		drm_gem_destroy(dev);
>  err_ctxbitmap:
>  	drm_legacy_ctxbitmap_cleanup(dev);
>  	drm_ht_remove(&dev->map_hash);
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> index 1930234ba5f1..fca97d3fc846 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> @@ -363,7 +363,6 @@ static int fsl_dcu_drm_probe(struct platform_device *pdev)
>  	fsl_dev->np = dev->of_node;
>  	drm->dev_private = fsl_dev;
>  	dev_set_drvdata(dev, fsl_dev);
> -	drm_dev_set_unique(drm, dev_name(dev));
>  
>  	ret = drm_dev_register(drm, 0);
>  	if (ret < 0)
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index 2d23f95f17ce..b3a563c44bcd 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -1046,10 +1046,6 @@ nouveau_platform_device_create(const struct nvkm_device_tegra_func *func,
>  		goto err_free;
>  	}
>  
> -	err = drm_dev_set_unique(drm, dev_name(&pdev->dev));
> -	if (err < 0)
> -		goto err_free;
> -
>  	drm->platformdev = pdev;
>  	platform_set_drvdata(pdev, drm);
>  
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> index 215d6c44af55..afbb7407c44f 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> @@ -450,10 +450,6 @@ static int rockchip_drm_bind(struct device *dev)
>  	if (!drm)
>  		return -ENOMEM;
>  
> -	ret = drm_dev_set_unique(drm, dev_name(dev));
> -	if (ret)
> -		goto err_free;
> -
>  	ret = drm_dev_register(drm, 0);
>  	if (ret)
>  		goto err_free;
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index 159ef515cab1..12e2d3ccbc9d 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -991,7 +991,6 @@ static int host1x_drm_probe(struct host1x_device *dev)
>  	if (!drm)
>  		return -ENOMEM;
>  
> -	drm_dev_set_unique(drm, dev_name(&dev->dev));
>  	dev_set_drvdata(&dev->dev, drm);
>  
>  	err = drm_dev_register(drm, 0);
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
> index d5db9e0f3b73..647772305e8f 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.c
> +++ b/drivers/gpu/drm/vc4/vc4_drv.c
> @@ -168,8 +168,6 @@ static int vc4_drm_bind(struct device *dev)
>  	vc4->dev = drm;
>  	drm->dev_private = vc4;
>  
> -	drm_dev_set_unique(drm, dev_name(dev));
> -
>  	drm_mode_config_init(drm);
>  	if (ret)
>  		goto unref;
> -- 
> 2.6.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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