Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 27 Jan 2020 19:25:26 -0500
From:   Joel Fernandes <joel@joelfernandes.org>
To:     Todd Kjos <tkjos@google.com>
Cc:     surenb@google.com, gregkh@linuxfoundation.org, arve@android.com,
        devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org,
        maco@google.com, kernel-team@android.com,
        Jann Horn <jannh@google.com>, stable <stable@vger.kernel.org>
Subject: Re: [PATCH v2] staging: android: ashmem: Disallow ashmem memory from
 being remapped
Message-ID: <20200128002526.GC175575@google.com>
References: <20200127235616.48920-1-tkjos@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200127235616.48920-1-tkjos@google.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Jan 27, 2020 at 03:56:16PM -0800, Todd Kjos wrote:
> From: Suren Baghdasaryan <surenb@google.com>
> 
> 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.

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

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(+)
> 
> v2: update commit message as suggested by joelaf@google.com.
> 
> 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
>