From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm: check for NULL parameter in exported drm_get_format_name() function. Date: Tue, 22 Nov 2016 20:06:19 +0200 Message-ID: <20161122180619.GE31595@intel.com> References: <20161122164106.31852-1-Liviu.Dudau@arm.com> <20161122165017.GC31595@intel.com> <20161122173157.GD31595@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by gabe.freedesktop.org (Postfix) with ESMTPS id 114886E355 for ; Tue, 22 Nov 2016 18:06:25 +0000 (UTC) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Rob Clark Cc: Jani Nikula , Daniel Vetter , Eric Engestrom , Liviu Dudau , LKML , DRI devel List-Id: dri-devel@lists.freedesktop.org T24gVHVlLCBOb3YgMjIsIDIwMTYgYXQgMTI6MzU6NTNQTSAtMDUwMCwgUm9iIENsYXJrIHdyb3Rl Ogo+IE9uIFR1ZSwgTm92IDIyLCAyMDE2IGF0IDEyOjMxIFBNLCBWaWxsZSBTeXJqw6Rsw6QKPiA8 dmlsbGUuc3lyamFsYUBsaW51eC5pbnRlbC5jb20+IHdyb3RlOgo+ID4gT24gVHVlLCBOb3YgMjIs IDIwMTYgYXQgMTI6MjM6NTlQTSAtMDUwMCwgUm9iIENsYXJrIHdyb3RlOgo+ID4+IE9uIFR1ZSwg Tm92IDIyLCAyMDE2IGF0IDExOjUwIEFNLCBWaWxsZSBTeXJqw6Rsw6QKPiA+PiA8dmlsbGUuc3ly amFsYUBsaW51eC5pbnRlbC5jb20+IHdyb3RlOgo+ID4+ID4gT24gVHVlLCBOb3YgMjIsIDIwMTYg YXQgMDQ6NDE6MDZQTSArMDAwMCwgTGl2aXUgRHVkYXUgd3JvdGU6Cj4gPj4gPj4gZHJtX2dldF9m b3JtYXRfbmFtZSgpIGRlLXJlZmVyZW5jZXMgdGhlIGJ1ZiBwYXJhbWV0ZXIgd2l0aG91dCBjaGVj a2luZwo+ID4+ID4+IGlmIHRoZSBwb2ludGVyIHdhcyBub3QgTlVMTC4gR2l2ZW4gdGhhdCB0aGUg ZnVuY3Rpb24gaXMgRVhQT1JULWVkLCBsZXRzCj4gPj4gPj4gc2FuaXRpc2UgdGhlIHBhcmFtZXRl cnMgYmVmb3JlIHByb2NlZWRpbmcuCj4gPj4gPj4KPiA+PiA+PiBGaXhlczogYjNjMTFhYzI2N2Q0 NjFkM2Q1ICgiZHJtOiBtb3ZlIGFsbG9jYXRpb24gb3V0IG9mIGRybV9nZXRfZm9ybWF0X25hbWUo KSkKPiA+PiA+PiBDYzogRXJpYyBFbmdlc3Ryb20gPGVyaWNAZW5nZXN0cm9tLmNoPgo+ID4+ID4+ IENjOiBSb2IgQ2xhcmsgPHJvYmRjbGFya0BnbWFpbC5jb20+Cj4gPj4gPj4gQ2M6IEphbmkgTmlr dWxhIDxqYW5pLm5pa3VsYUBpbnRlbC5jb20+Cj4gPj4gPj4gQ2M6IERhbmllbCBWZXR0ZXIgPGRh bmllbC52ZXR0ZXJAZmZ3bGwuY2g+Cj4gPj4gPj4KPiA+PiA+PiBTaWduZWQtb2ZmLWJ5OiBMaXZp dSBEdWRhdSA8TGl2aXUuRHVkYXVAYXJtLmNvbT4KPiA+PiA+PiAtLS0KPiA+PiA+PiAgZHJpdmVy cy9ncHUvZHJtL2RybV9mb3VyY2MuYyB8IDMgKysrCj4gPj4gPj4gIDEgZmlsZSBjaGFuZ2VkLCAz IGluc2VydGlvbnMoKykKPiA+PiA+Pgo+ID4+ID4+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2dwdS9k cm0vZHJtX2ZvdXJjYy5jIGIvZHJpdmVycy9ncHUvZHJtL2RybV9mb3VyY2MuYwo+ID4+ID4+IGlu ZGV4IDkwZDJjYzguLjBhM2ZmMGIgMTAwNjQ0Cj4gPj4gPj4gLS0tIGEvZHJpdmVycy9ncHUvZHJt L2RybV9mb3VyY2MuYwo+ID4+ID4+ICsrKyBiL2RyaXZlcnMvZ3B1L2RybS9kcm1fZm91cmNjLmMK PiA+PiA+PiBAQCAtODUsNiArODUsOSBAQCBFWFBPUlRfU1lNQk9MKGRybV9tb2RlX2xlZ2FjeV9m Yl9mb3JtYXQpOwo+ID4+ID4+ICAgKi8KPiA+PiA+PiAgY29uc3QgY2hhciAqZHJtX2dldF9mb3Jt YXRfbmFtZSh1aW50MzJfdCBmb3JtYXQsIHN0cnVjdCBkcm1fZm9ybWF0X25hbWVfYnVmICpidWYp Cj4gPj4gPj4gIHsKPiA+PiA+PiArICAgICBpZiAoIWJ1ZikKPiA+PiA+PiArICAgICAgICAgICAg IHJldHVybiBOVUxMOwo+ID4+ID4+ICsKPiA+PiA+Cj4gPj4gPiBTZWVtcyByYXRoZXIgcG9pbnRs ZXNzIHRvIG1lLiBXaHkgd291bGQgeW91IGV2ZXIgcGFzcyBOVUxMIHRvIHRoaXMgZ3V5Pwo+ID4+ Cj4gPj4gcGVyaGFwcyBCVUdfT04oIWJ1ZikuLi4KPiA+Cj4gPiBBbmQgaG93IGRvZXMgdGhhdCBk aWZmZXIgZnJvbSBqdXN0IGJ1Zi0+Zm9vPwo+IAo+IGl0IGdldHMgeW91IGEgZmlsZSBhbmQgbGlu ZSAjIGluIHRoZSBlcnJvciBzcGxhdC4uIG5vdCB0aGF0Cj4gZHJtX2dldF9mb3JtYXRfbmFtZSgp IGlzIHN1Y2ggYSBiaWcgZnVuY3Rpb24gdGhhdCBpdCB3b3VsZCBiZQo+IGRpZmZpY3VsdCB0byBk ZWNpcGhlciB0aGUgbnVsbCBkZXJlZiBjcmFzaCwgYnV0IGlmIHdlIGFkZGVkIGFueXRoaW5nCj4g aXQgc2hvdWxkIGJlIEJVR19PTigpIHRvIG1ha2UgaXQgY2xlYXIgdGhhdCBwYXNzaW5nIG51bGwg aXNuJ3QgYQo+IGNhbGxlciBlcnJvci4KClllYWgsIEJVR19PTigpIGF0IGxlYXN0IGRvY3VtZW50 cyB0aGUgaW50ZW50LCBzbyBpdCdzIGJldHRlciB0aGFuCnRoZSBudWxsIGNoZWNrLiBCdXQgZm9y IHNvbWV0aGluZyBsaWtlIHRoaXMgZXZlbiBCVUdfT04oKSBpcwpqdXN0IHdhc3RlZCBieXRlcyBJ TU8uCgpCVUdfT04oKSBjYW4gYmUgdXNlZnVsIGZvciB0aG9zZSB3ZWlyZCBidWdzIHdoZXJlIHNv bWV3aGVyZSBkZWVwCmRvd24geW91IGhpdCBhIG51bGwgcG9pbnRlciBhbmQgeW91IGNhbid0IGZp Z3VyZSBvdXQgd2hlcmUgdGhlCmJhZCBwb2ludGVyIGNhbWUgZnJvbS4gU28geW91IG1pZ2h0IHNw cmlua2xlIGEgZmV3IEJVR19PTnMoKQpmdXJ0aGVyIHVwIHRvIGNhdGNoIGl0IHNvb25lci4gRXNw LiBpZiB5b3UgY2FuJ3QgcmVwcm9kdWNlIHRoZQpidWcgeW91cnNlbGYgYW5kIGhhdmUgdG8gcmVs eSBvbiB1c2VyKHMpIHRvIGZpbmQgaXQgZm9yIHlvdS4KCkV2ZW4gV0FSTl9PTigpIHcvIG9yIHcv byBhbiBlYXJseSBiYWlsb3V0IG1pZ2h0IGJlIGEgZGVjZW50IGlkZWEKc29tZXRpbWVzIHNpbmNl IGl0IG1pZ2h0IGhhdmUgYSBzbGlnaHRseSBoaWdoZXIgY2hhbmNlIG9mIGtlZXBpbmcKdGhlIGtl cm5lbCBpbiB3b3JraW5nIGNvbmRpdGlvbiwgYnV0IElNTyBqdXN0IGJsaW5kbHkgdGhyb3dpbmcK aXQgYXJvdW5kIGV2ZXJ5d2hlcmUgaXMgbm90IGEgZ29vZCBhcHByb2FjaC4KCi0tIApWaWxsZSBT eXJqw6Rsw6QKSW50ZWwgT1RDCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNr dG9wLm9yZwpodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2Ry aS1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934176AbcKVSG0 (ORCPT ); Tue, 22 Nov 2016 13:06:26 -0500 Received: from mga07.intel.com ([134.134.136.100]:27244 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932768AbcKVSGZ (ORCPT ); Tue, 22 Nov 2016 13:06:25 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,533,1473145200"; d="scan'208";a="8031676" Date: Tue, 22 Nov 2016 20:06:19 +0200 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Rob Clark Cc: Liviu Dudau , Jani Nikula , Daniel Vetter , Eric Engestrom , LKML , DRI devel Subject: Re: [PATCH] drm: check for NULL parameter in exported drm_get_format_name() function. Message-ID: <20161122180619.GE31595@intel.com> References: <20161122164106.31852-1-Liviu.Dudau@arm.com> <20161122165017.GC31595@intel.com> <20161122173157.GD31595@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: 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 Tue, Nov 22, 2016 at 12:35:53PM -0500, Rob Clark wrote: > On Tue, Nov 22, 2016 at 12:31 PM, Ville Syrjälä > wrote: > > On Tue, Nov 22, 2016 at 12:23:59PM -0500, Rob Clark wrote: > >> On Tue, Nov 22, 2016 at 11:50 AM, Ville Syrjälä > >> wrote: > >> > On Tue, Nov 22, 2016 at 04:41:06PM +0000, 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. > >> >> > >> >> 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 > >> >> --- > >> >> drivers/gpu/drm/drm_fourcc.c | 3 +++ > >> >> 1 file changed, 3 insertions(+) > >> >> > >> >> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c > >> >> index 90d2cc8..0a3ff0b 100644 > >> >> --- a/drivers/gpu/drm/drm_fourcc.c > >> >> +++ b/drivers/gpu/drm/drm_fourcc.c > >> >> @@ -85,6 +85,9 @@ EXPORT_SYMBOL(drm_mode_legacy_fb_format); > >> >> */ > >> >> const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf) > >> >> { > >> >> + if (!buf) > >> >> + return NULL; > >> >> + > >> > > >> > Seems rather pointless to me. Why would you ever pass NULL to this guy? > >> > >> perhaps BUG_ON(!buf)... > > > > And how does that differ from just buf->foo? > > it gets you a file and line # in the error splat.. not that > drm_get_format_name() is such a big function that it would be > difficult to decipher the null deref crash, but if we added anything > it should be BUG_ON() to make it clear that passing null isn't a > caller error. Yeah, BUG_ON() at least documents the intent, so it's better than the null check. But for something like this even BUG_ON() is just wasted bytes IMO. BUG_ON() can be useful for those weird bugs where somewhere deep down you hit a null pointer and you can't figure out where the bad pointer came from. So you might sprinkle a few BUG_ONs() further up to catch it sooner. Esp. if you can't reproduce the bug yourself and have to rely on user(s) to find it for you. Even WARN_ON() w/ or w/o an early bailout might be a decent idea sometimes since it might have a slightly higher chance of keeping the kernel in working condition, but IMO just blindly throwing it around everywhere is not a good approach. -- Ville Syrjälä Intel OTC