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 Content-Length: 1975 Lines: 41 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