[PATCH 1/2] staging: android: ashmem: Remove use of unlikely()

[PATCH 1/2] staging: android: ashmem: Remove use of unlikely()

[Alistair Strachan]

There is no speed difference, and it makes the code harder to read.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Arve Hjønnevåg <arve@android.com>
Cc: Todd Kjos <tkjos@android.com>
Cc: Martijn Coenen <maco@android.com>
Cc: devel@driverdev.osuosl.org
Cc: linux-kernel@vger.kernel.org
Cc: kernel-team@android.com
Cc: Joel Fernandes <joel@joelfernandes.org>
Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Alistair Strachan <astrachan@google.com>
---
 drivers/staging/android/ashmem.c | 34 ++++++++++++++++----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index a1a0025b59e0..c6386e4f5c9b 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -178,7 +178,7 @@ static int range_alloc(struct ashmem_area *asma,
 	struct ashmem_range *range;
 
 	range = kmem_cache_zalloc(ashmem_range_cachep, GFP_KERNEL);
-	if (unlikely(!range))
+	if (!range)
 		return -ENOMEM;
 
 	range->asma = asma;
@@ -246,11 +246,11 @@ static int ashmem_open(struct inode *inode, struct file *file)
 	int ret;
 
 	ret = generic_file_open(inode, file);
-	if (unlikely(ret))
+	if (ret)
 		return ret;
 
 	asma = kmem_cache_zalloc(ashmem_area_cachep, GFP_KERNEL);
-	if (unlikely(!asma))
+	if (!asma)
 		return -ENOMEM;
 
 	INIT_LIST_HEAD(&asma->unpinned_list);
@@ -361,14 +361,14 @@ static int ashmem_mmap(struct file *file, struct vm_area_struct *vma)
 	mutex_lock(&ashmem_mutex);
 
 	/* user needs to SET_SIZE before mapping */
-	if (unlikely(!asma->size)) {
+	if (!asma->size) {
 		ret = -EINVAL;
 		goto out;
 	}
 
 	/* requested protection bits must match our allowed protection mask */
-	if (unlikely((vma->vm_flags & ~calc_vm_prot_bits(asma->prot_mask, 0)) &
-		     calc_vm_prot_bits(PROT_MASK, 0))) {
+	if ((vma->vm_flags & ~calc_vm_prot_bits(asma->prot_mask, 0)) &
+	    calc_vm_prot_bits(PROT_MASK, 0)) {
 		ret = -EPERM;
 		goto out;
 	}
@@ -486,7 +486,7 @@ static int set_prot_mask(struct ashmem_area *asma, unsigned long prot)
 	mutex_lock(&ashmem_mutex);
 
 	/* the user can only remove, not add, protection bits */
-	if (unlikely((asma->prot_mask & prot) != prot)) {
+	if ((asma->prot_mask & prot) != prot) {
 		ret = -EINVAL;
 		goto out;
 	}
@@ -524,7 +524,7 @@ static int set_name(struct ashmem_area *asma, void __user *name)
 		local_name[ASHMEM_NAME_LEN - 1] = '\0';
 	mutex_lock(&ashmem_mutex);
 	/* cannot change an existing mapping's name */
-	if (unlikely(asma->file))
+	if (asma->file)
 		ret = -EINVAL;
 	else
 		strcpy(asma->name + ASHMEM_NAME_PREFIX_LEN, local_name);
@@ -563,7 +563,7 @@ static int get_name(struct ashmem_area *asma, void __user *name)
 	 * Now we are just copying from the stack variable to userland
 	 * No lock held
 	 */
-	if (unlikely(copy_to_user(name, local_name, len)))
+	if (copy_to_user(name, local_name, len))
 		ret = -EFAULT;
 	return ret;
 }
@@ -701,25 +701,25 @@ 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))))
+	if (copy_from_user(&pin, p, sizeof(pin)))
 		return -EFAULT;
 
 	mutex_lock(&ashmem_mutex);
 
-	if (unlikely(!asma->file))
+	if (!asma->file)
 		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;
 
-	if (unlikely((pin.offset | pin.len) & ~PAGE_MASK))
+	if ((pin.offset | pin.len) & ~PAGE_MASK)
 		goto out_unlock;
 
-	if (unlikely(((__u32)-1) - pin.offset < pin.len))
+	if (((__u32)-1) - pin.offset < pin.len)
 		goto out_unlock;
 
-	if (unlikely(PAGE_ALIGN(asma->size) < pin.offset + pin.len))
+	if (PAGE_ALIGN(asma->size) < pin.offset + pin.len)
 		goto out_unlock;
 
 	pgstart = pin.offset / PAGE_SIZE;
@@ -856,7 +856,7 @@ static int __init ashmem_init(void)
 	ashmem_area_cachep = kmem_cache_create("ashmem_area_cache",
 					       sizeof(struct ashmem_area),
 					       0, 0, NULL);
-	if (unlikely(!ashmem_area_cachep)) {
+	if (!ashmem_area_cachep) {
 		pr_err("failed to create slab cache\n");
 		goto out;
 	}
@@ -864,13 +864,13 @@ static int __init ashmem_init(void)
 	ashmem_range_cachep = kmem_cache_create("ashmem_range_cache",
 						sizeof(struct ashmem_range),
 						0, 0, NULL);
-	if (unlikely(!ashmem_range_cachep)) {
+	if (!ashmem_range_cachep) {
 		pr_err("failed to create slab cache\n");
 		goto out_free1;
 	}
 
 	ret = misc_register(&ashmem_misc);
-	if (unlikely(ret)) {
+	if (ret) {
 		pr_err("failed to register misc device!\n");
 		goto out_free2;
 	}

[PATCH 2/2 v2] staging: android: ashmem: Fix mmap size validation

[Alistair Strachan]

The ashmem driver did not check that the size/offset of the vma passed
to its .mmap() function was not larger than the ashmem object being
mapped. This could cause mmap() to succeed, even though accessing parts
of the mapping would later fail with a segmentation fault.

Ensure an error is returned by the ashmem_mmap() function if the vma
size is larger than the ashmem object size. This enables safer handling
of the problem in userspace.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Arve Hjønnevåg <arve@android.com>
Cc: Todd Kjos <tkjos@android.com>
Cc: Martijn Coenen <maco@android.com>
Cc: devel@driverdev.osuosl.org
Cc: linux-kernel@vger.kernel.org
Cc: kernel-team@android.com
Cc: Joel Fernandes <joel@joelfernandes.org>
Signed-off-by: Alistair Strachan <astrachan@google.com>
---
v2: Removed unnecessary use of unlikely() macro

 drivers/staging/android/ashmem.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index c6386e4f5c9b..e392358ec244 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -366,6 +366,12 @@ static int ashmem_mmap(struct file *file, struct vm_area_struct *vma)
 		goto out;
 	}
 
+	/* requested mapping size larger than object size */
+	if (vma->vm_end - vma->vm_start > PAGE_ALIGN(asma->size)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	/* requested protection bits must match our allowed protection mask */
 	if ((vma->vm_flags & ~calc_vm_prot_bits(asma->prot_mask, 0)) &
 	    calc_vm_prot_bits(PROT_MASK, 0)) {

Re: [PATCH 2/2 v2] staging: android: ashmem: Fix mmap size validation

[Joel Fernandes]

On Tue, Jun 19, 2018 at 05:57:35PM -0700, Alistair Strachan wrote:
> The ashmem driver did not check that the size/offset of the vma passed
> to its .mmap() function was not larger than the ashmem object being
> mapped. This could cause mmap() to succeed, even though accessing parts
> of the mapping would later fail with a segmentation fault.
> 
> Ensure an error is returned by the ashmem_mmap() function if the vma
> size is larger than the ashmem object size. This enables safer handling
> of the problem in userspace.
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Arve Hjønnevåg <arve@android.com>
> Cc: Todd Kjos <tkjos@android.com>
> Cc: Martijn Coenen <maco@android.com>
> Cc: devel@driverdev.osuosl.org
> Cc: linux-kernel@vger.kernel.org
> Cc: kernel-team@android.com
> Cc: Joel Fernandes <joel@joelfernandes.org>
> Signed-off-by: Alistair Strachan <astrachan@google.com>
> ---
> v2: Removed unnecessary use of unlikely() macro
> 
>  drivers/staging/android/ashmem.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
> index c6386e4f5c9b..e392358ec244 100644
> --- a/drivers/staging/android/ashmem.c
> +++ b/drivers/staging/android/ashmem.c
> @@ -366,6 +366,12 @@ static int ashmem_mmap(struct file *file, struct vm_area_struct *vma)
>  		goto out;
>  	}
>  
> +	/* requested mapping size larger than object size */
> +	if (vma->vm_end - vma->vm_start > PAGE_ALIGN(asma->size)) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +

Acked-by: Joel Fernandes (Google) <joel@joelfernandes.org>

thanks,

 - Joel

Re: [PATCH 1/2] staging: android: ashmem: Remove use of unlikely()

[Joel Fernandes]

On Tue, Jun 19, 2018 at 05:57:34PM -0700, Alistair Strachan wrote:
> There is no speed difference, and it makes the code harder to read.
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Arve Hjønnevåg <arve@android.com>
> Cc: Todd Kjos <tkjos@android.com>
> Cc: Martijn Coenen <maco@android.com>
> Cc: devel@driverdev.osuosl.org
> Cc: linux-kernel@vger.kernel.org
> Cc: kernel-team@android.com
> Cc: Joel Fernandes <joel@joelfernandes.org>
> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Alistair Strachan <astrachan@google.com>

Acked-by: Joel Fernandes (Google) <joel@joelfernandes.org>

thanks,

- Joel

Re: [PATCH 2/2 v2] staging: android: ashmem: Fix mmap size validation

[Daniel Colascione]

On Tue, Jun 19, 2018 at 9:32 PM, Joel Fernandes <joel@joelfernandes.org> wrote:
> On Tue, Jun 19, 2018 at 05:57:35PM -0700, Alistair Strachan wrote:
> > The ashmem driver did not check that the size/offset of the vma passed
> > to its .mmap() function was not larger than the ashmem object being
> > mapped. This could cause mmap() to succeed, even though accessing parts
> > of the mapping would later fail with a segmentation fault.
> >
> > Ensure an error is returned by the ashmem_mmap() function if the vma
> > size is larger than the ashmem object size. This enables safer handling
> > of the problem in userspace.

Are we sure that this approach is a good idea? You can over-mmap
regular files. I don't like the idea of creating special mmap
semantics for files that happen to be ashmem files. Ashmem users can
detect size-changing shenanigans with ASHMEM_GET_SIZE after mmap,
since an ashmem file's size can't change after an mmap call succeeds.

Re: [PATCH 2/2 v2] staging: android: ashmem: Fix mmap size validation

[Joel Fernandes]

On Wed, Jun 20, 2018 at 02:21:57PM -0700, Daniel Colascione wrote:
> On Tue, Jun 19, 2018 at 9:32 PM, Joel Fernandes <joel@joelfernandes.org> wrote:
> > On Tue, Jun 19, 2018 at 05:57:35PM -0700, Alistair Strachan wrote:
> > > The ashmem driver did not check that the size/offset of the vma passed
> > > to its .mmap() function was not larger than the ashmem object being
> > > mapped. This could cause mmap() to succeed, even though accessing parts
> > > of the mapping would later fail with a segmentation fault.
> > >
> > > Ensure an error is returned by the ashmem_mmap() function if the vma
> > > size is larger than the ashmem object size. This enables safer handling
> > > of the problem in userspace.
> 
> Are we sure that this approach is a good idea? You can over-mmap
> regular files. I don't like the idea of creating special mmap

ashmem isn't a regular file.

> semantics for files that happen to be ashmem files. Ashmem users can
> detect size-changing shenanigans with ASHMEM_GET_SIZE after mmap,
> since an ashmem file's size can't change after an mmap call succeeds.

But it is misleading to allow it. If the mmap succeeds, the any writes to the
extra area is infact not a part of ashmem and will not be shared. Instead if
the mmap fails up front, then we're telling the user upfront that they
screwed up and they should do something about it. I would much rather have
the mmap fail than to allow for other issues to later occur.

Also if you look at the kernel sources, there are dozens of drivers that
check for correct VMA size in mmap handler and fail if it isn't sized
correctly.

thanks!

 - Joel

Re: [PATCH 2/2 v2] staging: android: ashmem: Fix mmap size validation

[Martijn Coenen]

On Thu, Jun 21, 2018 at 1:29 AM, Joel Fernandes <joel@joelfernandes.org> wrote:
> Also if you look at the kernel sources, there are dozens of drivers that
> check for correct VMA size in mmap handler and fail if it isn't sized
> correctly.

If that's the case, we should definitely do it this way for ashmem as
well. Since its size is fixed after creation, preventing anyone from
mapping a larger size seems reasonable to me.

Reviewed-by: Martijn Coenen <maco@android.com>

>
> thanks!
>
>  - Joel
>