From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S936347AbcKWMsA (ORCPT ); Wed, 23 Nov 2016 07:48:00 -0500 Received: from mga01.intel.com ([192.55.52.88]:62383 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933829AbcKWMr6 (ORCPT ); Wed, 23 Nov 2016 07:47:58 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,538,1473145200"; d="scan'208";a="789902226" From: Jani Nikula To: Daniel Vetter , Liviu Dudau Cc: Daniel Vetter , Eric Engestrom , LKML , DRI devel Subject: Re: [PATCH v2] drm: check for NULL parameter in exported drm_get_format_name() function. In-Reply-To: <20161123122634.4z2dftmzpbexnhjs@phenom.ffwll.local> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo References: <20161123105213.27674-1-Liviu.Dudau@arm.com> <87vavewjew.fsf@intel.com> <20161123112323.GX1005@e106497-lin.cambridge.arm.com> <20161123122634.4z2dftmzpbexnhjs@phenom.ffwll.local> Date: Wed, 23 Nov 2016 14:47:53 +0200 Message-ID: <87k2buwefa.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 Wed, 23 Nov 2016, Daniel Vetter wrote: > On Wed, Nov 23, 2016 at 11:23:23AM +0000, Liviu Dudau wrote: >> On Wed, Nov 23, 2016 at 01:00:07PM +0200, Jani Nikula wrote: >> > On Wed, 23 Nov 2016, Liviu Dudau wrote: >> > > drm_get_format_name() de-references the buf parameter without checking >> > > if the pointer was not NULL. Given that the function is EXPORT-ed, lets >> > > sanitise the parameters before proceeding. >> > > >> > > v2: Use BUG_ON() to annoy users that did not pass valid parameters to function. >> > > >> > > Fixes: b3c11ac267d461d3d5 ("drm: move allocation out of drm_get_format_name()) >> > > Cc: Eric Engestrom >> > > Cc: Rob Clark >> > > Cc: Jani Nikula >> > > Cc: Daniel Vetter >> > > >> > > Signed-off-by: Liviu Dudau >> > > --- >> > > I still think sanity checking the parameters of an exported function is worth >> > > doing, even if the way one triggers the NULL pointer crash is priviledged. Not >> > > a big fan of the verbosity of BUG_ON() and would rather silently reject NULL buf >> > > pointer, but that is a matter of taste. >> > >> > There really is no meaningful difference between doing BUG_ON(!bug) >> > vs. just letting buf->str oops. The kernel is full of functions that >> > expect sensible pointers, and I don't see why this one in particular >> > should be so special to warrant a BUG_ON(). >> >> Agree. That is why I prefer v1 where I return immediately on NULL pointers. > > The question for v1 is why did you hit that? "broken driver code" isn't > really a good reason, au contraire it's a reason to not merge your patch: > We do not want to hide driver bugs silently. Moreover, v1 puts the burden back on the *caller* of the function to check for NULL return, while it previously could not even return NULL. The function is fine. It isn't broken. Don't try to fix it. BR, Jani. > > There's definitely cases where handling NULL automatically is reasonable, > e.g. kfree(). But a NULL drm_format_name_buf sounds like, at least a quick > grep shows that all callers just put this struct onto the stack. > -Daniel -- Jani Nikula, Intel Open Source Technology Center From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jani Nikula Subject: Re: [PATCH v2] drm: check for NULL parameter in exported drm_get_format_name() function. Date: Wed, 23 Nov 2016 14:47:53 +0200 Message-ID: <87k2buwefa.fsf@intel.com> References: <20161123105213.27674-1-Liviu.Dudau@arm.com> <87vavewjew.fsf@intel.com> <20161123112323.GX1005@e106497-lin.cambridge.arm.com> <20161123122634.4z2dftmzpbexnhjs@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTPS id C97F16E873 for ; Wed, 23 Nov 2016 12:47:58 +0000 (UTC) In-Reply-To: <20161123122634.4z2dftmzpbexnhjs@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 , Liviu Dudau Cc: Daniel Vetter , Eric Engestrom , LKML , DRI devel List-Id: dri-devel@lists.freedesktop.org T24gV2VkLCAyMyBOb3YgMjAxNiwgRGFuaWVsIFZldHRlciA8ZGFuaWVsQGZmd2xsLmNoPiB3cm90 ZToKPiBPbiBXZWQsIE5vdiAyMywgMjAxNiBhdCAxMToyMzoyM0FNICswMDAwLCBMaXZpdSBEdWRh dSB3cm90ZToKPj4gT24gV2VkLCBOb3YgMjMsIDIwMTYgYXQgMDE6MDA6MDdQTSArMDIwMCwgSmFu aSBOaWt1bGEgd3JvdGU6Cj4+ID4gT24gV2VkLCAyMyBOb3YgMjAxNiwgTGl2aXUgRHVkYXUgPExp dml1LkR1ZGF1QGFybS5jb20+IHdyb3RlOgo+PiA+ID4gZHJtX2dldF9mb3JtYXRfbmFtZSgpIGRl LXJlZmVyZW5jZXMgdGhlIGJ1ZiBwYXJhbWV0ZXIgd2l0aG91dCBjaGVja2luZwo+PiA+ID4gaWYg dGhlIHBvaW50ZXIgd2FzIG5vdCBOVUxMLiBHaXZlbiB0aGF0IHRoZSBmdW5jdGlvbiBpcyBFWFBP UlQtZWQsIGxldHMKPj4gPiA+IHNhbml0aXNlIHRoZSBwYXJhbWV0ZXJzIGJlZm9yZSBwcm9jZWVk aW5nLgo+PiA+ID4KPj4gPiA+IHYyOiBVc2UgQlVHX09OKCkgdG8gYW5ub3kgdXNlcnMgdGhhdCBk aWQgbm90IHBhc3MgdmFsaWQgcGFyYW1ldGVycyB0byBmdW5jdGlvbi4KPj4gPiA+Cj4+ID4gPiBG aXhlczogYjNjMTFhYzI2N2Q0NjFkM2Q1ICgiZHJtOiBtb3ZlIGFsbG9jYXRpb24gb3V0IG9mIGRy bV9nZXRfZm9ybWF0X25hbWUoKSkKPj4gPiA+IENjOiBFcmljIEVuZ2VzdHJvbSA8ZXJpY0Blbmdl c3Ryb20uY2g+Cj4+ID4gPiBDYzogUm9iIENsYXJrIDxyb2JkY2xhcmtAZ21haWwuY29tPgo+PiA+ ID4gQ2M6IEphbmkgTmlrdWxhIDxqYW5pLm5pa3VsYUBpbnRlbC5jb20+Cj4+ID4gPiBDYzogRGFu aWVsIFZldHRlciA8ZGFuaWVsLnZldHRlckBmZndsbC5jaD4KPj4gPiA+Cj4+ID4gPiBTaWduZWQt b2ZmLWJ5OiBMaXZpdSBEdWRhdSA8TGl2aXUuRHVkYXVAYXJtLmNvbT4KPj4gPiA+IC0tLQo+PiA+ ID4gSSBzdGlsbCB0aGluayBzYW5pdHkgY2hlY2tpbmcgdGhlIHBhcmFtZXRlcnMgb2YgYW4gZXhw b3J0ZWQgZnVuY3Rpb24gaXMgd29ydGgKPj4gPiA+IGRvaW5nLCBldmVuIGlmIHRoZSB3YXkgb25l IHRyaWdnZXJzIHRoZSBOVUxMIHBvaW50ZXIgY3Jhc2ggaXMgcHJpdmlsZWRnZWQuIE5vdAo+PiA+ ID4gYSBiaWcgZmFuIG9mIHRoZSB2ZXJib3NpdHkgb2YgQlVHX09OKCkgYW5kIHdvdWxkIHJhdGhl ciBzaWxlbnRseSByZWplY3QgTlVMTCBidWYKPj4gPiA+IHBvaW50ZXIsIGJ1dCB0aGF0IGlzIGEg bWF0dGVyIG9mIHRhc3RlLgo+PiA+IAo+PiA+IFRoZXJlIHJlYWxseSBpcyBubyBtZWFuaW5nZnVs IGRpZmZlcmVuY2UgYmV0d2VlbiBkb2luZyBCVUdfT04oIWJ1ZykKPj4gPiB2cy4ganVzdCBsZXR0 aW5nIGJ1Zi0+c3RyIG9vcHMuIFRoZSBrZXJuZWwgaXMgZnVsbCBvZiBmdW5jdGlvbnMgdGhhdAo+ PiA+IGV4cGVjdCBzZW5zaWJsZSBwb2ludGVycywgYW5kIEkgZG9uJ3Qgc2VlIHdoeSB0aGlzIG9u ZSBpbiBwYXJ0aWN1bGFyCj4+ID4gc2hvdWxkIGJlIHNvIHNwZWNpYWwgdG8gd2FycmFudCBhIEJV R19PTigpLgo+PiAKPj4gQWdyZWUuIFRoYXQgaXMgd2h5IEkgcHJlZmVyIHYxIHdoZXJlIEkgcmV0 dXJuIGltbWVkaWF0ZWx5IG9uIE5VTEwgcG9pbnRlcnMuCj4KPiBUaGUgcXVlc3Rpb24gZm9yIHYx IGlzIHdoeSBkaWQgeW91IGhpdCB0aGF0PyAiYnJva2VuIGRyaXZlciBjb2RlIiBpc24ndAo+IHJl YWxseSBhIGdvb2QgcmVhc29uLCBhdSBjb250cmFpcmUgaXQncyBhIHJlYXNvbiB0byBub3QgbWVy Z2UgeW91ciBwYXRjaDoKPiBXZSBkbyBub3Qgd2FudCB0byBoaWRlIGRyaXZlciBidWdzIHNpbGVu dGx5LgoKTW9yZW92ZXIsIHYxIHB1dHMgdGhlIGJ1cmRlbiBiYWNrIG9uIHRoZSAqY2FsbGVyKiBv ZiB0aGUgZnVuY3Rpb24gdG8KY2hlY2sgZm9yIE5VTEwgcmV0dXJuLCB3aGlsZSBpdCBwcmV2aW91 c2x5IGNvdWxkIG5vdCBldmVuIHJldHVybiBOVUxMLgoKVGhlIGZ1bmN0aW9uIGlzIGZpbmUuIEl0 IGlzbid0IGJyb2tlbi4gRG9uJ3QgdHJ5IHRvIGZpeCBpdC4KCkJSLApKYW5pLgoKCgo+Cj4gVGhl cmUncyBkZWZpbml0ZWx5IGNhc2VzIHdoZXJlIGhhbmRsaW5nIE5VTEwgYXV0b21hdGljYWxseSBp cyByZWFzb25hYmxlLAo+IGUuZy4ga2ZyZWUoKS4gQnV0IGEgTlVMTCBkcm1fZm9ybWF0X25hbWVf YnVmIHNvdW5kcyBsaWtlLCBhdCBsZWFzdCBhIHF1aWNrCj4gZ3JlcCBzaG93cyB0aGF0IGFsbCBj YWxsZXJzIGp1c3QgcHV0IHRoaXMgc3RydWN0IG9udG8gdGhlIHN0YWNrLgo+IC1EYW5pZWwKCi0t IApKYW5pIE5pa3VsYSwgSW50ZWwgT3BlbiBTb3VyY2UgVGVjaG5vbG9neSBDZW50ZXIKX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVsIG1haWxp bmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJl ZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg==