From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S936487AbcKWM0j (ORCPT ); Wed, 23 Nov 2016 07:26:39 -0500 Received: from mail-wm0-f65.google.com ([74.125.82.65]:34900 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933928AbcKWM0h (ORCPT ); Wed, 23 Nov 2016 07:26:37 -0500 Date: Wed, 23 Nov 2016 13:26:34 +0100 From: Daniel Vetter To: Liviu Dudau Cc: Jani Nikula , Eric Engestrom , Daniel Vetter , LKML , DRI devel Subject: Re: [PATCH v2] drm: check for NULL parameter in exported drm_get_format_name() function. Message-ID: <20161123122634.4z2dftmzpbexnhjs@phenom.ffwll.local> Mail-Followup-To: Liviu Dudau , Jani Nikula , Eric Engestrom , LKML , DRI devel References: <20161123105213.27674-1-Liviu.Dudau@arm.com> <87vavewjew.fsf@intel.com> <20161123112323.GX1005@e106497-lin.cambridge.arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161123112323.GX1005@e106497-lin.cambridge.arm.com> X-Operating-System: Linux phenom 4.8.0-1-amd64 User-Agent: NeoMutt/20161104 (1.7.1) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH v2] drm: check for NULL parameter in exported drm_get_format_name() function. Date: Wed, 23 Nov 2016 13:26:34 +0100 Message-ID: <20161123122634.4z2dftmzpbexnhjs@phenom.ffwll.local> References: <20161123105213.27674-1-Liviu.Dudau@arm.com> <87vavewjew.fsf@intel.com> <20161123112323.GX1005@e106497-lin.cambridge.arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mail-wm0-x243.google.com (mail-wm0-x243.google.com [IPv6:2a00:1450:400c:c09::243]) by gabe.freedesktop.org (Postfix) with ESMTPS id 52AF16E861 for ; Wed, 23 Nov 2016 12:26:37 +0000 (UTC) Received: by mail-wm0-x243.google.com with SMTP id g23so1849872wme.1 for ; Wed, 23 Nov 2016 04:26:37 -0800 (PST) Content-Disposition: inline In-Reply-To: <20161123112323.GX1005@e106497-lin.cambridge.arm.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Liviu Dudau Cc: Jani Nikula , Daniel Vetter , Eric Engestrom , LKML , DRI devel List-Id: dri-devel@lists.freedesktop.org T24gV2VkLCBOb3YgMjMsIDIwMTYgYXQgMTE6MjM6MjNBTSArMDAwMCwgTGl2aXUgRHVkYXUgd3Jv dGU6Cj4gT24gV2VkLCBOb3YgMjMsIDIwMTYgYXQgMDE6MDA6MDdQTSArMDIwMCwgSmFuaSBOaWt1 bGEgd3JvdGU6Cj4gPiBPbiBXZWQsIDIzIE5vdiAyMDE2LCBMaXZpdSBEdWRhdSA8TGl2aXUuRHVk YXVAYXJtLmNvbT4gd3JvdGU6Cj4gPiA+IGRybV9nZXRfZm9ybWF0X25hbWUoKSBkZS1yZWZlcmVu Y2VzIHRoZSBidWYgcGFyYW1ldGVyIHdpdGhvdXQgY2hlY2tpbmcKPiA+ID4gaWYgdGhlIHBvaW50 ZXIgd2FzIG5vdCBOVUxMLiBHaXZlbiB0aGF0IHRoZSBmdW5jdGlvbiBpcyBFWFBPUlQtZWQsIGxl dHMKPiA+ID4gc2FuaXRpc2UgdGhlIHBhcmFtZXRlcnMgYmVmb3JlIHByb2NlZWRpbmcuCj4gPiA+ Cj4gPiA+IHYyOiBVc2UgQlVHX09OKCkgdG8gYW5ub3kgdXNlcnMgdGhhdCBkaWQgbm90IHBhc3Mg dmFsaWQgcGFyYW1ldGVycyB0byBmdW5jdGlvbi4KPiA+ID4KPiA+ID4gRml4ZXM6IGIzYzExYWMy NjdkNDYxZDNkNSAoImRybTogbW92ZSBhbGxvY2F0aW9uIG91dCBvZiBkcm1fZ2V0X2Zvcm1hdF9u YW1lKCkpCj4gPiA+IENjOiBFcmljIEVuZ2VzdHJvbSA8ZXJpY0Blbmdlc3Ryb20uY2g+Cj4gPiA+ IENjOiBSb2IgQ2xhcmsgPHJvYmRjbGFya0BnbWFpbC5jb20+Cj4gPiA+IENjOiBKYW5pIE5pa3Vs YSA8amFuaS5uaWt1bGFAaW50ZWwuY29tPgo+ID4gPiBDYzogRGFuaWVsIFZldHRlciA8ZGFuaWVs LnZldHRlckBmZndsbC5jaD4KPiA+ID4KPiA+ID4gU2lnbmVkLW9mZi1ieTogTGl2aXUgRHVkYXUg PExpdml1LkR1ZGF1QGFybS5jb20+Cj4gPiA+IC0tLQo+ID4gPiBJIHN0aWxsIHRoaW5rIHNhbml0 eSBjaGVja2luZyB0aGUgcGFyYW1ldGVycyBvZiBhbiBleHBvcnRlZCBmdW5jdGlvbiBpcyB3b3J0 aAo+ID4gPiBkb2luZywgZXZlbiBpZiB0aGUgd2F5IG9uZSB0cmlnZ2VycyB0aGUgTlVMTCBwb2lu dGVyIGNyYXNoIGlzIHByaXZpbGVkZ2VkLiBOb3QKPiA+ID4gYSBiaWcgZmFuIG9mIHRoZSB2ZXJi b3NpdHkgb2YgQlVHX09OKCkgYW5kIHdvdWxkIHJhdGhlciBzaWxlbnRseSByZWplY3QgTlVMTCBi dWYKPiA+ID4gcG9pbnRlciwgYnV0IHRoYXQgaXMgYSBtYXR0ZXIgb2YgdGFzdGUuCj4gPiAKPiA+ IFRoZXJlIHJlYWxseSBpcyBubyBtZWFuaW5nZnVsIGRpZmZlcmVuY2UgYmV0d2VlbiBkb2luZyBC VUdfT04oIWJ1ZykKPiA+IHZzLiBqdXN0IGxldHRpbmcgYnVmLT5zdHIgb29wcy4gVGhlIGtlcm5l bCBpcyBmdWxsIG9mIGZ1bmN0aW9ucyB0aGF0Cj4gPiBleHBlY3Qgc2Vuc2libGUgcG9pbnRlcnMs IGFuZCBJIGRvbid0IHNlZSB3aHkgdGhpcyBvbmUgaW4gcGFydGljdWxhcgo+ID4gc2hvdWxkIGJl IHNvIHNwZWNpYWwgdG8gd2FycmFudCBhIEJVR19PTigpLgo+IAo+IEFncmVlLiBUaGF0IGlzIHdo eSBJIHByZWZlciB2MSB3aGVyZSBJIHJldHVybiBpbW1lZGlhdGVseSBvbiBOVUxMIHBvaW50ZXJz LgoKVGhlIHF1ZXN0aW9uIGZvciB2MSBpcyB3aHkgZGlkIHlvdSBoaXQgdGhhdD8gImJyb2tlbiBk cml2ZXIgY29kZSIgaXNuJ3QKcmVhbGx5IGEgZ29vZCByZWFzb24sIGF1IGNvbnRyYWlyZSBpdCdz IGEgcmVhc29uIHRvIG5vdCBtZXJnZSB5b3VyIHBhdGNoOgpXZSBkbyBub3Qgd2FudCB0byBoaWRl IGRyaXZlciBidWdzIHNpbGVudGx5LgoKVGhlcmUncyBkZWZpbml0ZWx5IGNhc2VzIHdoZXJlIGhh bmRsaW5nIE5VTEwgYXV0b21hdGljYWxseSBpcyByZWFzb25hYmxlLAplLmcuIGtmcmVlKCkuIEJ1 dCBhIE5VTEwgZHJtX2Zvcm1hdF9uYW1lX2J1ZiBzb3VuZHMgbGlrZSwgYXQgbGVhc3QgYSBxdWlj awpncmVwIHNob3dzIHRoYXQgYWxsIGNhbGxlcnMganVzdCBwdXQgdGhpcyBzdHJ1Y3Qgb250byB0 aGUgc3RhY2suCi1EYW5pZWwKLS0gCkRhbmllbCBWZXR0ZXIKU29mdHdhcmUgRW5naW5lZXIsIElu dGVsIENvcnBvcmF0aW9uCmh0dHA6Ly9ibG9nLmZmd2xsLmNoCl9fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRl dmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9t YWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbAo=