From: Liviu Dudau <Liviu.Dudau@arm.com> To: Jani Nikula <jani.nikula@intel.com> Cc: Daniel Vetter <daniel@ffwll.ch>, Daniel Vetter <daniel.vetter@ffwll.ch>, Eric Engestrom <eric@engestrom.ch>, LKML <linux-kernel@vger.kernel.org>, DRI devel <dri-devel@lists.freedesktop.org> Subject: Re: [PATCH v2] drm: check for NULL parameter in exported drm_get_format_name() function. Date: Wed, 23 Nov 2016 13:38:36 +0000 [thread overview] Message-ID: <20161123133835.GY1005@e106497-lin.cambridge.arm.com> (raw) In-Reply-To: <87k2buwefa.fsf@intel.com> On Wed, Nov 23, 2016 at 02:47:53PM +0200, Jani Nikula wrote: > On Wed, 23 Nov 2016, Daniel Vetter <daniel@ffwll.ch> 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 <Liviu.Dudau@arm.com> 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 <eric@engestrom.ch> > >> > > Cc: Rob Clark <robdclark@gmail.com> > >> > > Cc: Jani Nikula <jani.nikula@intel.com> > >> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > >> > > > >> > > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com> > >> > > --- > >> > > 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! / --------------- ¯\_(ツ)_/¯
WARNING: multiple messages have this Message-ID (diff)
From: Liviu Dudau <Liviu.Dudau@arm.com> To: Jani Nikula <jani.nikula@intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>, DRI devel <dri-devel@lists.freedesktop.org>, Eric Engestrom <eric@engestrom.ch>, LKML <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v2] drm: check for NULL parameter in exported drm_get_format_name() function. Date: Wed, 23 Nov 2016 13:38:36 +0000 [thread overview] Message-ID: <20161123133835.GY1005@e106497-lin.cambridge.arm.com> (raw) In-Reply-To: <87k2buwefa.fsf@intel.com> On Wed, Nov 23, 2016 at 02:47:53PM +0200, Jani Nikula wrote: > On Wed, 23 Nov 2016, Daniel Vetter <daniel@ffwll.ch> 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 <Liviu.Dudau@arm.com> 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 <eric@engestrom.ch> > >> > > Cc: Rob Clark <robdclark@gmail.com> > >> > > Cc: Jani Nikula <jani.nikula@intel.com> > >> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > >> > > > >> > > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com> > >> > > --- > >> > > 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! / --------------- ¯\_(ツ)_/¯ _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2016-11-23 13:38 UTC|newest] Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-11-22 16:41 [PATCH] drm: check for NULL parameter in exported drm_get_format_name() function Liviu Dudau 2016-11-22 16:41 ` Liviu Dudau 2016-11-22 16:50 ` Ville Syrjälä 2016-11-22 16:50 ` Ville Syrjälä 2016-11-22 17:23 ` Rob Clark 2016-11-22 17:23 ` Rob Clark 2016-11-22 17:31 ` Ville Syrjälä 2016-11-22 17:31 ` Ville Syrjälä 2016-11-22 17:35 ` Rob Clark 2016-11-22 17:35 ` Rob Clark 2016-11-22 18:06 ` Ville Syrjälä 2016-11-22 18:06 ` Ville Syrjälä 2016-11-22 18:15 ` Sean Paul 2016-11-22 18:15 ` Sean Paul 2016-11-22 18:47 ` Liviu Dudau 2016-11-22 18:47 ` Liviu Dudau 2016-11-22 21:46 ` Rob Clark 2016-11-22 21:46 ` Rob Clark 2016-11-23 10:52 ` [PATCH v2] " Liviu Dudau 2016-11-23 10:52 ` Liviu Dudau 2016-11-23 11:00 ` Jani Nikula 2016-11-23 11:00 ` Jani Nikula 2016-11-23 11:23 ` Liviu Dudau 2016-11-23 11:23 ` Liviu Dudau 2016-11-23 12:26 ` Daniel Vetter 2016-11-23 12:26 ` Daniel Vetter 2016-11-23 12:47 ` Jani Nikula 2016-11-23 12:47 ` Jani Nikula 2016-11-23 13:38 ` Liviu Dudau [this message] 2016-11-23 13:38 ` Liviu Dudau 2016-11-22 18:49 ` [PATCH] " Liviu Dudau 2016-11-22 18:49 ` Liviu Dudau
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20161123133835.GY1005@e106497-lin.cambridge.arm.com \ --to=liviu.dudau@arm.com \ --cc=daniel.vetter@ffwll.ch \ --cc=daniel@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=eric@engestrom.ch \ --cc=jani.nikula@intel.com \ --cc=linux-kernel@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.