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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20151207114652.GC10243@phenom.ffwll.local>
Date:	Mon, 7 Dec 2015 12:46:52 +0100
From:	Daniel Vetter <daniel@...ll.ch>
To:	Jani Nikula <jani.nikula@...ux.intel.com>
Cc:	Daniel Vetter <daniel@...ll.ch>,
	Nicolas Iooss <nicolas.iooss_linux@....org>,
	Terje Bergström <tbergstrom@...dia.com>,
	Alison Wang <alison.wang@...escale.com>,
	linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
	linux-tegra@...r.kernel.org
Subject: Re: [PATCH] drm: do not use device name as a format string

On Mon, Dec 07, 2015 at 11:53:01AM +0200, Jani Nikula wrote:
> On Mon, 07 Dec 2015, Daniel Vetter <daniel@...ll.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/