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

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3210 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-/w4YWyX8dFk@public.gmane.org> 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 --]