Re: [PATCH V2 1/1] staging: android: ashmem: get_name,set_name not to hold ashmem_mutex

[Dan Carpenter <dan.carpenter@oracle.com>]

On Wed, Feb 20, 2013 at 07:57:08PM +0530, Shankar Brahadeeswaran wrote:
> --- a/drivers/staging/android/ashmem.c
> +++ b/drivers/staging/android/ashmem.c
> @@ -413,50 +413,66 @@ out:
>  
>  static int set_name(struct ashmem_area *asma, void __user *name)
>  {
> -	int ret = 0;
> -
> -	mutex_lock(&ashmem_mutex);
> +	char local_name[ASHMEM_NAME_LEN];
>  
>  	/* cannot change an existing mapping's name */
> -	if (unlikely(asma->file)) {
> -		ret = -EINVAL;
> -		goto out;
> -	}
> +	if (unlikely(asma->file))
> +		return -EINVAL;

Gar.  No.  Sorry I wasn't paying attention properly last time.  This
isn't right but I didn't explain things properly from the beginning.
When you drop a lock, obviously the first thing I'm going to look at
is if it introduces race conditions.  The problem is that checking
asma->file has to be done under lock and also we can't drop the lock
before we set the name.  Otherwise someone could set asma->file
while we were waiting for the copy to complete.

It should be something like this:

	char local_name[ASHMEM_NAME_LEN];
	int ret = 0;

	if (copy_from_user(local_name, name, ASHMEM_NAME_LEN))
		return -EFAULT;
	local_name[ASHMEM_FULL_NAME_LEN - 1] = '\0';

	mutex_lock(&ashmem_mutex);
	if (asma->file) {
		ret = -EINVAL;
		goto out;
	}
	memcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name,
	       ASHMEM_NAME_LEN);
out:
	mutex_unlock(&ashmem_mutex);

	return ret;

(I removed some calls to likely/unlikely() because putting those
around copy_from_user() is probably not going to speed anything up.)

Sorry, again for the miscommunication.

regards,
dan carpenter