[PATCH] drm: do not use device name as a format string

[PATCH] drm: do not use device name as a format string

[Nicolas Iooss]

drm_dev_set_unique() formats its parameter using kvasprintf() but many
of its callers directly pass dev_name(dev) as printf format string,
without any format parameter.  This can cause some issues when the
device name contains '%' characters.

To avoid any potential issue, always use "%s" when using
drm_dev_set_unique() with dev_name().

Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 2 +-
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c    | 2 +-
 drivers/gpu/drm/tegra/drm.c                  | 2 +-
 drivers/gpu/drm/vc4/vc4_drv.c                | 2 +-
 include/drm/drmP.h                           | 1 +
 5 files changed, 5 insertions(+), 4 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..0d720d3a7ee0 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
@@ -733,7 +733,7 @@ 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));
+	ret = drm_dev_set_unique(ddev, "%s", dev_name(ddev->dev));
 	if (ret)
 		goto err_unref;
 
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..947d75f59881 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,7 @@ 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));
+	drm_dev_set_unique(drm, "%s", dev_name(dev));
 
 	ret = drm_dev_register(drm, 0);
 	if (ret < 0)
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 159ef515cab1..b278f60f4376 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -991,7 +991,7 @@ static int host1x_drm_probe(struct host1x_device *dev)
 	if (!drm)
 		return -ENOMEM;
 
-	drm_dev_set_unique(drm, dev_name(&dev->dev));
+	drm_dev_set_unique(drm, "%s", 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 6e730605edcc..c90a451aaa79 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -168,7 +168,7 @@ static int vc4_drm_bind(struct device *dev)
 	vc4->dev = drm;
 	drm->dev_private = vc4;
 
-	drm_dev_set_unique(drm, dev_name(dev));
+	drm_dev_set_unique(drm, "%s", dev_name(dev));
 
 	drm_mode_config_init(drm);
 	if (ret)
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 0b921ae06cd8..995fb96cb740 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1049,6 +1049,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);
+__printf(2, 3)
 int drm_dev_set_unique(struct drm_device *dev, const char *fmt, ...);
 
 struct drm_minor *drm_minor_acquire(unsigned int minor_id);
-- 
2.6.2

Re: [PATCH] drm: do not use device name as a format string

[Nicolas Iooss]

Hello,
I sent the path below a few weeks ago and did not have any feedback.
Is there any issue in it that I need to fix before submitting it again?

Thanks,
Nicolas Iooss

On 11/18/2015 06:58 PM, Nicolas Iooss wrote:
> drm_dev_set_unique() formats its parameter using kvasprintf() but many
> of its callers directly pass dev_name(dev) as printf format string,
> without any format parameter.  This can cause some issues when the
> device name contains '%' characters.
> 
> To avoid any potential issue, always use "%s" when using
> drm_dev_set_unique() with dev_name().
> 
> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 2 +-
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c    | 2 +-
>  drivers/gpu/drm/tegra/drm.c                  | 2 +-
>  drivers/gpu/drm/vc4/vc4_drv.c                | 2 +-
>  include/drm/drmP.h                           | 1 +
>  5 files changed, 5 insertions(+), 4 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..0d720d3a7ee0 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> @@ -733,7 +733,7 @@ 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));
> +	ret = drm_dev_set_unique(ddev, "%s", dev_name(ddev->dev));
>  	if (ret)
>  		goto err_unref;
>  
> 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..947d75f59881 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,7 @@ 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));
> +	drm_dev_set_unique(drm, "%s", dev_name(dev));
>  
>  	ret = drm_dev_register(drm, 0);
>  	if (ret < 0)
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index 159ef515cab1..b278f60f4376 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -991,7 +991,7 @@ static int host1x_drm_probe(struct host1x_device *dev)
>  	if (!drm)
>  		return -ENOMEM;
>  
> -	drm_dev_set_unique(drm, dev_name(&dev->dev));
> +	drm_dev_set_unique(drm, "%s", 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 6e730605edcc..c90a451aaa79 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.c
> +++ b/drivers/gpu/drm/vc4/vc4_drv.c
> @@ -168,7 +168,7 @@ static int vc4_drm_bind(struct device *dev)
>  	vc4->dev = drm;
>  	drm->dev_private = vc4;
>  
> -	drm_dev_set_unique(drm, dev_name(dev));
> +	drm_dev_set_unique(drm, "%s", dev_name(dev));
>  
>  	drm_mode_config_init(drm);
>  	if (ret)
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 0b921ae06cd8..995fb96cb740 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1049,6 +1049,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);
> +__printf(2, 3)
>  int drm_dev_set_unique(struct drm_device *dev, const char *fmt, ...);
>  
>  struct drm_minor *drm_minor_acquire(unsigned int minor_id);
> 

Re: [PATCH] drm: do not use device name as a format string

[Daniel Vetter]

On Sat, Dec 05, 2015 at 10:45:50AM +0100, Nicolas Iooss wrote:
> Hello,
> I sent the path below a few weeks ago and did not have any feedback.
> Is there any issue in it that I need to fix before submitting it again?

Sorry, must have missed this.

> 
> Thanks,
> Nicolas Iooss
> 
> On 11/18/2015 06:58 PM, Nicolas Iooss wrote:
> > drm_dev_set_unique() formats its parameter using kvasprintf() but many
> > of its callers directly pass dev_name(dev) as printf format string,
> > without any format parameter.  This can cause some issues when the
> > device name contains '%' characters.
> > 
> > To avoid any potential issue, always use "%s" when using
> > drm_dev_set_unique() with dev_name().

Not sure this is worth it really, normally people don't place % characters
into their device names, ever. And if they do it'll blow up. There's also
no security issue here since userspace can't set this name.

If the maintainers of the affected drivers don't want this I won't merge
this patch.

Thanks, Daniel

> > 
> > Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
> > ---
> >  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 2 +-
> >  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c    | 2 +-
> >  drivers/gpu/drm/tegra/drm.c                  | 2 +-
> >  drivers/gpu/drm/vc4/vc4_drv.c                | 2 +-
> >  include/drm/drmP.h                           | 1 +
> >  5 files changed, 5 insertions(+), 4 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..0d720d3a7ee0 100644
> > --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> > +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> > @@ -733,7 +733,7 @@ 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));
> > +	ret = drm_dev_set_unique(ddev, "%s", dev_name(ddev->dev));
> >  	if (ret)
> >  		goto err_unref;
> >  
> > 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..947d75f59881 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,7 @@ 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));
> > +	drm_dev_set_unique(drm, "%s", dev_name(dev));
> >  
> >  	ret = drm_dev_register(drm, 0);
> >  	if (ret < 0)
> > diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> > index 159ef515cab1..b278f60f4376 100644
> > --- a/drivers/gpu/drm/tegra/drm.c
> > +++ b/drivers/gpu/drm/tegra/drm.c
> > @@ -991,7 +991,7 @@ static int host1x_drm_probe(struct host1x_device *dev)
> >  	if (!drm)
> >  		return -ENOMEM;
> >  
> > -	drm_dev_set_unique(drm, dev_name(&dev->dev));
> > +	drm_dev_set_unique(drm, "%s", 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 6e730605edcc..c90a451aaa79 100644
> > --- a/drivers/gpu/drm/vc4/vc4_drv.c
> > +++ b/drivers/gpu/drm/vc4/vc4_drv.c
> > @@ -168,7 +168,7 @@ static int vc4_drm_bind(struct device *dev)
> >  	vc4->dev = drm;
> >  	drm->dev_private = vc4;
> >  
> > -	drm_dev_set_unique(drm, dev_name(dev));
> > +	drm_dev_set_unique(drm, "%s", dev_name(dev));
> >  
> >  	drm_mode_config_init(drm);
> >  	if (ret)
> > diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> > index 0b921ae06cd8..995fb96cb740 100644
> > --- a/include/drm/drmP.h
> > +++ b/include/drm/drmP.h
> > @@ -1049,6 +1049,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);
> > +__printf(2, 3)
> >  int drm_dev_set_unique(struct drm_device *dev, const char *fmt, ...);
> >  
> >  struct drm_minor *drm_minor_acquire(unsigned int minor_id);
> > 
> 
> _______________________________________________
> 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

Re: [PATCH] drm: do not use device name as a format string

[Nicolas Iooss]

On 12/06/2015 10:35 AM, Daniel Vetter wrote:
>> On 11/18/2015 06:58 PM, Nicolas Iooss wrote:
>>> drm_dev_set_unique() formats its parameter using kvasprintf() but many
>>> of its callers directly pass dev_name(dev) as printf format string,
>>> without any format parameter.  This can cause some issues when the
>>> device name contains '%' characters.
>>>
>>> To avoid any potential issue, always use "%s" when using
>>> drm_dev_set_unique() with dev_name().
> 
> Not sure this is worth it really, normally people don't place % characters
> into their device names, ever. And if they do it'll blow up. There's also
> no security issue here since userspace can't set this name.
> 
> If the maintainers of the affected drivers don't want this I won't merge
> this patch.

Actually I had the same opinion before I began to add __printf
attributes and "%s" in several places in the kernel to make
-Wformat-security useful.  This led me to discover some funny issues
like the one fixed by commit 3958b79266b1 ("configfs: fix kernel
infoleak through user-controlled format string",
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=3958b79266b14729edd61daf9dfb84de45f4ec6d
).  The patch I sent is in fact a very small step towards making
-Wformat-security useful again to detect "real" issues.

Of course, if you do not feel it is worth it and believe that dev_name
is fully controlled by trusted sources which will never introduce any %
character, I understand your will of not merging my patch.

Regards,
Nicolas

Re: [PATCH] drm: do not use device name as a format string

[Daniel Vetter]

On Sun, Dec 06, 2015 at 11:16:32AM +0100, Nicolas Iooss wrote:
> On 12/06/2015 10:35 AM, Daniel Vetter wrote:
> >> On 11/18/2015 06:58 PM, Nicolas Iooss wrote:
> >>> drm_dev_set_unique() formats its parameter using kvasprintf() but many
> >>> of its callers directly pass dev_name(dev) as printf format string,
> >>> without any format parameter.  This can cause some issues when the
> >>> device name contains '%' characters.
> >>>
> >>> To avoid any potential issue, always use "%s" when using
> >>> drm_dev_set_unique() with dev_name().
> > 
> > Not sure this is worth it really, normally people don't place % characters
> > into their device names, ever. And if they do it'll blow up. There's also
> > no security issue here since userspace can't set this name.
> > 
> > If the maintainers of the affected drivers don't want this I won't merge
> > this patch.
> 
> Actually I had the same opinion before I began to add __printf
> attributes and "%s" in several places in the kernel to make
> -Wformat-security useful.  This led me to discover some funny issues
> like the one fixed by commit 3958b79266b1 ("configfs: fix kernel
> infoleak through user-controlled format string",
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=3958b79266b14729edd61daf9dfb84de45f4ec6d
> ).  The patch I sent is in fact a very small step towards making
> -Wformat-security useful again to detect "real" issues.
> 
> Of course, if you do not feel it is worth it and believe that dev_name
> is fully controlled by trusted sources which will never introduce any %
> character, I understand your will of not merging my patch.

Ah, that's the context I was missing, that really should be in the commit
message. If this is part of an overall effort to enable something useful
it makes more sense to get it in.

On the patch itself it feels rather funny to do a "%s", str); combo, maybe
we should have a drm_dev_set_unique_static instead? Including kerneldoc
explaining why there's too.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm: do not use device name as a format string

[Jani Nikula]

On Mon, 07 Dec 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Sun, Dec 06, 2015 at 11:16:32AM +0100, Nicolas Iooss wrote:
>> On 12/06/2015 10:35 AM, Daniel Vetter wrote:
>> >> On 11/18/2015 06:58 PM, Nicolas Iooss wrote:
>> >>> drm_dev_set_unique() formats its parameter using kvasprintf() but many
>> >>> of its callers directly pass dev_name(dev) as printf format string,
>> >>> without any format parameter.  This can cause some issues when the
>> >>> device name contains '%' characters.
>> >>>
>> >>> To avoid any potential issue, always use "%s" when using
>> >>> drm_dev_set_unique() with dev_name().
>> > 
>> > Not sure this is worth it really, normally people don't place % characters
>> > into their device names, ever. And if they do it'll blow up. There's also
>> > no security issue here since userspace can't set this name.
>> > 
>> > If the maintainers of the affected drivers don't want this I won't merge
>> > this patch.
>> 
>> Actually I had the same opinion before I began to add __printf
>> attributes and "%s" in several places in the kernel to make
>> -Wformat-security useful.  This led me to discover some funny issues
>> like the one fixed by commit 3958b79266b1 ("configfs: fix kernel
>> infoleak through user-controlled format string",
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=3958b79266b14729edd61daf9dfb84de45f4ec6d
>> ).  The patch I sent is in fact a very small step towards making
>> -Wformat-security useful again to detect "real" issues.
>> 
>> Of course, if you do not feel it is worth it and believe that dev_name
>> is fully controlled by trusted sources which will never introduce any %
>> character, I understand your will of not merging my patch.
>
> Ah, that's the context I was missing, that really should be in the commit
> message. If this is part of an overall effort to enable something useful
> it makes more sense to get it in.
>
> On the patch itself it feels rather funny to do a "%s", str); combo, maybe
> we should have a drm_dev_set_unique_static instead? Including kerneldoc
> explaining why there's too.

No caller of drm_dev_set_unique() actually uses the formatting for
anything... so you'd end up with drm_dev_set_unique_static() and an
orphaned drm_dev_set_unique()...

BR,
Jani.



-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH] drm: do not use device name as a format string

[Daniel Vetter]

On Mon, Dec 07, 2015 at 11:53:01AM +0200, Jani Nikula wrote:
> On Mon, 07 Dec 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Sun, Dec 06, 2015 at 11:16:32AM +0100, Nicolas Iooss wrote:
> >> On 12/06/2015 10:35 AM, Daniel Vetter wrote:
> >> >> On 11/18/2015 06:58 PM, Nicolas Iooss wrote:
> >> >>> drm_dev_set_unique() formats its parameter using kvasprintf() but many
> >> >>> of its callers directly pass dev_name(dev) as printf format string,
> >> >>> without any format parameter.  This can cause some issues when the
> >> >>> device name contains '%' characters.
> >> >>>
> >> >>> To avoid any potential issue, always use "%s" when using
> >> >>> drm_dev_set_unique() with dev_name().
> >> > 
> >> > Not sure this is worth it really, normally people don't place % characters
> >> > into their device names, ever. And if they do it'll blow up. There's also
> >> > no security issue here since userspace can't set this name.
> >> > 
> >> > If the maintainers of the affected drivers don't want this I won't merge
> >> > this patch.
> >> 
> >> Actually I had the same opinion before I began to add __printf
> >> attributes and "%s" in several places in the kernel to make
> >> -Wformat-security useful.  This led me to discover some funny issues
> >> like the one fixed by commit 3958b79266b1 ("configfs: fix kernel
> >> infoleak through user-controlled format string",
> >> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=3958b79266b14729edd61daf9dfb84de45f4ec6d
> >> ).  The patch I sent is in fact a very small step towards making
> >> -Wformat-security useful again to detect "real" issues.
> >> 
> >> Of course, if you do not feel it is worth it and believe that dev_name
> >> is fully controlled by trusted sources which will never introduce any %
> >> character, I understand your will of not merging my patch.
> >
> > Ah, that's the context I was missing, that really should be in the commit
> > message. If this is part of an overall effort to enable something useful
> > it makes more sense to get it in.
> >
> > On the patch itself it feels rather funny to do a "%s", str); combo, maybe
> > we should have a drm_dev_set_unique_static instead? Including kerneldoc
> > explaining why there's too.
> 
> No caller of drm_dev_set_unique() actually uses the formatting for
> anything... so you'd end up with drm_dev_set_unique_static() and an
> orphaned drm_dev_set_unique()...

Ok, then I guess we can just ditch the printf stuff from set_unique.
Nicolas, you're up for that?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm: do not use device name as a format string

[Boris Brezillon]

On Wed, 18 Nov 2015 18:58:18 +0100
Nicolas Iooss <nicolas.iooss_linux@m4x.org> wrote:

> drm_dev_set_unique() formats its parameter using kvasprintf() but many
> of its callers directly pass dev_name(dev) as printf format string,
> without any format parameter.  This can cause some issues when the
> device name contains '%' characters.
> 
> To avoid any potential issue, always use "%s" when using
> drm_dev_set_unique() with dev_name().
> 
> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>

I'll let Daniel decide whether it's relevant or not to add a new
function/macro to hide this, but I'm fine with the current patch.

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

> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 2 +-
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c    | 2 +-
>  drivers/gpu/drm/tegra/drm.c                  | 2 +-
>  drivers/gpu/drm/vc4/vc4_drv.c                | 2 +-
>  include/drm/drmP.h                           | 1 +
>  5 files changed, 5 insertions(+), 4 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..0d720d3a7ee0 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> @@ -733,7 +733,7 @@ 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));
> +	ret = drm_dev_set_unique(ddev, "%s", dev_name(ddev->dev));
>  	if (ret)
>  		goto err_unref;
>  
> 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..947d75f59881 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,7 @@ 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));
> +	drm_dev_set_unique(drm, "%s", dev_name(dev));
>  
>  	ret = drm_dev_register(drm, 0);
>  	if (ret < 0)
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index 159ef515cab1..b278f60f4376 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -991,7 +991,7 @@ static int host1x_drm_probe(struct host1x_device *dev)
>  	if (!drm)
>  		return -ENOMEM;
>  
> -	drm_dev_set_unique(drm, dev_name(&dev->dev));
> +	drm_dev_set_unique(drm, "%s", 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 6e730605edcc..c90a451aaa79 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.c
> +++ b/drivers/gpu/drm/vc4/vc4_drv.c
> @@ -168,7 +168,7 @@ static int vc4_drm_bind(struct device *dev)
>  	vc4->dev = drm;
>  	drm->dev_private = vc4;
>  
> -	drm_dev_set_unique(drm, dev_name(dev));
> +	drm_dev_set_unique(drm, "%s", dev_name(dev));
>  
>  	drm_mode_config_init(drm);
>  	if (ret)
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 0b921ae06cd8..995fb96cb740 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1049,6 +1049,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);
> +__printf(2, 3)
>  int drm_dev_set_unique(struct drm_device *dev, const char *fmt, ...);
>  
>  struct drm_minor *drm_minor_acquire(unsigned int minor_id);



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH] drm: do not use device name as a format string

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3190 bytes --]

On Mon, Dec 07, 2015 at 12:46:52PM +0100, Daniel Vetter wrote:
> On Mon, Dec 07, 2015 at 11:53:01AM +0200, Jani Nikula wrote:
> > On Mon, 07 Dec 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > On Sun, Dec 06, 2015 at 11:16:32AM +0100, Nicolas Iooss wrote:
> > >> On 12/06/2015 10:35 AM, Daniel Vetter wrote:
> > >> >> On 11/18/2015 06:58 PM, Nicolas Iooss wrote:
> > >> >>> drm_dev_set_unique() formats its parameter using kvasprintf() but many
> > >> >>> of its callers directly pass dev_name(dev) as printf format string,
> > >> >>> without any format parameter.  This can cause some issues when the
> > >> >>> device name contains '%' characters.
> > >> >>>
> > >> >>> To avoid any potential issue, always use "%s" when using
> > >> >>> drm_dev_set_unique() with dev_name().
> > >> > 
> > >> > Not sure this is worth it really, normally people don't place % characters
> > >> > into their device names, ever. And if they do it'll blow up. There's also
> > >> > no security issue here since userspace can't set this name.
> > >> > 
> > >> > If the maintainers of the affected drivers don't want this I won't merge
> > >> > this patch.
> > >> 
> > >> Actually I had the same opinion before I began to add __printf
> > >> attributes and "%s" in several places in the kernel to make
> > >> -Wformat-security useful.  This led me to discover some funny issues
> > >> like the one fixed by commit 3958b79266b1 ("configfs: fix kernel
> > >> infoleak through user-controlled format string",
> > >> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=3958b79266b14729edd61daf9dfb84de45f4ec6d
> > >> ).  The patch I sent is in fact a very small step towards making
> > >> -Wformat-security useful again to detect "real" issues.
> > >> 
> > >> Of course, if you do not feel it is worth it and believe that dev_name
> > >> is fully controlled by trusted sources which will never introduce any %
> > >> character, I understand your will of not merging my patch.
> > >
> > > Ah, that's the context I was missing, that really should be in the commit
> > > message. If this is part of an overall effort to enable something useful
> > > it makes more sense to get it in.
> > >
> > > On the patch itself it feels rather funny to do a "%s", str); combo, maybe
> > > we should have a drm_dev_set_unique_static instead? Including kerneldoc
> > > explaining why there's too.
> > 
> > No caller of drm_dev_set_unique() actually uses the formatting for
> > anything... so you'd end up with drm_dev_set_unique_static() and an
> > orphaned drm_dev_set_unique()...
> 
> Ok, then I guess we can just ditch the printf stuff from set_unique.
> Nicolas, you're up for that?

Looking at all the callsites of drm_dev_set_unique() it seems like all
of the drivers (with the exception of vgem) use dev_name() on the same
device that's already passed into drm_dev_alloc(), so perhaps another
alternative would be to have drm_dev_alloc() set the unique name by
default and keep the function for cases where it needs to be set
explicitly (like for vgem). vgem passes drm_dev_alloc() a NULL device,
so that could serve as condition.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] drm: do not use device name as a format string

[Nicolas Iooss]

On 12/07/2015 01:31 PM, Thierry Reding wrote:
> On Mon, Dec 07, 2015 at 12:46:52PM +0100, Daniel Vetter wrote:
>> On Mon, Dec 07, 2015 at 11:53:01AM +0200, Jani Nikula wrote:
>>> On Mon, 07 Dec 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
>>>> On Sun, Dec 06, 2015 at 11:16:32AM +0100, Nicolas Iooss wrote:
>>>>> On 12/06/2015 10:35 AM, Daniel Vetter wrote:
>>>>>>> On 11/18/2015 06:58 PM, Nicolas Iooss wrote:
>>>>>>>> drm_dev_set_unique() formats its parameter using kvasprintf() but many
>>>>>>>> of its callers directly pass dev_name(dev) as printf format string,
>>>>>>>> without any format parameter.  This can cause some issues when the
>>>>>>>> device name contains '%' characters.
>>>>>>>>
>>>>>>>> To avoid any potential issue, always use "%s" when using
>>>>>>>> drm_dev_set_unique() with dev_name().
>>>>>>
>>>>>> Not sure this is worth it really, normally people don't place % characters
>>>>>> into their device names, ever. And if they do it'll blow up. There's also
>>>>>> no security issue here since userspace can't set this name.
>>>>>>
>>>>>> If the maintainers of the affected drivers don't want this I won't merge
>>>>>> this patch.
>>>>>
>>>>> Actually I had the same opinion before I began to add __printf
>>>>> attributes and "%s" in several places in the kernel to make
>>>>> -Wformat-security useful.  This led me to discover some funny issues
>>>>> like the one fixed by commit 3958b79266b1 ("configfs: fix kernel
>>>>> infoleak through user-controlled format string",
>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=3958b79266b14729edd61daf9dfb84de45f4ec6d
>>>>> ).  The patch I sent is in fact a very small step towards making
>>>>> -Wformat-security useful again to detect "real" issues.
>>>>>
>>>>> Of course, if you do not feel it is worth it and believe that dev_name
>>>>> is fully controlled by trusted sources which will never introduce any %
>>>>> character, I understand your will of not merging my patch.
>>>>
>>>> Ah, that's the context I was missing, that really should be in the commit
>>>> message. If this is part of an overall effort to enable something useful
>>>> it makes more sense to get it in.
>>>>
>>>> On the patch itself it feels rather funny to do a "%s", str); combo, maybe
>>>> we should have a drm_dev_set_unique_static instead? Including kerneldoc
>>>> explaining why there's too.
>>>
>>> No caller of drm_dev_set_unique() actually uses the formatting for
>>> anything... so you'd end up with drm_dev_set_unique_static() and an
>>> orphaned drm_dev_set_unique()...
>>
>> Ok, then I guess we can just ditch the printf stuff from set_unique.
>> Nicolas, you're up for that?
> 
> Looking at all the callsites of drm_dev_set_unique() it seems like all
> of the drivers (with the exception of vgem) use dev_name() on the same
> device that's already passed into drm_dev_alloc(), so perhaps another
> alternative would be to have drm_dev_alloc() set the unique name by
> default and keep the function for cases where it needs to be set
> explicitly (like for vgem). vgem passes drm_dev_alloc() a NULL device,
> so that could serve as condition.

I have written a patch which removes the printf format processing from
drm_dev_set_unique().  I will test it as soon as possible and depending
on the results, send it or explain what went wrong.  If no one does the
work before me, I'll also take a look at calling drm_dev_set_unique() in
drm_dev_alloc(), and this would be an other patch.

Thanks,
Nicolas