All of lore.kernel.org
 help / color / mirror / Atom feed
From: Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org>
To: "Boris Brezillon"
	<boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>,
	"David Airlie" <airlied-cv59FeDIM0c@public.gmane.org>,
	"Jianwei Wang"
	<jianwei.wang.chn-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>,
	"Alison Wang"
	<alison.wang-KZfg59tc24xl57MIdRCFDg@public.gmane.org>,
	"Thierry Reding"
	<thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>,
	"Terje Bergström"
	<tbergstrom-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>,
	dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: Re: [PATCH] drm: do not use device name as a format string
Date: Sun, 6 Dec 2015 11:16:32 +0100	[thread overview]
Message-ID: <56640B00.7040100@m4x.org> (raw)
In-Reply-To: <20151206093559.GT10243-dv86pmgwkMBes7Z6vYuT8azUEOm+Xw19@public.gmane.org>

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

WARNING: multiple messages have this Message-ID (diff)
From: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
To: "Boris Brezillon" <boris.brezillon@free-electrons.com>,
	"David Airlie" <airlied@linux.ie>,
	"Jianwei Wang" <jianwei.wang.chn@gmail.com>,
	"Alison Wang" <alison.wang@freescale.com>,
	"Thierry Reding" <thierry.reding@gmail.com>,
	"Terje Bergström" <tbergstrom@nvidia.com>,
	dri-devel@lists.freedesktop.org, linux-tegra@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Subject: Re: [PATCH] drm: do not use device name as a format string
Date: Sun, 6 Dec 2015 11:16:32 +0100	[thread overview]
Message-ID: <56640B00.7040100@m4x.org> (raw)
In-Reply-To: <20151206093559.GT10243@phenom.ffwll.local>

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


  parent reply	other threads:[~2015-12-06 10:16 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-11-18 17:58 [PATCH] drm: do not use device name as a format string Nicolas Iooss
2015-11-18 17:58 ` Nicolas Iooss
     [not found] ` <1447869498-13277-1-git-send-email-nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org>
2015-12-05  9:45   ` Nicolas Iooss
2015-12-05  9:45     ` Nicolas Iooss
2015-12-06  9:35     ` Daniel Vetter
2015-12-06  9:35       ` Daniel Vetter
     [not found]       ` <20151206093559.GT10243-dv86pmgwkMBes7Z6vYuT8azUEOm+Xw19@public.gmane.org>
2015-12-06 10:16         ` Nicolas Iooss [this message]
2015-12-06 10:16           ` Nicolas Iooss
2015-12-07  7:43           ` Daniel Vetter
2015-12-07  7:43             ` Daniel Vetter
2015-12-07  9:53             ` Jani Nikula
2015-12-07  9:53               ` Jani Nikula
     [not found]               ` <874mfuzite.fsf-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2015-12-07 11:46                 ` Daniel Vetter
2015-12-07 11:46                   ` Daniel Vetter
     [not found]                   ` <20151207114652.GC10243-dv86pmgwkMBes7Z6vYuT8azUEOm+Xw19@public.gmane.org>
2015-12-07 12:31                     ` Thierry Reding
2015-12-07 12:31                       ` Thierry Reding
2015-12-07 17:25                       ` Nicolas Iooss
2015-12-07 17:25                         ` Nicolas Iooss
2015-12-07 12:25 ` Boris Brezillon
2015-12-07 12:25   ` Boris Brezillon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=56640B00.7040100@m4x.org \
    --to=nicolas.iooss_linux-owgtiyur0i8@public.gmane.org \
    --cc=airlied-cv59FeDIM0c@public.gmane.org \
    --cc=alison.wang-KZfg59tc24xl57MIdRCFDg@public.gmane.org \
    --cc=boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org \
    --cc=dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org \
    --cc=jianwei.wang.chn-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \
    --cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=tbergstrom-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org \
    --cc=thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.