Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754784AbbLGHnz (ORCPT ); Mon, 7 Dec 2015 02:43:55 -0500 Received: from mail-wm0-f54.google.com ([74.125.82.54]:33675 "EHLO mail-wm0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754572AbbLGHnR (ORCPT ); Mon, 7 Dec 2015 02:43:17 -0500 Date: Mon, 7 Dec 2015 08:43:12 +0100 From: Daniel Vetter To: Nicolas Iooss Cc: Boris Brezillon , David Airlie , Jianwei Wang , Alison Wang , Thierry Reding , Terje =?iso-8859-1?Q?Bergstr=F6m?= , dri-devel@lists.freedesktop.org, linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] drm: do not use device name as a format string Message-ID: <20151207074312.GU10243@phenom.ffwll.local> Mail-Followup-To: Nicolas Iooss , Boris Brezillon , David Airlie , Jianwei Wang , Alison Wang , Thierry Reding , Terje =?iso-8859-1?Q?Bergstr=F6m?= , dri-devel@lists.freedesktop.org, linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org References: <1447869498-13277-1-git-send-email-nicolas.iooss_linux@m4x.org> <5662B24E.4090202@m4x.org> <20151206093559.GT10243@phenom.ffwll.local> <56640B00.7040100@m4x.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <56640B00.7040100@m4x.org> X-Operating-System: Linux phenom 4.1.0-2-amd64 User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2324 Lines: 48 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/