* [PATCH] Prevent mmap command to map beyond EOF
@ 2021-10-04 14:11 Carlos Maiolino
2021-10-05 22:36 ` Darrick J. Wong
0 siblings, 1 reply; 6+ messages in thread
From: Carlos Maiolino @ 2021-10-04 14:11 UTC (permalink / raw)
To: linux-xfs
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?
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Prevent mmap command to map beyond EOF
2021-10-04 14:11 [PATCH] Prevent mmap command to map beyond EOF Carlos Maiolino
@ 2021-10-05 22:36 ` Darrick J. Wong
2021-10-06 11:34 ` Carlos Maiolino
0 siblings, 1 reply; 6+ messages in thread
From: Darrick J. Wong @ 2021-10-05 22:36 UTC (permalink / raw)
To: Carlos Maiolino; +Cc: linux-xfs
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.
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.
--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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Prevent mmap command to map beyond EOF
2021-10-05 22:36 ` Darrick J. Wong
@ 2021-10-06 11:34 ` Carlos Maiolino
2021-10-06 15:54 ` Darrick J. Wong
0 siblings, 1 reply; 6+ messages in thread
From: Carlos Maiolino @ 2021-10-06 11:34 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
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.
>
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Prevent mmap command to map beyond EOF
2021-10-06 11:34 ` Carlos Maiolino
@ 2021-10-06 15:54 ` Darrick J. Wong
2021-10-06 16:54 ` Carlos Maiolino
0 siblings, 1 reply; 6+ messages in thread
From: Darrick J. Wong @ 2021-10-06 15:54 UTC (permalink / raw)
To: linux-xfs
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Prevent mmap command to map beyond EOF
2021-10-06 15:54 ` Darrick J. Wong
@ 2021-10-06 16:54 ` Carlos Maiolino
2021-10-07 10:45 ` Carlos Maiolino
0 siblings, 1 reply; 6+ messages in thread
From: Carlos Maiolino @ 2021-10-06 16:54 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
> > 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...
TBH, this kind of test in xfstests didn't pass through my mind, but, this is
mmap (and its mapped regions) behavior, I don't see why we would want to test
this in xfstests, but, well, I am neither a memory expert to say what it
should/shouldn't do (I'm just following the man page), nor I have any authority
to say what we can/can't do in xfstests :)
>
> 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.
The whole command that ends up receiving a SIGBUS is:
xfs_io -i -f -c 'mmap -rw 0 41943040' -c 'mwrite -S 0x62 0 41943040'
I have a slightly suspicious this is happening because it's trying to access the
very last byte of the file, but I do need to check again the machine where I was
running g/173. At a later point, I just did some tests using an empty, 0
sized file. So I do not recall from the top of my head the behavior from g/173.
I can do it later, now I need to run to a doc appt.
>
> --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
> >
>
--
Carlos
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Prevent mmap command to map beyond EOF
2021-10-06 16:54 ` Carlos Maiolino
@ 2021-10-07 10:45 ` Carlos Maiolino
0 siblings, 0 replies; 6+ messages in thread
From: Carlos Maiolino @ 2021-10-07 10:45 UTC (permalink / raw)
To: Darrick J. Wong, linux-xfs
On Wed, Oct 06, 2021 at 06:54:07PM +0200, Carlos Maiolino wrote:
> > > 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...
After looking closer into g/173, I see what you mean now.
>
> The whole command that ends up receiving a SIGBUS is:
>
> xfs_io -i -f -c 'mmap -rw 0 41943040' -c 'mwrite -S 0x62 0 41943040'
And...
> At a later point, I just did some tests using an empty, 0
> sized file.
And playing around with a 0 sized file without coming back to look closer into
g/173 was what had been blinding me.
Please just disregard this patch and my apologies for the noise.
--
Carlos
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-10-07 10:45 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-04 14:11 [PATCH] Prevent mmap command to map beyond EOF Carlos Maiolino
2021-10-05 22:36 ` Darrick J. Wong
2021-10-06 11:34 ` Carlos Maiolino
2021-10-06 15:54 ` Darrick J. Wong
2021-10-06 16:54 ` Carlos Maiolino
2021-10-07 10:45 ` Carlos Maiolino
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.