| Message ID | 1447869498-13277-1-git-send-email-nicolas.iooss_linux@m4x.org |
|---|---|
| State | Deferred |
| Headers | show |
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); > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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.
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
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);
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(-)