linux-kernel - Re: [PATCH] drm/mm: avoid possible null pointer dereference

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 23 May 2016 14:38:29 +0100
From:	Eric Engestrom <eric.engestrom@...tec.com>
To:	Chris Wilson <chris@...is-wilson.co.uk>,
	Heinrich Schuchardt <xypron.glpk@....de>,
	<linux-kernel@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH] drm/mm: avoid possible null pointer dereference

On Mon, May 23, 2016 at 02:02:26PM +0100, Chris Wilson wrote:
> On Mon, May 23, 2016 at 02:56:45PM +0200, Daniel Vetter wrote:
> > On Mon, May 23, 2016 at 11:27:14AM +0100, Eric Engestrom wrote:
> > > On Wed, May 18, 2016 at 10:17:19PM +0200, Heinrich Schuchardt wrote:
> > > > Do not dereference node before the check if node is NULL.
> > > > 
> > > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@....de>
> > > > ---
> > > >  drivers/gpu/drm/drm_mm.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
> > > > index 04de6fd..cb39f45 100644
> > > > --- a/drivers/gpu/drm/drm_mm.c
> > > > +++ b/drivers/gpu/drm/drm_mm.c
> > > > @@ -179,12 +179,14 @@ static void drm_mm_insert_helper(struct drm_mm_node *hole_node,
> > > >  int drm_mm_reserve_node(struct drm_mm *mm, struct drm_mm_node *node)
> > > >  {
> > > >  	struct drm_mm_node *hole;
> > > > -	u64 end = node->start + node->size;
> > > > +	u64 end;
> > > >  	u64 hole_start;
> > > >  	u64 hole_end;
> > > >  
> > > >  	BUG_ON(node == NULL);
> > > >  
> > > > +	end = node->start + node->size;
> > > > +
> > > >  	/* Find the relevant hole to add our node to */
> > > >  	drm_mm_for_each_hole(hole, mm, hole_start, hole_end) {
> > > >  		if (hole_start > node->start || hole_end < end)
> > > > -- 
> > > > 2.1.4
> > > 
> > > Reviewed-by: Eric Engestrom <eric.engestrom@...tec.com>
> 
> Remove useless check instead?
> -Chris

I tend to prefer erring on the side of caution and have (too) many checks,
especially simple ones like avoiding null dereferences.

That said, BUG() might be too extreme. I'm not all that familiar with
the code, but it doesn't seem like it would be an unrecoverable failure.
WARN() instead?