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 Content-Length: 3019 Lines: 75 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! / --------------- ¯\_(ツ)_/¯