[PATCH] staging: android: ashmem: Remove deadlock

[PATCH] staging: android: ashmem: Remove deadlock

[Paul Lawrence]

Regression introduced in commit ce8a3a9e76d0193e2e8d74a06d275b3c324ca652
("staging: android: ashmem: Fix a race condition in pin ioctls")
causing deadlock.

No need to hold ashmem_mutex while copying from user

Stacks are:

ashmem_mmap+0x53/0x400 drivers/staging/android/ashmem.c:379
mmap_region+0x7dd/0xfd0 mm/mmap.c:1694
do_mmap+0x57b/0xbe0 mm/mmap.c:1473

and

lock_acquire+0x12e/0x410 kernel/locking/lockdep.c:3756
__might_fault+0x14a/0x1d0 mm/memory.c:4014
copy_from_user arch/x86/include/asm/uaccess.h:705 [inline]
ashmem_pin_unpin drivers/staging/android/ashmem.c:719 [inline]

Signed-off-by: Paul Lawrence <paullawrence@google.com>
Cc: <stable@vger.kernel.org> # 4.9.x
Cc: <stable@vger.kernel.org> # 4.4.x
Cc: <stable@vger.kernel.org> # 3.18.x: ce8a3a9e76d01
Cc: <stable@vger.kernel.org> # 3.18.x
Cc: Ben Hutchings <ben@decadent.org.uk>
---
 drivers/staging/android/ashmem.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index 6dbba5aff191..8c55706c2cfa 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -702,16 +702,14 @@ static int ashmem_pin_unpin(struct ashmem_area *asma, unsigned long cmd,
 	size_t pgstart, pgend;
 	int ret = -EINVAL;
 
+	if (unlikely(copy_from_user(&pin, p, sizeof(pin))))
+		return -EFAULT;
+
 	mutex_lock(&ashmem_mutex);
 
 	if (unlikely(!asma->file))
 		goto out_unlock;
 
-	if (unlikely(copy_from_user(&pin, p, sizeof(pin)))) {
-		ret = -EFAULT;
-		goto out_unlock;
-	}
-
 	/* per custom, you can pass zero for len to mean "everything onward" */
 	if (!pin.len)
 		pin.len = PAGE_ALIGN(asma->size) - pin.offset;
-- 
2.16.2.395.g2e18187dfd-goog

Re: [PATCH] staging: android: ashmem: Remove deadlock

[Nathan Chancellor]

On Wed, Mar 07, 2018 at 01:40:30PM -0800, Paul Lawrence wrote:
> Regression introduced in commit ce8a3a9e76d0193e2e8d74a06d275b3c324ca652
> ("staging: android: ashmem: Fix a race condition in pin ioctls")
> causing deadlock.
> 
> No need to hold ashmem_mutex while copying from user
> 
> Stacks are:
> 
> ashmem_mmap+0x53/0x400 drivers/staging/android/ashmem.c:379
> mmap_region+0x7dd/0xfd0 mm/mmap.c:1694
> do_mmap+0x57b/0xbe0 mm/mmap.c:1473
> 
> and
> 
> lock_acquire+0x12e/0x410 kernel/locking/lockdep.c:3756
> __might_fault+0x14a/0x1d0 mm/memory.c:4014
> copy_from_user arch/x86/include/asm/uaccess.h:705 [inline]
> ashmem_pin_unpin drivers/staging/android/ashmem.c:719 [inline]
> 
> Signed-off-by: Paul Lawrence <paullawrence@google.com>
> Cc: <stable@vger.kernel.org> # 4.9.x
> Cc: <stable@vger.kernel.org> # 4.4.x
> Cc: <stable@vger.kernel.org> # 3.18.x: ce8a3a9e76d01
> Cc: <stable@vger.kernel.org> # 3.18.x
> Cc: Ben Hutchings <ben@decadent.org.uk>
> ---
>  drivers/staging/android/ashmem.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
> index 6dbba5aff191..8c55706c2cfa 100644
> --- a/drivers/staging/android/ashmem.c
> +++ b/drivers/staging/android/ashmem.c
> @@ -702,16 +702,14 @@ static int ashmem_pin_unpin(struct ashmem_area *asma, unsigned long cmd,
>  	size_t pgstart, pgend;
>  	int ret = -EINVAL;
>  
> +	if (unlikely(copy_from_user(&pin, p, sizeof(pin))))
> +		return -EFAULT;
> +
>  	mutex_lock(&ashmem_mutex);
>  
>  	if (unlikely(!asma->file))
>  		goto out_unlock;
>  
> -	if (unlikely(copy_from_user(&pin, p, sizeof(pin)))) {
> -		ret = -EFAULT;
> -		goto out_unlock;
> -	}
> -
>  	/* per custom, you can pass zero for len to mean "everything onward" */
>  	if (!pin.len)
>  		pin.len = PAGE_ALIGN(asma->size) - pin.offset;
> -- 
> 2.16.2.395.g2e18187dfd-goog
>

Hey Paul,

Looks like this same patch is already in Greg's tree:
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?id=740a5759bf222332fbb5eda42f89aa25ba38f9b2

Cheers!
Nathan Chancellor

Re: [PATCH] staging: android: ashmem: Remove deadlock

[Greg Kroah-Hartman]

On Wed, Mar 07, 2018 at 02:02:13PM -0800, Paul Lawrence wrote:
> Great! We need to make sure this gets backported to 4.4 and 4.9, and to
> 3.18 with the original dependency, please.

That will happen when it lands in Linus's tree, which should be later
this week if all goes well.

thanks,

greg k-h

Re: [PATCH] staging: android: ashmem: Remove deadlock

[Dan Carpenter]

Thanks!  Somebody already sent this.

On Wed, Mar 07, 2018 at 01:40:30PM -0800, Paul Lawrence wrote:
> Regression introduced in commit ce8a3a9e76d0193e2e8d74a06d275b3c324ca652
> ("staging: android: ashmem: Fix a race condition in pin ioctls")
> causing deadlock.

Use the Fixes tag for this.  The format is:

Fixes: ce8a3a9e76d0 ("staging: android: ashmem: Fix a race condition in pin ioctls")

Then you only need one stable CC line.  Just the oldest kernel which
needs to be patched.

Cc: <stable@vger.kernel.org> # 3.18.x

The extra git hash after the stable CC is for if a bug requires several
patches to fix it.  In this case it only requires just this patch so
there's no need to have the hash.

Basically you did extra work which isn't required.  Anyway, this fix was
already merged so no need to resend or anything.

regards,
dan carpenter