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

[Joel Fernandes <joel@joelfernandes.org>]

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