Re: [PATCH] Prevent mmap command to map beyond EOF

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Oct 06, 2021 at 01:34:00PM +0200, Carlos Maiolino wrote:
> On Tue, Oct 05, 2021 at 03:36:53PM -0700, Darrick J. Wong wrote:
> > On Mon, Oct 04, 2021 at 04:11:40PM +0200, Carlos Maiolino wrote:
> > > Attempting to access a mmapp'ed region that does not correspond to the
> > > file results in a SIGBUS, so prevent xfs_io to even attempt to mmap() a
> > > region beyond EOF.
> > > 
> > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > > ---
> > > 
> > > There is a caveat about this patch though. It is possible to mmap() a
> > > non-existent file region, extent the file to go beyond such region, and run
> > > operations in this mmapped region without such operations triggering a SIGBUS
> > > (excluding the file corruption factor here :). So, I'm not quite sure if it
> > > would be ok to check for this in mmap_f() as this patch does, or create a helper
> > > to check for such condition, and use it on the other operations (mread_f,
> > > mwrite_f, etc). What you folks think?
> > 
> > What's the motivation for checking this in userspace?  Programs are
> > allowed to set up this (admittedly minimally functional) configuration,
> > or even set it up after the mmap by truncating the file.
> 
> My biggest motivation was actually seeing xfs_io crashing due a sigbus
> while running generic/172 and generic/173. And personally, I'd rather see an
> error message like "attempt to mmap/mwrite beyond EOF" than seeing it crash.
> Also, as you mentioned, programs are allowed to set up such kind of
> configuration (IIUC what you mean, mixing mmap, extend, truncate, etc), so, I
> believe such userspace programs should also ensure they are not attempting to
> write to invalid memory.

This patch would /also/ prevent us from writing an fstest to check that
a process /does/ get SIGBUS when writing to a mapping beyond EOF.  Huh,
we don't have a test for that...

Also, where does generic/173 write to a mapping beyond EOF?  It sets up
a file of blksz*nr_blks bytes, clones it, fills the fs to full, and then
writes that number of bytes to the mmap region to trigger SIGBUS when
the COW fails due to ENOSPC.

--D

> > OTOH if your goal is to write a test to check the SIGBUS functionality,
> > you could install a sigbus handler to report the signal to stderr, which
> > would avoid bash writing junk about the sigbus to the terminal.
> 
> No, I'm just trying to avoid xfs_io crashing if we point it to invalid memory :)
> 
> Cheers.
> 
> > 
> > --D
> > 
> > > 
> > >  io/mmap.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/io/mmap.c b/io/mmap.c
> > > index 9816cf68..77c5f2b6 100644
> > > --- a/io/mmap.c
> > > +++ b/io/mmap.c
> > > @@ -242,6 +242,13 @@ mmap_f(
> > >  		return 0;
> > >  	}
> > >  
> > > +	/* Check if we are mmapping beyond EOF */
> > > +	if ((offset + length) > filesize()) {
> > > +		printf(_("Attempting to mmap() beyond EOF\n"));
> > > +		exitcode = 1;
> > > +		return 0;
> > > +	}
> > > +
> > >  	/*
> > >  	 * mmap and munmap memory area of length2 region is helpful to
> > >  	 * make a region of extendible free memory. It's generally used
> > > -- 
> > > 2.31.1
> > > 
> > 
> 
> -- 
> Carlos
>