There is no speed difference, and it makes the code harder to read.
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Arve Hjønnevåg <[email protected]>
Cc: Todd Kjos <[email protected]>
Cc: Martijn Coenen <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: Joel Fernandes <[email protected]>
Suggested-by: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Alistair Strachan <[email protected]>
---
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;
}
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 <[email protected]>
Cc: Arve Hjønnevåg <[email protected]>
Cc: Todd Kjos <[email protected]>
Cc: Martijn Coenen <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: Joel Fernandes <[email protected]>
Signed-off-by: Alistair Strachan <[email protected]>
---
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)) {
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 <[email protected]>
> Cc: Arve Hj?nnev?g <[email protected]>
> Cc: Todd Kjos <[email protected]>
> Cc: Martijn Coenen <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: Joel Fernandes <[email protected]>
> Signed-off-by: Alistair Strachan <[email protected]>
> ---
> 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) <[email protected]>
thanks,
- Joel
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 <[email protected]>
> Cc: Arve Hj?nnev?g <[email protected]>
> Cc: Todd Kjos <[email protected]>
> Cc: Martijn Coenen <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: Joel Fernandes <[email protected]>
> Suggested-by: Greg Kroah-Hartman <[email protected]>
> Signed-off-by: Alistair Strachan <[email protected]>
Acked-by: Joel Fernandes (Google) <[email protected]>
thanks,
- Joel
On Tue, Jun 19, 2018 at 9:32 PM, Joel Fernandes <[email protected]> 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.
On Wed, Jun 20, 2018 at 02:21:57PM -0700, Daniel Colascione wrote:
> On Tue, Jun 19, 2018 at 9:32 PM, Joel Fernandes <[email protected]> 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
On Thu, Jun 21, 2018 at 1:29 AM, Joel Fernandes <[email protected]> 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 <[email protected]>
>
> thanks!
>
> - Joel
>