[v2,1/1] brk02: Add test for removing more than one VMA

When brk expands, it attempts to expand a VMA.  This expansion will
succeed depending on the anonymous VMA chain and if the vma flags are
compatible.  This test expands brk() then calls mprotect to ensure the
next brk call will create a new VMA, then it calls brk a final time to
restore the first brk address.  The test is the final brk call which
will remove more than an entire VMA from the vm area.

Signed-off-by: Liam R. Howlett <Liam.Howlett@Oracle.com>
---
 testcases/kernel/syscalls/brk/brk02.c | 54 +++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 testcases/kernel/syscalls/brk/brk02.c

Hi Liam,

> When brk expands, it attempts to expand a VMA.  This expansion will
> succeed depending on the anonymous VMA chain and if the vma flags are
> compatible.  This test expands brk() then calls mprotect to ensure the
> next brk call will create a new VMA, then it calls brk a final time to
> restore the first brk address.  The test is the final brk call which
> will remove more than an entire VMA from the vm area.

> Signed-off-by: Liam R. Howlett <Liam.Howlett@Oracle.com>
> ---
>  testcases/kernel/syscalls/brk/brk02.c | 54 +++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
>  create mode 100644 testcases/kernel/syscalls/brk/brk02.c

> diff --git a/testcases/kernel/syscalls/brk/brk02.c b/testcases/kernel/syscalls/brk/brk02.c
> new file mode 100644
> index 000000000..ef84fb440
> --- /dev/null
> +++ b/testcases/kernel/syscalls/brk/brk02.c
> @@ -0,0 +1,54 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2021 Liam R. Howlett <liam.howlett@oracle.com>
> + *
> + *  Expand the brk by at least 2 pages to ensure there is a newly created VMA
> + *  and not expanding the original due to multiple anon pages.  mprotect that
> + *  new VMA then brk back to the original address therefore causing a munmap of
> + *  at least one full VMA.
I'll put this as a docparse formatting (will show in our documentation):

/*\
 * [DESCRIPTION]
 * Expand brk() by at least 2 pages to ensure there is a newly created VMA
 * and not expanding the original due to multiple anon pages.  mprotect() that
 * new VMA then brk() back to the original address therefore causing a munmap of
 * at least one full VMA.
\*/

> + */
> +
> +#include <unistd.h>
> +#include <sys/mman.h>
> +#include "tst_test.h"
> +
> +void brk_down_vmas(void)
> +{
> +	void *brk_addr = sbrk(0);

Shouldn't there be a check?

	if (brk_addr == (void *) -1)
		tst_brk(TBROK, "sbrk() failed");

> +	unsigned long page_size = getpagesize();
> +	void *addr = brk_addr + page_size;
> +
> +	if (brk(addr)) {
> +		tst_res(TFAIL, "Cannot expand brk by page size.");
nit: remove dot at the end.
> +		return;
> +	}
> +
> +	addr += page_size;
> +	if (brk(addr)) {
> +		tst_res(TFAIL, "Cannot expand brk by 2x page size.");
> +		return;
> +	}
> +
> +	if (mprotect(addr - page_size, page_size,
> +		     PROT_READ|PROT_WRITE|PROT_EXEC)) {
> +		tst_res(TFAIL, "Cannot mprotect new VMA.");
> +		return;
> +	}
> +
> +	addr += page_size;
> +	if (brk(addr)) {
> +		tst_res(TFAIL, "Cannot expand brk after mprotect.");
> +		return;
> +	}
> +
> +	if (brk(brk_addr)) {
> +		tst_res(TFAIL, "Cannot restore brk to start address.");
> +		return;
> +	}
> +
> +	tst_res(TPASS, "munmap at least two VMAs of brk() passed.");
> +}
> +
> +static struct tst_test test = {
> +	.test_all = brk_down_vmas,
> +};

No need to repost if you agree with these changes below.

Kind regards,
Petr

diff --git testcases/kernel/syscalls/brk/brk02.c testcases/kernel/syscalls/brk/brk02.c
index ef84fb440..2e604eb5d 100644
--- testcases/kernel/syscalls/brk/brk02.c
+++ testcases/kernel/syscalls/brk/brk02.c
@@ -1,13 +1,16 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 /*
  * Copyright (c) 2021 Liam R. Howlett <liam.howlett@oracle.com>
- *
- *  Expand the brk by at least 2 pages to ensure there is a newly created VMA
- *  and not expanding the original due to multiple anon pages.  mprotect that
- *  new VMA then brk back to the original address therefore causing a munmap of
- *  at least one full VMA.
  */
 
+/*\
+ * [DESCRIPTION]
+ * Expand brk() by at least 2 pages to ensure there is a newly created VMA
+ * and not expanding the original due to multiple anon pages.  mprotect() that
+ * new VMA then brk() back to the original address therefore causing a munmap of
+ * at least one full VMA.
+\*/
+
 #include <unistd.h>
 #include <sys/mman.h>
 #include "tst_test.h"
@@ -15,38 +18,42 @@
 void brk_down_vmas(void)
 {
 	void *brk_addr = sbrk(0);
+
+	if (brk_addr == (void *) -1)
+		tst_brk(TBROK, "sbrk() failed");
+
 	unsigned long page_size = getpagesize();
 	void *addr = brk_addr + page_size;
 
 	if (brk(addr)) {
-		tst_res(TFAIL, "Cannot expand brk by page size.");
+		tst_res(TFAIL, "Cannot expand brk() by page size");
 		return;
 	}
 
 	addr += page_size;
 	if (brk(addr)) {
-		tst_res(TFAIL, "Cannot expand brk by 2x page size.");
+		tst_res(TFAIL, "Cannot expand brk() by 2x page size");
 		return;
 	}
 
 	if (mprotect(addr - page_size, page_size,
 		     PROT_READ|PROT_WRITE|PROT_EXEC)) {
-		tst_res(TFAIL, "Cannot mprotect new VMA.");
+		tst_res(TFAIL, "Cannot mprotect new VMA");
 		return;
 	}
 
 	addr += page_size;
 	if (brk(addr)) {
-		tst_res(TFAIL, "Cannot expand brk after mprotect.");
+		tst_res(TFAIL, "Cannot expand brk() after mprotect");
 		return;
 	}
 
 	if (brk(brk_addr)) {
-		tst_res(TFAIL, "Cannot restore brk to start address.");
+		tst_res(TFAIL, "Cannot restore brk() to start address");
 		return;
 	}
 
-	tst_res(TPASS, "munmap at least two VMAs of brk() passed.");
+	tst_res(TPASS, "munmap at least two VMAs of brk() passed");
 }
 
 static struct tst_test test = {

Hello Petr,

* Petr Vorel <pvorel@suse.cz> [210225 14:02]:
> Hi Liam,
> 
> > When brk expands, it attempts to expand a VMA.  This expansion will
> > succeed depending on the anonymous VMA chain and if the vma flags are
> > compatible.  This test expands brk() then calls mprotect to ensure the
> > next brk call will create a new VMA, then it calls brk a final time to
> > restore the first brk address.  The test is the final brk call which
> > will remove more than an entire VMA from the vm area.
> 
> > Signed-off-by: Liam R. Howlett <Liam.Howlett@Oracle.com>
> > ---
> >  testcases/kernel/syscalls/brk/brk02.c | 54 +++++++++++++++++++++++++++
> >  1 file changed, 54 insertions(+)
> >  create mode 100644 testcases/kernel/syscalls/brk/brk02.c
> 
> > diff --git a/testcases/kernel/syscalls/brk/brk02.c b/testcases/kernel/syscalls/brk/brk02.c
> > new file mode 100644
> > index 000000000..ef84fb440
> > --- /dev/null
> > +++ b/testcases/kernel/syscalls/brk/brk02.c
> > @@ -0,0 +1,54 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2021 Liam R. Howlett <liam.howlett@oracle.com>
> > + *
> > + *  Expand the brk by at least 2 pages to ensure there is a newly created VMA
> > + *  and not expanding the original due to multiple anon pages.  mprotect that
> > + *  new VMA then brk back to the original address therefore causing a munmap of
> > + *  at least one full VMA.
> I'll put this as a docparse formatting (will show in our documentation):

Thanks

> 
> /*\
>  * [DESCRIPTION]
>  * Expand brk() by at least 2 pages to ensure there is a newly created VMA
>  * and not expanding the original due to multiple anon pages.  mprotect() that
>  * new VMA then brk() back to the original address therefore causing a munmap of
>  * at least one full VMA.
> \*/
> 
> > + */
> > +
> > +#include <unistd.h>
> > +#include <sys/mman.h>
> > +#include "tst_test.h"
> > +
> > +void brk_down_vmas(void)
> > +{
> > +	void *brk_addr = sbrk(0);
> 
> Shouldn't there be a check?
> 
> 	if (brk_addr == (void *) -1)
> 		tst_brk(TBROK, "sbrk() failed");

Yes, you are correct.  And -1 is the correct thing to check.

> 
> > +	unsigned long page_size = getpagesize();
> > +	void *addr = brk_addr + page_size;
> > +
> > +	if (brk(addr)) {
> > +		tst_res(TFAIL, "Cannot expand brk by page size.");
> nit: remove dot at the end.

Sounds good

> > +		return;
> > +	}
> > +
> > +	addr += page_size;
> > +	if (brk(addr)) {
> > +		tst_res(TFAIL, "Cannot expand brk by 2x page size.");
> > +		return;
> > +	}
> > +
> > +	if (mprotect(addr - page_size, page_size,
> > +		     PROT_READ|PROT_WRITE|PROT_EXEC)) {
> > +		tst_res(TFAIL, "Cannot mprotect new VMA.");
> > +		return;
> > +	}
> > +
> > +	addr += page_size;
> > +	if (brk(addr)) {
> > +		tst_res(TFAIL, "Cannot expand brk after mprotect.");
> > +		return;
> > +	}
> > +
> > +	if (brk(brk_addr)) {
> > +		tst_res(TFAIL, "Cannot restore brk to start address.");
> > +		return;
> > +	}
> > +
> > +	tst_res(TPASS, "munmap at least two VMAs of brk() passed.");
> > +}
> > +
> > +static struct tst_test test = {
> > +	.test_all = brk_down_vmas,
> > +};
> 
> No need to repost if you agree with these changes below.
> 
> Kind regards,
> Petr

The changes below look good.

Thanks,
Liam

> 
> diff --git testcases/kernel/syscalls/brk/brk02.c testcases/kernel/syscalls/brk/brk02.c
> index ef84fb440..2e604eb5d 100644
> --- testcases/kernel/syscalls/brk/brk02.c
> +++ testcases/kernel/syscalls/brk/brk02.c
> @@ -1,13 +1,16 @@
>  // SPDX-License-Identifier: GPL-2.0-or-later
>  /*
>   * Copyright (c) 2021 Liam R. Howlett <liam.howlett@oracle.com>
> - *
> - *  Expand the brk by at least 2 pages to ensure there is a newly created VMA
> - *  and not expanding the original due to multiple anon pages.  mprotect that
> - *  new VMA then brk back to the original address therefore causing a munmap of
> - *  at least one full VMA.
>   */
>  
> +/*\
> + * [DESCRIPTION]
> + * Expand brk() by at least 2 pages to ensure there is a newly created VMA
> + * and not expanding the original due to multiple anon pages.  mprotect() that
> + * new VMA then brk() back to the original address therefore causing a munmap of
> + * at least one full VMA.
> +\*/
> +
>  #include <unistd.h>
>  #include <sys/mman.h>
>  #include "tst_test.h"
> @@ -15,38 +18,42 @@
>  void brk_down_vmas(void)
>  {
>  	void *brk_addr = sbrk(0);
> +
> +	if (brk_addr == (void *) -1)
> +		tst_brk(TBROK, "sbrk() failed");
> +
>  	unsigned long page_size = getpagesize();
>  	void *addr = brk_addr + page_size;
>  
>  	if (brk(addr)) {
> -		tst_res(TFAIL, "Cannot expand brk by page size.");
> +		tst_res(TFAIL, "Cannot expand brk() by page size");
>  		return;
>  	}
>  
>  	addr += page_size;
>  	if (brk(addr)) {
> -		tst_res(TFAIL, "Cannot expand brk by 2x page size.");
> +		tst_res(TFAIL, "Cannot expand brk() by 2x page size");
>  		return;
>  	}
>  
>  	if (mprotect(addr - page_size, page_size,
>  		     PROT_READ|PROT_WRITE|PROT_EXEC)) {
> -		tst_res(TFAIL, "Cannot mprotect new VMA.");
> +		tst_res(TFAIL, "Cannot mprotect new VMA");
>  		return;
>  	}
>  
>  	addr += page_size;
>  	if (brk(addr)) {
> -		tst_res(TFAIL, "Cannot expand brk after mprotect.");
> +		tst_res(TFAIL, "Cannot expand brk() after mprotect");
>  		return;
>  	}
>  
>  	if (brk(brk_addr)) {
> -		tst_res(TFAIL, "Cannot restore brk to start address.");
> +		tst_res(TFAIL, "Cannot restore brk() to start address");
>  		return;
>  	}
>  
> -	tst_res(TPASS, "munmap at least two VMAs of brk() passed.");
> +	tst_res(TPASS, "munmap at least two VMAs of brk() passed");
>  }
>  
>  static struct tst_test test = {

Hi Liam,

> The changes below look good.
Good, patchset merged.
Thanks for your work.

Kind regards,
Petr

Hi Liam, Petr,

Liam Howlett <liam.howlett@oracle.com> wrote:


> ...
> +       if (mprotect(addr - page_size, page_size,
> +                    PROT_READ|PROT_WRITE|PROT_EXEC)) {
> +               tst_res(TFAIL, "Cannot mprotect new VMA.");
> +               return;
> +       }
>

We got permission denied here while performing the brk02 on
x86_64/s390x(kernel-4.18~). After looking at the manual page of
mprotect(), seems the access issue caused by PROT_EXEC.

"
POSIX says that the behavior of mprotect() is unspecified if it is applied
to a region of memory that was not obtained via mmap(2).
...
Whether  PROT_EXEC has any effect different from PROT_READ
depends on processor architecture, kernel version, and process state.
If READ_IMPLIES_EXEC is set in the process's personality flags
(see personality(2)), specifying PROT_READ will implicitly add PROT_EXEC.
"

# ./brk02
tst_test.c:1291: TINFO: Timeout per run is 0h 05m 00s
brk02.c:41: TFAIL: Cannot mprotect new VMA

After removing the PROT_EXEC:

# ./brk02
tst_test.c:1291: TINFO: Timeout per run is 0h 05m 00s
brk02.c:56: TPASS: munmap at least two VMAs of brk() passed

Hello Li,

Thank you for looking at this testcase.

* Li Wang <liwang@redhat.com> [210317 07:08]:
> Hi Liam, Petr,
> 
> Liam Howlett <liam.howlett@oracle.com> wrote:
> 
> 
> > ...
> > +       if (mprotect(addr - page_size, page_size,
> > +                    PROT_READ|PROT_WRITE|PROT_EXEC)) {
> > +               tst_res(TFAIL, "Cannot mprotect new VMA.");
> > +               return;
> > +       }
> >
> 
> We got permission denied here while performing the brk02 on
> x86_64/s390x(kernel-4.18~). After looking at the manual page of
> mprotect(), seems the access issue caused by PROT_EXEC.
> 
> "
> POSIX says that the behavior of mprotect() is unspecified if it is applied
> to a region of memory that was not obtained via mmap(2).
> ...
> Whether  PROT_EXEC has any effect different from PROT_READ
> depends on processor architecture, kernel version, and process state.
> If READ_IMPLIES_EXEC is set in the process's personality flags
> (see personality(2)), specifying PROT_READ will implicitly add PROT_EXEC.
> "


Unforntunately, dropping the PROT_EXEC causes the VMA behaviour to
change and does not test what this testcase is intended to test.

Removing the PROT_EXEC and the PROT_WRITE does test what is supposed to
be tested.  Can you verify that this works on the s390x?

Are you using real hardware to test this or can I use some sort of
emulation to test on my side?

Thank you,
Liam

Hi Liam,

On Wed, Mar 24, 2021 at 12:27 AM Liam Howlett <liam.howlett@oracle.com>
wrote:

> Hello Li,
>
> Thank you for looking at this testcase.
>
> * Li Wang <liwang@redhat.com> [210317 07:08]:
> > Hi Liam, Petr,
> >
> > Liam Howlett <liam.howlett@oracle.com> wrote:
> >
> >
> > > ...
> > > +       if (mprotect(addr - page_size, page_size,
> > > +                    PROT_READ|PROT_WRITE|PROT_EXEC)) {
> > > +               tst_res(TFAIL, "Cannot mprotect new VMA.");
> > > +               return;
> > > +       }
> > >
> >
> > We got permission denied here while performing the brk02 on
> > x86_64/s390x(kernel-4.18~). After looking at the manual page of
> > mprotect(), seems the access issue caused by PROT_EXEC.
> >
> > "
> > POSIX says that the behavior of mprotect() is unspecified if it is
> applied
> > to a region of memory that was not obtained via mmap(2).
> > ...
> > Whether  PROT_EXEC has any effect different from PROT_READ
> > depends on processor architecture, kernel version, and process state.
> > If READ_IMPLIES_EXEC is set in the process's personality flags
> > (see personality(2)), specifying PROT_READ will implicitly add PROT_EXEC.
> > "
>
>
> Unforntunately, dropping the PROT_EXEC causes the VMA behaviour to
> change and does not test what this testcase is intended to test.
>

Yes, I agree with this. But am not sure if Linux take some action on
security
side to prevent setting PROT_EXEC permission arbitrary.


>
> Removing the PROT_EXEC and the PROT_WRITE does test what is supposed to
> be tested.  Can you verify that this works on the s390x?
>

Actually just removing the PROT_EXEC then the brk02 can be passed on all my
platforms.


>
> Are you using real hardware to test this or can I use some sort of
> emulation to test on my side?
>

It looks like easily to reproduce.

I get failed with both virtual system and bare metal, e.g. the first one
is on my Fedora33-workstation. But the worth to say, brk02 passed
with raspberry pi3 and pi4.

x86_64
-------------
# virt-what
# echo $?
0
# uname -r
5.10.22-200.fc33.x86_64
# ./brk02
tst_test.c:1289: TINFO: Timeout per run is 0h 05m 00s
brk02.c:41: TFAIL: Cannot mprotect new VMA

x86_64
-------------
# virt-what
kvm
# ./brk02
tst_test.c:1291: TINFO: Timeout per run is 0h 05m 00s
brk02.c:41: TFAIL: Cannot mprotect new VMA

s390x
-------------
# uname -r
5.12.0-rc4.vdso+
# virt-what
ibm_systemz
ibm_systemz-zvm
# ./brk02
tst_test.c:1289: TINFO: Timeout per run is 0h 05m 00s
brk02.c:41: TFAIL: Cannot mprotect new VMA


armv7l -- raspberry-pi3
-----------------------------
# uname  -r
5.4.96-v7.1.el7
# ./brk02
tst_test.c:1291: TINFO: Timeout per run is 0h 05m 00s
brk02.c:56: TPASS: munmap at least two VMAs of brk() passed
armv7l -- raspberry-pi4
-----------------------------
# uname  -rm
5.10.17-v7l+ armv7l
# ./brk02
tst_test.c:1291: TINFO: Timeout per run is 0h 05m 00s
brk02.c:56: TPASS: munmap at least two VMAs of brk() passed

* Li Wang <liwang@redhat.com> [210325 04:37]:
> Hi Liam,
> 
> On Wed, Mar 24, 2021 at 12:27 AM Liam Howlett <liam.howlett@oracle.com>
> wrote:
> 
> > Hello Li,
> >
> > Thank you for looking at this testcase.
> >
> > * Li Wang <liwang@redhat.com> [210317 07:08]:
> > > Hi Liam, Petr,
> > >
> > > Liam Howlett <liam.howlett@oracle.com> wrote:
> > >
> > >
> > > > ...
> > > > +       if (mprotect(addr - page_size, page_size,
> > > > +                    PROT_READ|PROT_WRITE|PROT_EXEC)) {
> > > > +               tst_res(TFAIL, "Cannot mprotect new VMA.");
> > > > +               return;
> > > > +       }
> > > >
> > >
> > > We got permission denied here while performing the brk02 on
> > > x86_64/s390x(kernel-4.18~). After looking at the manual page of
> > > mprotect(), seems the access issue caused by PROT_EXEC.
> > >
> > > "
> > > POSIX says that the behavior of mprotect() is unspecified if it is
> > applied
> > > to a region of memory that was not obtained via mmap(2).
> > > ...
> > > Whether  PROT_EXEC has any effect different from PROT_READ
> > > depends on processor architecture, kernel version, and process state.
> > > If READ_IMPLIES_EXEC is set in the process's personality flags
> > > (see personality(2)), specifying PROT_READ will implicitly add PROT_EXEC.
> > > "
> >
> >
> > Unforntunately, dropping the PROT_EXEC causes the VMA behaviour to
> > change and does not test what this testcase is intended to test.
> >
> 
> Yes, I agree with this. But am not sure if Linux take some action on
> security
> side to prevent setting PROT_EXEC permission arbitrary.
> 
> 
> >
> > Removing the PROT_EXEC and the PROT_WRITE does test what is supposed to
> > be tested.  Can you verify that this works on the s390x?
> >
> 
> Actually just removing the PROT_EXEC then the brk02 can be passed on all my
> platforms.

Just removing the PROT_EXEC invalidates the test.  However, if both
PROT_EXEC and PROT_WRITE are removed, then the test still does what is
intended.

> 
> 
> >
> > Are you using real hardware to test this or can I use some sort of
> > emulation to test on my side?
> >
> 
> It looks like easily to reproduce.
> 
> I get failed with both virtual system and bare metal, e.g. the first one
> is on my Fedora33-workstation. But the worth to say, brk02 passed
> with raspberry pi3 and pi4.
> 
> x86_64
> -------------
> # virt-what
> # echo $?
> 0
> # uname -r
> 5.10.22-200.fc33.x86_64
> # ./brk02
> tst_test.c:1289: TINFO: Timeout per run is 0h 05m 00s
> brk02.c:41: TFAIL: Cannot mprotect new VMA


This was my test platform.  I also sent it to the Travis CI which passed
for x86_64.  I will re-examine the accepted code to ensure the cosmetic
changes didn't invalidate my testing.


> 
> x86_64
> -------------
> # virt-what
> kvm
> # ./brk02
> tst_test.c:1291: TINFO: Timeout per run is 0h 05m 00s
> brk02.c:41: TFAIL: Cannot mprotect new VMA
> 
> s390x
> -------------
> # uname -r
> 5.12.0-rc4.vdso+
> # virt-what
> ibm_systemz
> ibm_systemz-zvm
> # ./brk02
> tst_test.c:1289: TINFO: Timeout per run is 0h 05m 00s
> brk02.c:41: TFAIL: Cannot mprotect new VMA
> 
> 
> armv7l -- raspberry-pi3
> -----------------------------
> # uname  -r
> 5.4.96-v7.1.el7
> # ./brk02
> tst_test.c:1291: TINFO: Timeout per run is 0h 05m 00s
> brk02.c:56: TPASS: munmap at least two VMAs of brk() passed
> armv7l -- raspberry-pi4
> -----------------------------
> # uname  -rm
> 5.10.17-v7l+ armv7l
> # ./brk02
> tst_test.c:1291: TINFO: Timeout per run is 0h 05m 00s
> brk02.c:56: TPASS: munmap at least two VMAs of brk() passed
> 

Would you be willing to re-run the tests without both PROT_EXEC and
PROT_WRITE?


Thank you,
Liam

Hi Liam,

On Thu, Mar 25, 2021 at 9:16 PM Liam Howlett <liam.howlett@oracle.com>
wrote:

> * Li Wang <liwang@redhat.com> [210325 04:37]:
> > Hi Liam,
> >
> > On Wed, Mar 24, 2021 at 12:27 AM Liam Howlett <liam.howlett@oracle.com>
> > wrote:
> >
> > > Hello Li,
> > >
> > > Thank you for looking at this testcase.
> > >
> > > * Li Wang <liwang@redhat.com> [210317 07:08]:
> > > > Hi Liam, Petr,
> > > >
> > > > Liam Howlett <liam.howlett@oracle.com> wrote:
> > > >
> > > >
> > > > > ...
> > > > > +       if (mprotect(addr - page_size, page_size,
> > > > > +                    PROT_READ|PROT_WRITE|PROT_EXEC)) {
> > > > > +               tst_res(TFAIL, "Cannot mprotect new VMA.");
> > > > > +               return;
> > > > > +       }
> > > > >
> > > >
> > > > We got permission denied here while performing the brk02 on
> > > > x86_64/s390x(kernel-4.18~). After looking at the manual page of
> > > > mprotect(), seems the access issue caused by PROT_EXEC.
> > > >
> > > > "
> > > > POSIX says that the behavior of mprotect() is unspecified if it is
> > > applied
> > > > to a region of memory that was not obtained via mmap(2).
> > > > ...
> > > > Whether  PROT_EXEC has any effect different from PROT_READ
> > > > depends on processor architecture, kernel version, and process state.
> > > > If READ_IMPLIES_EXEC is set in the process's personality flags
> > > > (see personality(2)), specifying PROT_READ will implicitly add
> PROT_EXEC.
> > > > "
> > >
> > >
> > > Unforntunately, dropping the PROT_EXEC causes the VMA behaviour to
> > > change and does not test what this testcase is intended to test.
> > >
> >
> > Yes, I agree with this. But am not sure if Linux take some action on
> > security
> > side to prevent setting PROT_EXEC permission arbitrary.
> >
> >
> > >
> > > Removing the PROT_EXEC and the PROT_WRITE does test what is supposed to
> > > be tested.  Can you verify that this works on the s390x?
> > >
> >
> > Actually just removing the PROT_EXEC then the brk02 can be passed on all
> my
> > platforms.
>
> Just removing the PROT_EXEC invalidates the test.  However, if both
> PROT_EXEC and PROT_WRITE are removed, then the test still does what is
> intended.


> >
> >
> > >
> > > Are you using real hardware to test this or can I use some sort of
> > > emulation to test on my side?
> > >
> >
> > It looks like easily to reproduce.
> >
> > I get failed with both virtual system and bare metal, e.g. the first one
> > is on my Fedora33-workstation. But the worth to say, brk02 passed
> > with raspberry pi3 and pi4.
> >
> > x86_64
> > -------------
> > # virt-what
> > # echo $?
> > 0
> > # uname -r
> > 5.10.22-200.fc33.x86_64
> > # ./brk02
> > tst_test.c:1289: TINFO: Timeout per run is 0h 05m 00s
> > brk02.c:41: TFAIL: Cannot mprotect new VMA
>
>
> This was my test platform.  I also sent it to the Travis CI which passed
> for x86_64.  I will re-examine the accepted code to ensure the cosmetic
> changes didn't invalidate my testing.
>

FWIK, the Travis CI does not run test case, it just compiles/builds LTP
across
many arches/platforms.



>
>
> >
> > x86_64
> > -------------
> > # virt-what
> > kvm
> > # ./brk02
> > tst_test.c:1291: TINFO: Timeout per run is 0h 05m 00s
> > brk02.c:41: TFAIL: Cannot mprotect new VMA
> >
> > s390x
> > -------------
> > # uname -r
> > 5.12.0-rc4.vdso+
> > # virt-what
> > ibm_systemz
> > ibm_systemz-zvm
> > # ./brk02
> > tst_test.c:1289: TINFO: Timeout per run is 0h 05m 00s
> > brk02.c:41: TFAIL: Cannot mprotect new VMA
> >
> >
> > armv7l -- raspberry-pi3
> > -----------------------------
> > # uname  -r
> > 5.4.96-v7.1.el7
> > # ./brk02
> > tst_test.c:1291: TINFO: Timeout per run is 0h 05m 00s
> > brk02.c:56: TPASS: munmap at least two VMAs of brk() passed
> > armv7l -- raspberry-pi4
> > -----------------------------
> > # uname  -rm
> > 5.10.17-v7l+ armv7l
> > # ./brk02
> > tst_test.c:1291: TINFO: Timeout per run is 0h 05m 00s
> > brk02.c:56: TPASS: munmap at least two VMAs of brk() passed
> >
>
> Would you be willing to re-run the tests without both PROT_EXEC and
> PROT_WRITE?
>

Yes, it will always PASS without 'PROT_EXEC|PROT_WRITE'.

Hi Liam, Li,

> > This was my test platform.  I also sent it to the Travis CI which passed
> > for x86_64.  I will re-examine the accepted code to ensure the cosmetic
> > changes didn't invalidate my testing.

> FWIK, the Travis CI does not run test case, it just compiles/builds LTP
> across
> many arches/platforms.

Yes, while KernelCI and some enterprise / embedded distros for their kernels run
LTP testcases, the project does not run it. Cyril run some regression tests for
few most important runtests before release manually. But having a server it'd be
handy to run them.

Kind regards,
Petr

* Petr Vorel <pvorel@suse.cz> [210326 06:43]:
> Hi Liam, Li,
> 
> > > This was my test platform.  I also sent it to the Travis CI which passed
> > > for x86_64.  I will re-examine the accepted code to ensure the cosmetic
> > > changes didn't invalidate my testing.
> 
> > FWIK, the Travis CI does not run test case, it just compiles/builds LTP
> > across
> > many arches/platforms.
> 
> Yes, while KernelCI and some enterprise / embedded distros for their kernels run
> LTP testcases, the project does not run it. Cyril run some regression tests for
> few most important runtests before release manually. But having a server it'd be
> handy to run them.


Thank you for clarification.  What is the best way to re-test my
changes?  As I had said, I made a change that will not add EXEC but will
still test removal of more than one VMA.  We cannot just mprotect
PROT_READ|PROT_WRITE as this will allow VMA merging to occur.  My
solution is to change the anon vma to just PROT_READ.  It passes in my
x86_64 test but since that was the case for me regardless, I cannot say
that I have fixed the issue, but I have verified that the test still
does what I expect it to do.


Thanks,
Liam

Patch below.
-------------------------------------------

+++ b/testcases/kernel/syscalls/brk/brk02.c
@@ -36,8 +36,7 @@ void brk_down_vmas(void)
                return;
        }
 
-       if (mprotect(addr - page_size, page_size,
-                    PROT_READ|PROT_WRITE|PROT_EXEC)) {
+       if (mprotect(addr - page_size, page_size, PROT_READ)) {
                tst_res(TFAIL, "Cannot mprotect new VMA");
                return;
        }

Hi Laim and all,

On Sat, Mar 27, 2021 at 12:14 AM Liam Howlett <liam.howlett@oracle.com>
wrote:

> * Petr Vorel <pvorel@suse.cz> [210326 06:43]:
> > Hi Liam, Li,
> >
> > > > This was my test platform.  I also sent it to the Travis CI which
> passed
> > > > for x86_64.  I will re-examine the accepted code to ensure the
> cosmetic
> > > > changes didn't invalidate my testing.
> >
> > > FWIK, the Travis CI does not run test case, it just compiles/builds LTP
> > > across
> > > many arches/platforms.
> >
> > Yes, while KernelCI and some enterprise / embedded distros for their
> kernels run
> > LTP testcases, the project does not run it. Cyril run some regression
> tests for
> > few most important runtests before release manually. But having a server
> it'd be
> > handy to run them.
>
>
> Thank you for clarification.  What is the best way to re-test my
> changes?  As I had said, I made a change that will not add EXEC but will
>

No best way I guess, it would be great if you can do more arches
verification before patch sending, but then the maintainers would
help do that also.



> still test removal of more than one VMA.  We cannot just mprotect
> PROT_READ|PROT_WRITE as this will allow VMA merging to occur.  My
> solution is to change the anon vma to just PROT_READ.  It passes in my
> x86_64 test but since that was the case for me regardless, I cannot say
> that I have fixed the issue, but I have verified that the test still
> does what I expect it to do.
>

I'm fine with just test PROT_READ. And we can take this way to fix
the FAIL before the next LTP release.

But there is still a query in my mind, whether the FAIL I mentioned before
is a kernel bug or just caused by preventing process Self Modiefed Code,
that probably needs more investigation.



>
>
> Thanks,
> Liam
>
> Patch below.
> -------------------------------------------
>
> +++ b/testcases/kernel/syscalls/brk/brk02.c
> @@ -36,8 +36,7 @@ void brk_down_vmas(void)
>                 return;
>         }
>
> -       if (mprotect(addr - page_size, page_size,
> -                    PROT_READ|PROT_WRITE|PROT_EXEC)) {
> +       if (mprotect(addr - page_size, page_size, PROT_READ)) {
>                 tst_res(TFAIL, "Cannot mprotect new VMA");
>                 return;
>         }
>
>