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

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: <20130220144853.GE9189@mwanda>
Date:	Wed, 20 Feb 2013 17:48:53 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Shankar Brahadeeswaran <shankoo77@...il.com>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org, devel@...verdev.osuosl.org,
	Cruz Julian Bishop <cruzjbishop@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Hugh Dickins <hughd@...gle.com>,
	Konstantin Khlebnikov <khlebnikov@...nvz.org>
Subject: Re: [PATCH V2 1/1] staging: android: ashmem: get_name,set_name not
 to hold ashmem_mutex

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/