stable.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] staging: android: ashmem: Disallow ashmem memory from being remapped
@ 2020-01-27 21:00 Todd Kjos
  2020-01-27 22:30 ` Joel Fernandes
  0 siblings, 1 reply; 4+ messages in thread
From: Todd Kjos @ 2020-01-27 21:00 UTC (permalink / raw)
  To: tkjos, surenb, gregkh, arve, devel, linux-kernel, maco
  Cc: joel, kernel-team, Jann Horn, stable

From: Suren Baghdasaryan <surenb@google.com>

When ashmem file is being mmapped the resulting vma->vm_file points to the
backing shmem file with the generic fops that do not check ashmem
permissions like fops of ashmem do. Fix that by disallowing mapping
operation for backing shmem file.

Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Suren Baghdasaryan <surenb@google.com>
Cc: stable <stable@vger.kernel.org> # 4.4,4.9,4.14,4.18,5.4
Signed-off-by: Todd Kjos <tkjos@google.com>
---
 drivers/staging/android/ashmem.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index 74d497d39c5a..c6695354b123 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -351,8 +351,23 @@ static inline vm_flags_t calc_vm_may_flags(unsigned long prot)
 	       _calc_vm_trans(prot, PROT_EXEC,  VM_MAYEXEC);
 }
 
+static int ashmem_vmfile_mmap(struct file *file, struct vm_area_struct *vma)
+{
+	/* do not allow to mmap ashmem backing shmem file directly */
+	return -EPERM;
+}
+
+static unsigned long
+ashmem_vmfile_get_unmapped_area(struct file *file, unsigned long addr,
+				unsigned long len, unsigned long pgoff,
+				unsigned long flags)
+{
+	return current->mm->get_unmapped_area(file, addr, len, pgoff, flags);
+}
+
 static int ashmem_mmap(struct file *file, struct vm_area_struct *vma)
 {
+	static struct file_operations vmfile_fops;
 	struct ashmem_area *asma = file->private_data;
 	int ret = 0;
 
@@ -393,6 +408,19 @@ static int ashmem_mmap(struct file *file, struct vm_area_struct *vma)
 		}
 		vmfile->f_mode |= FMODE_LSEEK;
 		asma->file = vmfile;
+		/*
+		 * override mmap operation of the vmfile so that it can't be
+		 * remapped which would lead to creation of a new vma with no
+		 * asma permission checks. Have to override get_unmapped_area
+		 * as well to prevent VM_BUG_ON check for f_ops modification.
+		 */
+		if (!vmfile_fops.mmap) {
+			vmfile_fops = *vmfile->f_op;
+			vmfile_fops.mmap = ashmem_vmfile_mmap;
+			vmfile_fops.get_unmapped_area =
+					ashmem_vmfile_get_unmapped_area;
+		}
+		vmfile->f_op = &vmfile_fops;
 	}
 	get_file(asma->file);
 
-- 
2.25.0.341.g760bfbb309-goog


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] staging: android: ashmem: Disallow ashmem memory from being remapped
  2020-01-27 21:00 [PATCH] staging: android: ashmem: Disallow ashmem memory from being remapped Todd Kjos
@ 2020-01-27 22:30 ` Joel Fernandes
  2020-01-27 23:56   ` Todd Kjos
  0 siblings, 1 reply; 4+ messages in thread
From: Joel Fernandes @ 2020-01-27 22:30 UTC (permalink / raw)
  To: Todd Kjos
  Cc: Suren Baghdasaryan, Greg Kroah-Hartman, Arve Hjonnevag,
	open list:ANDROID DRIVERS, LKML, Martijn Coenen,
	Joel Fernandes (Google),
	Cc: Android Kernel, Jann Horn, stable

On Mon, Jan 27, 2020 at 1:00 PM 'Todd Kjos' via kernel-team
<kernel-team@android.com> wrote:
>
> From: Suren Baghdasaryan <surenb@google.com>
>
> When ashmem file is being mmapped the resulting vma->vm_file points to the
> backing shmem file with the generic fops that do not check ashmem
> permissions like fops of ashmem do. Fix that by disallowing mapping
> operation for backing shmem file.

Looks good, but I think the commit message is confusing. I had to read
the code a couple times to understand what's going on since there are
no links to a PoC for the security issue, in the commit message. I
think a better message could have been:

 When ashmem file is mmapped, the resulting vma->vm_file points to the
 backing shmem file with the generic fops that do not check ashmem
 permissions like fops of ashmem do. If an mremap is done on the ashmem
 region, then the permission checks will be skipped. Fix that by disallowing
 mapping operation on the backing shmem file.

Or did I miss something?

thanks!

- Joel



>
> Reported-by: Jann Horn <jannh@google.com>
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> Cc: stable <stable@vger.kernel.org> # 4.4,4.9,4.14,4.18,5.4
> Signed-off-by: Todd Kjos <tkjos@google.com>
> ---
>  drivers/staging/android/ashmem.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
> index 74d497d39c5a..c6695354b123 100644
> --- a/drivers/staging/android/ashmem.c
> +++ b/drivers/staging/android/ashmem.c
> @@ -351,8 +351,23 @@ static inline vm_flags_t calc_vm_may_flags(unsigned long prot)
>                _calc_vm_trans(prot, PROT_EXEC,  VM_MAYEXEC);
>  }
>
> +static int ashmem_vmfile_mmap(struct file *file, struct vm_area_struct *vma)
> +{
> +       /* do not allow to mmap ashmem backing shmem file directly */
> +       return -EPERM;
> +}
> +
> +static unsigned long
> +ashmem_vmfile_get_unmapped_area(struct file *file, unsigned long addr,
> +                               unsigned long len, unsigned long pgoff,
> +                               unsigned long flags)
> +{
> +       return current->mm->get_unmapped_area(file, addr, len, pgoff, flags);
> +}
> +
>  static int ashmem_mmap(struct file *file, struct vm_area_struct *vma)
>  {
> +       static struct file_operations vmfile_fops;
>         struct ashmem_area *asma = file->private_data;
>         int ret = 0;
>
> @@ -393,6 +408,19 @@ static int ashmem_mmap(struct file *file, struct vm_area_struct *vma)
>                 }
>                 vmfile->f_mode |= FMODE_LSEEK;
>                 asma->file = vmfile;
> +               /*
> +                * override mmap operation of the vmfile so that it can't be
> +                * remapped which would lead to creation of a new vma with no
> +                * asma permission checks. Have to override get_unmapped_area
> +                * as well to prevent VM_BUG_ON check for f_ops modification.
> +                */
> +               if (!vmfile_fops.mmap) {
> +                       vmfile_fops = *vmfile->f_op;
> +                       vmfile_fops.mmap = ashmem_vmfile_mmap;
> +                       vmfile_fops.get_unmapped_area =
> +                                       ashmem_vmfile_get_unmapped_area;
> +               }
> +               vmfile->f_op = &vmfile_fops;
>         }
>         get_file(asma->file);
>
> --
> 2.25.0.341.g760bfbb309-goog
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] staging: android: ashmem: Disallow ashmem memory from being remapped
  2020-01-27 22:30 ` Joel Fernandes
@ 2020-01-27 23:56   ` Todd Kjos
  2020-01-28  0:05     ` Suren Baghdasaryan
  0 siblings, 1 reply; 4+ messages in thread
From: Todd Kjos @ 2020-01-27 23:56 UTC (permalink / raw)
  To: Joel Fernandes
  Cc: Suren Baghdasaryan, Greg Kroah-Hartman, Arve Hjonnevag,
	open list:ANDROID DRIVERS, LKML, Martijn Coenen,
	Joel Fernandes (Google),
	Cc: Android Kernel, Jann Horn, stable

On Mon, Jan 27, 2020 at 2:30 PM Joel Fernandes <joelaf@google.com> wrote:
>
> On Mon, Jan 27, 2020 at 1:00 PM 'Todd Kjos' via kernel-team
> <kernel-team@android.com> wrote:
> >
> > From: Suren Baghdasaryan <surenb@google.com>
> >
> > When ashmem file is being mmapped the resulting vma->vm_file points to the
> > backing shmem file with the generic fops that do not check ashmem
> > permissions like fops of ashmem do. Fix that by disallowing mapping
> > operation for backing shmem file.
>
> Looks good, but I think the commit message is confusing. I had to read
> the code a couple times to understand what's going on since there are
> no links to a PoC for the security issue, in the commit message. I
> think a better message could have been:
>
>  When ashmem file is mmapped, the resulting vma->vm_file points to the
>  backing shmem file with the generic fops that do not check ashmem
>  permissions like fops of ashmem do. If an mremap is done on the ashmem
>  region, then the permission checks will be skipped. Fix that by disallowing
>  mapping operation on the backing shmem file.

Sent v2 with the suggested change.

>
> Or did I miss something?
>
> thanks!
>
> - Joel
>
>
>
> >
> > Reported-by: Jann Horn <jannh@google.com>
> > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > Cc: stable <stable@vger.kernel.org> # 4.4,4.9,4.14,4.18,5.4
> > Signed-off-by: Todd Kjos <tkjos@google.com>
> > ---
> >  drivers/staging/android/ashmem.c | 28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> >
> > diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
> > index 74d497d39c5a..c6695354b123 100644
> > --- a/drivers/staging/android/ashmem.c
> > +++ b/drivers/staging/android/ashmem.c
> > @@ -351,8 +351,23 @@ static inline vm_flags_t calc_vm_may_flags(unsigned long prot)
> >                _calc_vm_trans(prot, PROT_EXEC,  VM_MAYEXEC);
> >  }
> >
> > +static int ashmem_vmfile_mmap(struct file *file, struct vm_area_struct *vma)
> > +{
> > +       /* do not allow to mmap ashmem backing shmem file directly */
> > +       return -EPERM;
> > +}
> > +
> > +static unsigned long
> > +ashmem_vmfile_get_unmapped_area(struct file *file, unsigned long addr,
> > +                               unsigned long len, unsigned long pgoff,
> > +                               unsigned long flags)
> > +{
> > +       return current->mm->get_unmapped_area(file, addr, len, pgoff, flags);
> > +}
> > +
> >  static int ashmem_mmap(struct file *file, struct vm_area_struct *vma)
> >  {
> > +       static struct file_operations vmfile_fops;
> >         struct ashmem_area *asma = file->private_data;
> >         int ret = 0;
> >
> > @@ -393,6 +408,19 @@ static int ashmem_mmap(struct file *file, struct vm_area_struct *vma)
> >                 }
> >                 vmfile->f_mode |= FMODE_LSEEK;
> >                 asma->file = vmfile;
> > +               /*
> > +                * override mmap operation of the vmfile so that it can't be
> > +                * remapped which would lead to creation of a new vma with no
> > +                * asma permission checks. Have to override get_unmapped_area
> > +                * as well to prevent VM_BUG_ON check for f_ops modification.
> > +                */
> > +               if (!vmfile_fops.mmap) {
> > +                       vmfile_fops = *vmfile->f_op;
> > +                       vmfile_fops.mmap = ashmem_vmfile_mmap;
> > +                       vmfile_fops.get_unmapped_area =
> > +                                       ashmem_vmfile_get_unmapped_area;
> > +               }
> > +               vmfile->f_op = &vmfile_fops;
> >         }
> >         get_file(asma->file);
> >
> > --
> > 2.25.0.341.g760bfbb309-goog
> >
> > --
> > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
> >

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] staging: android: ashmem: Disallow ashmem memory from being remapped
  2020-01-27 23:56   ` Todd Kjos
@ 2020-01-28  0:05     ` Suren Baghdasaryan
  0 siblings, 0 replies; 4+ messages in thread
From: Suren Baghdasaryan @ 2020-01-28  0:05 UTC (permalink / raw)
  To: Todd Kjos
  Cc: Joel Fernandes, Greg Kroah-Hartman, Arve Hjonnevag,
	open list:ANDROID DRIVERS, LKML, Martijn Coenen,
	Joel Fernandes (Google),
	Cc: Android Kernel, Jann Horn, stable

On Mon, Jan 27, 2020 at 3:57 PM 'Todd Kjos' via kernel-team
<kernel-team@android.com> wrote:
>
> On Mon, Jan 27, 2020 at 2:30 PM Joel Fernandes <joelaf@google.com> wrote:
> >
> > On Mon, Jan 27, 2020 at 1:00 PM 'Todd Kjos' via kernel-team
> > <kernel-team@android.com> wrote:
> > >
> > > From: Suren Baghdasaryan <surenb@google.com>
> > >
> > > When ashmem file is being mmapped the resulting vma->vm_file points to the
> > > backing shmem file with the generic fops that do not check ashmem
> > > permissions like fops of ashmem do. Fix that by disallowing mapping
> > > operation for backing shmem file.
> >
> > Looks good, but I think the commit message is confusing. I had to read
> > the code a couple times to understand what's going on since there are
> > no links to a PoC for the security issue, in the commit message. I
> > think a better message could have been:
> >
> >  When ashmem file is mmapped, the resulting vma->vm_file points to the
> >  backing shmem file with the generic fops that do not check ashmem
> >  permissions like fops of ashmem do. If an mremap is done on the ashmem
> >  region, then the permission checks will be skipped. Fix that by disallowing
> >  mapping operation on the backing shmem file.
>
> Sent v2 with the suggested change.

Sorry for the delay. The suggestion makes sense to me. Thanks!

>
> >
> > Or did I miss something?
> >
> > thanks!
> >
> > - Joel
> >
> >
> >
> > >
> > > Reported-by: Jann Horn <jannh@google.com>
> > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > Cc: stable <stable@vger.kernel.org> # 4.4,4.9,4.14,4.18,5.4
> > > Signed-off-by: Todd Kjos <tkjos@google.com>
> > > ---
> > >  drivers/staging/android/ashmem.c | 28 ++++++++++++++++++++++++++++
> > >  1 file changed, 28 insertions(+)
> > >
> > > diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
> > > index 74d497d39c5a..c6695354b123 100644
> > > --- a/drivers/staging/android/ashmem.c
> > > +++ b/drivers/staging/android/ashmem.c
> > > @@ -351,8 +351,23 @@ static inline vm_flags_t calc_vm_may_flags(unsigned long prot)
> > >                _calc_vm_trans(prot, PROT_EXEC,  VM_MAYEXEC);
> > >  }
> > >
> > > +static int ashmem_vmfile_mmap(struct file *file, struct vm_area_struct *vma)
> > > +{
> > > +       /* do not allow to mmap ashmem backing shmem file directly */
> > > +       return -EPERM;
> > > +}
> > > +
> > > +static unsigned long
> > > +ashmem_vmfile_get_unmapped_area(struct file *file, unsigned long addr,
> > > +                               unsigned long len, unsigned long pgoff,
> > > +                               unsigned long flags)
> > > +{
> > > +       return current->mm->get_unmapped_area(file, addr, len, pgoff, flags);
> > > +}
> > > +
> > >  static int ashmem_mmap(struct file *file, struct vm_area_struct *vma)
> > >  {
> > > +       static struct file_operations vmfile_fops;
> > >         struct ashmem_area *asma = file->private_data;
> > >         int ret = 0;
> > >
> > > @@ -393,6 +408,19 @@ static int ashmem_mmap(struct file *file, struct vm_area_struct *vma)
> > >                 }
> > >                 vmfile->f_mode |= FMODE_LSEEK;
> > >                 asma->file = vmfile;
> > > +               /*
> > > +                * override mmap operation of the vmfile so that it can't be
> > > +                * remapped which would lead to creation of a new vma with no
> > > +                * asma permission checks. Have to override get_unmapped_area
> > > +                * as well to prevent VM_BUG_ON check for f_ops modification.
> > > +                */
> > > +               if (!vmfile_fops.mmap) {
> > > +                       vmfile_fops = *vmfile->f_op;
> > > +                       vmfile_fops.mmap = ashmem_vmfile_mmap;
> > > +                       vmfile_fops.get_unmapped_area =
> > > +                                       ashmem_vmfile_get_unmapped_area;
> > > +               }
> > > +               vmfile->f_op = &vmfile_fops;
> > >         }
> > >         get_file(asma->file);
> > >
> > > --
> > > 2.25.0.341.g760bfbb309-goog
> > >
> > > --
> > > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
> > >
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-01-28  0:06 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-27 21:00 [PATCH] staging: android: ashmem: Disallow ashmem memory from being remapped Todd Kjos
2020-01-27 22:30 ` Joel Fernandes
2020-01-27 23:56   ` Todd Kjos
2020-01-28  0:05     ` Suren Baghdasaryan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).