From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S938482AbcKWNil (ORCPT ); Wed, 23 Nov 2016 08:38:41 -0500 Received: from foss.arm.com ([217.140.101.70]:52630 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S935649AbcKWNii (ORCPT ); Wed, 23 Nov 2016 08:38:38 -0500 Date: Wed, 23 Nov 2016 13:38:36 +0000 From: Liviu Dudau To: Jani Nikula Cc: Daniel Vetter , Daniel Vetter , Eric Engestrom , LKML , DRI devel Subject: Re: [PATCH v2] drm: check for NULL parameter in exported drm_get_format_name() function. Message-ID: <20161123133835.GY1005@e106497-lin.cambridge.arm.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> <87k2buwefa.fsf@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <87k2buwefa.fsf@intel.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Nov 23, 2016 at 02:47:53PM +0200, Jani Nikula wrote: > 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. I was updating a stashed series and discovered that signature of the function has changed. When I looked at how it changed and I got past the "you pass as a parameter a pointer to a struct that is used as a buffer and then that buffer is returned by function" weirdness, I thought that at least checking for bad parameters should be done. > > 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. OK. I just like defensive programming, that's all. :) Best regards, Liviu > > 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 -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ¯\_(ツ)_/¯ From mboxrd@z Thu Jan 1 00:00:00 1970 From: Liviu Dudau Subject: Re: [PATCH v2] drm: check for NULL parameter in exported drm_get_format_name() function. Date: Wed, 23 Nov 2016 13:38:36 +0000 Message-ID: <20161123133835.GY1005@e106497-lin.cambridge.arm.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> <87k2buwefa.fsf@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from foss.arm.com (foss.arm.com [217.140.101.70]) by gabe.freedesktop.org (Postfix) with ESMTP id 05FAF6E879 for ; Wed, 23 Nov 2016 13:38:38 +0000 (UTC) Content-Disposition: inline In-Reply-To: <87k2buwefa.fsf@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Jani Nikula Cc: Daniel Vetter , DRI devel , Eric Engestrom , LKML List-Id: dri-devel@lists.freedesktop.org T24gV2VkLCBOb3YgMjMsIDIwMTYgYXQgMDI6NDc6NTNQTSArMDIwMCwgSmFuaSBOaWt1bGEgd3Jv dGU6Cj4gT24gV2VkLCAyMyBOb3YgMjAxNiwgRGFuaWVsIFZldHRlciA8ZGFuaWVsQGZmd2xsLmNo PiB3cm90ZToKPiA+IE9uIFdlZCwgTm92IDIzLCAyMDE2IGF0IDExOjIzOjIzQU0gKzAwMDAsIExp dml1IER1ZGF1IHdyb3RlOgo+ID4+IE9uIFdlZCwgTm92IDIzLCAyMDE2IGF0IDAxOjAwOjA3UE0g KzAyMDAsIEphbmkgTmlrdWxhIHdyb3RlOgo+ID4+ID4gT24gV2VkLCAyMyBOb3YgMjAxNiwgTGl2 aXUgRHVkYXUgPExpdml1LkR1ZGF1QGFybS5jb20+IHdyb3RlOgo+ID4+ID4gPiBkcm1fZ2V0X2Zv cm1hdF9uYW1lKCkgZGUtcmVmZXJlbmNlcyB0aGUgYnVmIHBhcmFtZXRlciB3aXRob3V0IGNoZWNr aW5nCj4gPj4gPiA+IGlmIHRoZSBwb2ludGVyIHdhcyBub3QgTlVMTC4gR2l2ZW4gdGhhdCB0aGUg ZnVuY3Rpb24gaXMgRVhQT1JULWVkLCBsZXRzCj4gPj4gPiA+IHNhbml0aXNlIHRoZSBwYXJhbWV0 ZXJzIGJlZm9yZSBwcm9jZWVkaW5nLgo+ID4+ID4gPgo+ID4+ID4gPiB2MjogVXNlIEJVR19PTigp IHRvIGFubm95IHVzZXJzIHRoYXQgZGlkIG5vdCBwYXNzIHZhbGlkIHBhcmFtZXRlcnMgdG8gZnVu Y3Rpb24uCj4gPj4gPiA+Cj4gPj4gPiA+IEZpeGVzOiBiM2MxMWFjMjY3ZDQ2MWQzZDUgKCJkcm06 IG1vdmUgYWxsb2NhdGlvbiBvdXQgb2YgZHJtX2dldF9mb3JtYXRfbmFtZSgpKQo+ID4+ID4gPiBD YzogRXJpYyBFbmdlc3Ryb20gPGVyaWNAZW5nZXN0cm9tLmNoPgo+ID4+ID4gPiBDYzogUm9iIENs YXJrIDxyb2JkY2xhcmtAZ21haWwuY29tPgo+ID4+ID4gPiBDYzogSmFuaSBOaWt1bGEgPGphbmku bmlrdWxhQGludGVsLmNvbT4KPiA+PiA+ID4gQ2M6IERhbmllbCBWZXR0ZXIgPGRhbmllbC52ZXR0 ZXJAZmZ3bGwuY2g+Cj4gPj4gPiA+Cj4gPj4gPiA+IFNpZ25lZC1vZmYtYnk6IExpdml1IER1ZGF1 IDxMaXZpdS5EdWRhdUBhcm0uY29tPgo+ID4+ID4gPiAtLS0KPiA+PiA+ID4gSSBzdGlsbCB0aGlu ayBzYW5pdHkgY2hlY2tpbmcgdGhlIHBhcmFtZXRlcnMgb2YgYW4gZXhwb3J0ZWQgZnVuY3Rpb24g aXMgd29ydGgKPiA+PiA+ID4gZG9pbmcsIGV2ZW4gaWYgdGhlIHdheSBvbmUgdHJpZ2dlcnMgdGhl IE5VTEwgcG9pbnRlciBjcmFzaCBpcyBwcml2aWxlZGdlZC4gTm90Cj4gPj4gPiA+IGEgYmlnIGZh biBvZiB0aGUgdmVyYm9zaXR5IG9mIEJVR19PTigpIGFuZCB3b3VsZCByYXRoZXIgc2lsZW50bHkg cmVqZWN0IE5VTEwgYnVmCj4gPj4gPiA+IHBvaW50ZXIsIGJ1dCB0aGF0IGlzIGEgbWF0dGVyIG9m IHRhc3RlLgo+ID4+ID4gCj4gPj4gPiBUaGVyZSByZWFsbHkgaXMgbm8gbWVhbmluZ2Z1bCBkaWZm ZXJlbmNlIGJldHdlZW4gZG9pbmcgQlVHX09OKCFidWcpCj4gPj4gPiB2cy4ganVzdCBsZXR0aW5n IGJ1Zi0+c3RyIG9vcHMuIFRoZSBrZXJuZWwgaXMgZnVsbCBvZiBmdW5jdGlvbnMgdGhhdAo+ID4+ ID4gZXhwZWN0IHNlbnNpYmxlIHBvaW50ZXJzLCBhbmQgSSBkb24ndCBzZWUgd2h5IHRoaXMgb25l IGluIHBhcnRpY3VsYXIKPiA+PiA+IHNob3VsZCBiZSBzbyBzcGVjaWFsIHRvIHdhcnJhbnQgYSBC VUdfT04oKS4KPiA+PiAKPiA+PiBBZ3JlZS4gVGhhdCBpcyB3aHkgSSBwcmVmZXIgdjEgd2hlcmUg SSByZXR1cm4gaW1tZWRpYXRlbHkgb24gTlVMTCBwb2ludGVycy4KPiA+Cj4gPiBUaGUgcXVlc3Rp b24gZm9yIHYxIGlzIHdoeSBkaWQgeW91IGhpdCB0aGF0PyAiYnJva2VuIGRyaXZlciBjb2RlIiBp c24ndAo+ID4gcmVhbGx5IGEgZ29vZCByZWFzb24sIGF1IGNvbnRyYWlyZSBpdCdzIGEgcmVhc29u IHRvIG5vdCBtZXJnZSB5b3VyIHBhdGNoOgo+ID4gV2UgZG8gbm90IHdhbnQgdG8gaGlkZSBkcml2 ZXIgYnVncyBzaWxlbnRseS4KCkkgd2FzIHVwZGF0aW5nIGEgc3Rhc2hlZCBzZXJpZXMgYW5kIGRp c2NvdmVyZWQgdGhhdCBzaWduYXR1cmUgb2YgdGhlIGZ1bmN0aW9uIGhhcyBjaGFuZ2VkLgpXaGVu IEkgbG9va2VkIGF0IGhvdyBpdCBjaGFuZ2VkIGFuZCBJIGdvdCBwYXN0IHRoZSAieW91IHBhc3Mg YXMgYSBwYXJhbWV0ZXIgYSBwb2ludGVyIAp0byBhIHN0cnVjdCB0aGF0IGlzIHVzZWQgYXMgYSBi dWZmZXIgYW5kIHRoZW4gdGhhdCBidWZmZXIgaXMgcmV0dXJuZWQgYnkgZnVuY3Rpb24iIHdlaXJk bmVzcywKSSB0aG91Z2h0IHRoYXQgYXQgbGVhc3QgY2hlY2tpbmcgZm9yIGJhZCBwYXJhbWV0ZXJz IHNob3VsZCBiZSBkb25lLgoKPiAKPiBNb3Jlb3ZlciwgdjEgcHV0cyB0aGUgYnVyZGVuIGJhY2sg b24gdGhlICpjYWxsZXIqIG9mIHRoZSBmdW5jdGlvbiB0bwo+IGNoZWNrIGZvciBOVUxMIHJldHVy biwgd2hpbGUgaXQgcHJldmlvdXNseSBjb3VsZCBub3QgZXZlbiByZXR1cm4gTlVMTC4KPiAKPiBU aGUgZnVuY3Rpb24gaXMgZmluZS4gSXQgaXNuJ3QgYnJva2VuLiBEb24ndCB0cnkgdG8gZml4IGl0 LgoKT0suIEkganVzdCBsaWtlIGRlZmVuc2l2ZSBwcm9ncmFtbWluZywgdGhhdCdzIGFsbC4gOikK CkJlc3QgcmVnYXJkcywKTGl2aXUKCj4gCj4gQlIsCj4gSmFuaS4KPiAKPiAKPiAKPiA+Cj4gPiBU aGVyZSdzIGRlZmluaXRlbHkgY2FzZXMgd2hlcmUgaGFuZGxpbmcgTlVMTCBhdXRvbWF0aWNhbGx5 IGlzIHJlYXNvbmFibGUsCj4gPiBlLmcuIGtmcmVlKCkuIEJ1dCBhIE5VTEwgZHJtX2Zvcm1hdF9u YW1lX2J1ZiBzb3VuZHMgbGlrZSwgYXQgbGVhc3QgYSBxdWljawo+ID4gZ3JlcCBzaG93cyB0aGF0 IGFsbCBjYWxsZXJzIGp1c3QgcHV0IHRoaXMgc3RydWN0IG9udG8gdGhlIHN0YWNrLgo+ID4gLURh bmllbAo+IAo+IC0tIAo+IEphbmkgTmlrdWxhLCBJbnRlbCBPcGVuIFNvdXJjZSBUZWNobm9sb2d5 IENlbnRlcgoKLS0gCj09PT09PT09PT09PT09PT09PT09CnwgSSB3b3VsZCBsaWtlIHRvIHwKfCBm aXggdGhlIHdvcmxkLCAgfAp8IGJ1dCB0aGV5J3JlIG5vdCB8CnwgZ2l2aW5nIG1lIHRoZSAgIHwK IFwgc291cmNlIGNvZGUhICAvCiAgLS0tLS0tLS0tLS0tLS0tCiAgICDCr1xfKOODhClfL8KvCl9f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmRyaS1kZXZlbCBt YWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwczovL2xpc3Rz LmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbAo=