linux-kernel - Re: Regression X Hangs at bootup -- PATCH

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]

Message-Id: <1239121300.9617.0.camel@gaiman.anholt.net>
Date:	Tue, 07 Apr 2009 09:21:40 -0700
From:	Eric Anholt <eric@...olt.net>
To:	Florian Mickler <florian@...kler.org>
Cc:	LKML <linux-kernel@...r.kernel.org>, jbarnes@...tuousgeek.org,
	airlied@...ux.ie, keithp@...thp.com
Subject: Re: Regression X Hangs at bootup -- PATCH

On Tue, 2009-04-07 at 09:23 +0200, Florian Mickler wrote:
> On Mon, 06 Apr 2009 19:03:55 -0700
> Eric Anholt <eric@...olt.net> wrote:
> 
> > Nice catch!  Thanks.  I did some cleanup that brings it more in line
> > with style elsewhere in the code and cuts some of the gratuitous
> > looking changes.  Would you be OK with these changes rolled into your
> > original diff?
> 
>  i take it, you appended the endresult?
> 
> i'm ok with it, it's less invasive. but i think your
> i915_gem_put_relocs part is wrong. (see below)
> 
> 
> > 
> >  drivers/gpu/drm/i915/i915_gem.c |   34
> > ++++++++++++++++++++++------------ 1 files changed, 22 insertions(+),
> > 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c
> > b/drivers/gpu/drm/i915/i915_gem.c index 33ab07b..6f7d0e2 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -141,15 +141,18 @@ fast_shmem_read(struct page **pages,
> >  		int length)
> >  {
> >  	char __iomem *vaddr;
> > -	int ret;
> > +	int unwritten;
> >  
> >  	vaddr = kmap_atomic(pages[page_base >> PAGE_SHIFT],
> > KM_USER0); if (vaddr == NULL)
> >  		return -ENOMEM;
> > -	ret = __copy_to_user_inatomic(data, vaddr + page_offset,
> > length);
> > +	unwritten = __copy_to_user_inatomic(data, vaddr +
> > page_offset, length); kunmap_atomic(vaddr, KM_USER0);
> >  
> > -	return ret;
> > +	if (unwritten)
> > +		return -EFAULT;
> > +
> > +	return 0;
> >  }
> 
> yep thats ok.
> 
> >  
> >  static inline int
> > @@ -3000,13 +3003,13 @@ i915_gem_get_relocs_from_user(struct
> > drm_i915_gem_exec_object *exec_list, drm_free(*relocs, reloc_count *
> > sizeof(**relocs), DRM_MEM_DRIVER);
> >  			*relocs = NULL;
> > -			return ret;
> > +			return -EFAULT;
> >  		}
> >  
> >  		reloc_index += exec_list[i].relocation_count;
> >  	}
> >  
> > -	return ret;
> > +	return 0;
> >  }
> >  
> 
> right.
> 
> >  static int
> > @@ -3015,23 +3018,28 @@ i915_gem_put_relocs_to_user(struct
> > drm_i915_gem_exec_object *exec_list, struct
> > drm_i915_gem_relocation_entry *relocs) {
> >  	uint32_t reloc_count = 0, i;
> > -	int ret;
> > +	int ret = 0;
> >  
> >  	for (i = 0; i < buffer_count; i++) {
> >  		struct drm_i915_gem_relocation_entry __user
> > *user_relocs;
> > +		int unwritten;
> >  
> >  		user_relocs = (void __user
> > *)(uintptr_t)exec_list[i].relocs_ptr; 
> > -		if (ret == 0) {
> > -			ret = copy_to_user(user_relocs,
> > -					   &relocs[reloc_count],
> > -
> > exec_list[i].relocation_count *
> > -					   sizeof(*relocs));
> > +		unwritten = copy_to_user(user_relocs,
> > +					 &relocs[reloc_count],
> > +
> > exec_list[i].relocation_count *
> > +					 sizeof(*relocs));
> > +
> > +		if (unwritten) {
> > +			ret = -EFAULT;
> > +			goto err;
> >  		}
> >  
> >  		reloc_count += exec_list[i].relocation_count;
> >  	}
> >  
> 
> i wondered too at first about the if (ret == 0) part, but you need the
> whole reloc_count to free everything in the next part:
> 
> > +err:
> >  	drm_free(relocs, reloc_count * sizeof(*relocs),
> > DRM_MEM_DRIVER); 
> >  	return ret;
> 
> 
> so i think, this would be a memleak in the error-case (if it ever
> happens)

drm_free's other arguments are unused memory debug leftovers.  I've got
a patch I need to push at airlied to remove
drm_malloc/drm_calloc/drm_free.

-- 
Eric Anholt
eric@...olt.net                         eric.anholt@...el.com



Download attachment "signature.asc" of type "application/pgp-signature" (198 bytes)