From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jani Nikula Subject: Re: [PATCH] drm: do not use device name as a format string Date: Mon, 07 Dec 2015 11:53:01 +0200 Message-ID: <874mfuzite.fsf@intel.com> 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> <20151207074312.GU10243@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20151207074312.GU10243@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Daniel Vetter , Nicolas Iooss Cc: linux-tegra@vger.kernel.org, Terje =?utf-8?Q?Bergstr=C3=B6m?= , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Alison Wang List-Id: linux-tegra@vger.kernel.org T24gTW9uLCAwNyBEZWMgMjAxNSwgRGFuaWVsIFZldHRlciA8ZGFuaWVsQGZmd2xsLmNoPiB3cm90 ZToKPiBPbiBTdW4sIERlYyAwNiwgMjAxNSBhdCAxMToxNjozMkFNICswMTAwLCBOaWNvbGFzIElv b3NzIHdyb3RlOgo+PiBPbiAxMi8wNi8yMDE1IDEwOjM1IEFNLCBEYW5pZWwgVmV0dGVyIHdyb3Rl Ogo+PiA+PiBPbiAxMS8xOC8yMDE1IDA2OjU4IFBNLCBOaWNvbGFzIElvb3NzIHdyb3RlOgo+PiA+ Pj4gZHJtX2Rldl9zZXRfdW5pcXVlKCkgZm9ybWF0cyBpdHMgcGFyYW1ldGVyIHVzaW5nIGt2YXNw cmludGYoKSBidXQgbWFueQo+PiA+Pj4gb2YgaXRzIGNhbGxlcnMgZGlyZWN0bHkgcGFzcyBkZXZf bmFtZShkZXYpIGFzIHByaW50ZiBmb3JtYXQgc3RyaW5nLAo+PiA+Pj4gd2l0aG91dCBhbnkgZm9y bWF0IHBhcmFtZXRlci4gIFRoaXMgY2FuIGNhdXNlIHNvbWUgaXNzdWVzIHdoZW4gdGhlCj4+ID4+ PiBkZXZpY2UgbmFtZSBjb250YWlucyAnJScgY2hhcmFjdGVycy4KPj4gPj4+Cj4+ID4+PiBUbyBh dm9pZCBhbnkgcG90ZW50aWFsIGlzc3VlLCBhbHdheXMgdXNlICIlcyIgd2hlbiB1c2luZwo+PiA+ Pj4gZHJtX2Rldl9zZXRfdW5pcXVlKCkgd2l0aCBkZXZfbmFtZSgpLgo+PiA+IAo+PiA+IE5vdCBz dXJlIHRoaXMgaXMgd29ydGggaXQgcmVhbGx5LCBub3JtYWxseSBwZW9wbGUgZG9uJ3QgcGxhY2Ug JSBjaGFyYWN0ZXJzCj4+ID4gaW50byB0aGVpciBkZXZpY2UgbmFtZXMsIGV2ZXIuIEFuZCBpZiB0 aGV5IGRvIGl0J2xsIGJsb3cgdXAuIFRoZXJlJ3MgYWxzbwo+PiA+IG5vIHNlY3VyaXR5IGlzc3Vl IGhlcmUgc2luY2UgdXNlcnNwYWNlIGNhbid0IHNldCB0aGlzIG5hbWUuCj4+ID4gCj4+ID4gSWYg dGhlIG1haW50YWluZXJzIG9mIHRoZSBhZmZlY3RlZCBkcml2ZXJzIGRvbid0IHdhbnQgdGhpcyBJ IHdvbid0IG1lcmdlCj4+ID4gdGhpcyBwYXRjaC4KPj4gCj4+IEFjdHVhbGx5IEkgaGFkIHRoZSBz YW1lIG9waW5pb24gYmVmb3JlIEkgYmVnYW4gdG8gYWRkIF9fcHJpbnRmCj4+IGF0dHJpYnV0ZXMg YW5kICIlcyIgaW4gc2V2ZXJhbCBwbGFjZXMgaW4gdGhlIGtlcm5lbCB0byBtYWtlCj4+IC1XZm9y bWF0LXNlY3VyaXR5IHVzZWZ1bC4gIFRoaXMgbGVkIG1lIHRvIGRpc2NvdmVyIHNvbWUgZnVubnkg aXNzdWVzCj4+IGxpa2UgdGhlIG9uZSBmaXhlZCBieSBjb21taXQgMzk1OGI3OTI2NmIxICgiY29u ZmlnZnM6IGZpeCBrZXJuZWwKPj4gaW5mb2xlYWsgdGhyb3VnaCB1c2VyLWNvbnRyb2xsZWQgZm9y bWF0IHN0cmluZyIsCj4+IGh0dHBzOi8vZ2l0Lmtlcm5lbC5vcmcvY2dpdC9saW51eC9rZXJuZWwv Z2l0L3RvcnZhbGRzL2xpbnV4LmdpdC9jb21taXQvP2lkPTM5NThiNzkyNjZiMTQ3MjllZGQ2MWRh ZjlkZmI4NGRlNDVmNGVjNmQKPj4gKS4gIFRoZSBwYXRjaCBJIHNlbnQgaXMgaW4gZmFjdCBhIHZl cnkgc21hbGwgc3RlcCB0b3dhcmRzIG1ha2luZwo+PiAtV2Zvcm1hdC1zZWN1cml0eSB1c2VmdWwg YWdhaW4gdG8gZGV0ZWN0ICJyZWFsIiBpc3N1ZXMuCj4+IAo+PiBPZiBjb3Vyc2UsIGlmIHlvdSBk byBub3QgZmVlbCBpdCBpcyB3b3J0aCBpdCBhbmQgYmVsaWV2ZSB0aGF0IGRldl9uYW1lCj4+IGlz IGZ1bGx5IGNvbnRyb2xsZWQgYnkgdHJ1c3RlZCBzb3VyY2VzIHdoaWNoIHdpbGwgbmV2ZXIgaW50 cm9kdWNlIGFueSAlCj4+IGNoYXJhY3RlciwgSSB1bmRlcnN0YW5kIHlvdXIgd2lsbCBvZiBub3Qg bWVyZ2luZyBteSBwYXRjaC4KPgo+IEFoLCB0aGF0J3MgdGhlIGNvbnRleHQgSSB3YXMgbWlzc2lu ZywgdGhhdCByZWFsbHkgc2hvdWxkIGJlIGluIHRoZSBjb21taXQKPiBtZXNzYWdlLiBJZiB0aGlz IGlzIHBhcnQgb2YgYW4gb3ZlcmFsbCBlZmZvcnQgdG8gZW5hYmxlIHNvbWV0aGluZyB1c2VmdWwK PiBpdCBtYWtlcyBtb3JlIHNlbnNlIHRvIGdldCBpdCBpbi4KPgo+IE9uIHRoZSBwYXRjaCBpdHNl bGYgaXQgZmVlbHMgcmF0aGVyIGZ1bm55IHRvIGRvIGEgIiVzIiwgc3RyKTsgY29tYm8sIG1heWJl Cj4gd2Ugc2hvdWxkIGhhdmUgYSBkcm1fZGV2X3NldF91bmlxdWVfc3RhdGljIGluc3RlYWQ/IElu Y2x1ZGluZyBrZXJuZWxkb2MKPiBleHBsYWluaW5nIHdoeSB0aGVyZSdzIHRvby4KCk5vIGNhbGxl ciBvZiBkcm1fZGV2X3NldF91bmlxdWUoKSBhY3R1YWxseSB1c2VzIHRoZSBmb3JtYXR0aW5nIGZv cgphbnl0aGluZy4uLiBzbyB5b3UnZCBlbmQgdXAgd2l0aCBkcm1fZGV2X3NldF91bmlxdWVfc3Rh dGljKCkgYW5kIGFuCm9ycGhhbmVkIGRybV9kZXZfc2V0X3VuaXF1ZSgpLi4uCgpCUiwKSmFuaS4K CgoKLS0gCkphbmkgTmlrdWxhLCBJbnRlbCBPcGVuIFNvdXJjZSBUZWNobm9sb2d5IENlbnRlcgpf X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpkcmktZGV2ZWwg bWFpbGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cDovL2xpc3Rz LmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755702AbbLGJxH (ORCPT ); Mon, 7 Dec 2015 04:53:07 -0500 Received: from mga02.intel.com ([134.134.136.20]:35544 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755024AbbLGJxF (ORCPT ); Mon, 7 Dec 2015 04:53:05 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,394,1444719600"; d="scan'208";a="868192584" From: Jani Nikula To: Daniel Vetter , Nicolas Iooss Cc: Terje =?utf-8?Q?Bergstr=C3=B6m?= , Alison Wang , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-tegra@vger.kernel.org Subject: Re: [PATCH] drm: do not use device name as a format string In-Reply-To: <20151207074312.GU10243@phenom.ffwll.local> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo 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> <20151207074312.GU10243@phenom.ffwll.local> User-Agent: Notmuch/0.20.2+101~gb57168b (http://notmuchmail.org) Emacs/24.4.1 (x86_64-pc-linux-gnu) Date: Mon, 07 Dec 2015 11:53:01 +0200 Message-ID: <874mfuzite.fsf@intel.com> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 07 Dec 2015, Daniel Vetter 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()... BR, Jani. -- Jani Nikula, Intel Open Source Technology Center