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]

Date:	Mon, 7 Dec 2015 08:43:12 +0100
From:	Daniel Vetter <daniel@...ll.ch>
To:	Nicolas Iooss <nicolas.iooss_linux@....org>
Cc:	Boris Brezillon <boris.brezillon@...e-electrons.com>,
	David Airlie <airlied@...ux.ie>,
	Jianwei Wang <jianwei.wang.chn@...il.com>,
	Alison Wang <alison.wang@...escale.com>,
	Thierry Reding <thierry.reding@...il.com>,
	Terje Bergström <tbergstrom@...dia.com>,
	dri-devel@...ts.freedesktop.org, linux-tegra@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm: do not use device name as a format string

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
--
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/