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 Content-Length: 2274 Lines: 52 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