lib: allow checkpoints to be used by any uid

PATCH New Jan Stancek 2 participants 5 messages Oct 6, 2017 – Oct 10, 2017 Library code

Share & tools

Jan Stancek

author

2017-10-06 11:57 UTC #0 lore raw

PATCH v2 1/2 lib: allow checkpoints to be used by any uid

Allow unprivileged child to synchronize with privileged parent. Use chmod after open, because effective permissions set by open() are modified by the process's umask: (mode & ~umask).

Signed-off-by Jan Stancek <[email protected]>

---
 lib/tst_test.c | 1 +
 1 file changed, 1 insertion(+)

No changes in v2.

lib/tst_test.c +1 -0

index 26414e31ca77..233b370794a6 100644
@@ -93,6 +93,7 @@ static void setup_ipc(void)
 	ipc_fd = open(shm_path, O_CREAT | O_EXCL | O_RDWR, 0600);
 	if (ipc_fd < 0)
 		tst_brk(TBROK | TERRNO, "open(%s)", shm_path);
+	SAFE_CHMOD(shm_path, 0666);
 
 	SAFE_FTRUNCATE(ipc_fd, size);

Signature · 1 line

1.8.3.1

Jan Stancek

author

2017-10-06 11:57 UTC #1 lore raw

PATCH v2 2/2 security/dirtyc0w: synchronize parent and child

Add checkpoint to guarantee that parent doesn't send signal to child before it sets up signal handler.

Signed-off-by Jan Stancek <[email protected]>

---
 testcases/kernel/security/dirtyc0w/dirtyc0w.c       | 9 ++++++++-
 testcases/kernel/security/dirtyc0w/dirtyc0w_child.c | 3 +++
 2 files changed, 11 insertions(+), 1 deletion(-)

changes in v2:
don't copy child executable, use execvpe instead.

testcases/kernel/security/dirtyc0w/dirtyc0w.c +8 -1

index b8094ba977ab..b145838ecac7 100644
@@ -36,6 +36,8 @@
  *   mm: remove gup_flags FOLL_WRITE games from __get_user_pages()
  */
 
+#define _GNU_SOURCE
+#include <errno.h>
 #include <sys/mman.h>
 #include <fcntl.h>
 #include <pthread.h>

@@ -49,6 +51,7 @@
 
 #define FNAME "test"
 #define STR   "this is not a test\n"
+#define TEST_APP "dirtyc0w_child"
 
 static uid_t nobody_uid;
 static gid_t nobody_gid;

@@ -67,6 +70,7 @@ void dirtyc0w_test(void)
 {
 	int i, fd, pid, fail = 0;
 	char c;
+	char *av[] = {TEST_APP, NULL};
 
 	/* Create file */
 	fd = SAFE_OPEN(FNAME, O_WRONLY|O_CREAT|O_EXCL, 0444);

@@ -78,9 +82,11 @@ void dirtyc0w_test(void)
 	if (!pid) {
 		SAFE_SETGID(nobody_gid);
 		SAFE_SETUID(nobody_uid);
-		SAFE_EXECLP("dirtyc0w_child", "dirtyc0w_child", NULL);
+		(void)execvpe(TEST_APP, av, tst_ipc_envp);
+		tst_brk(TBROK|TERRNO, "exec failed");
 	}
 
+	TST_CHECKPOINT_WAIT(0);
 	for (i = 0; i < 100; i++)  {
 		usleep(10000);

@@ -104,6 +110,7 @@ void dirtyc0w_test(void)
 
 static struct tst_test test = {
 	.needs_tmpdir = 1,
+	.needs_checkpoints = 1,
 	.forks_child = 1,
 	.needs_root = 1,
 	.setup = setup,

testcases/kernel/security/dirtyc0w/dirtyc0w_child.c +3 -0

index 49abdd6ba52e..bb93c62cb979 100644
@@ -104,7 +104,10 @@ int main(void)
 	int fd;
 	struct stat st;
 
+	tst_reinit();
+
 	SAFE_SIGNAL(SIGUSR1, sighandler);
+	TST_CHECKPOINT_WAKE(0);
 
 	/* Open it read only and map */
 	fd = SAFE_OPEN(FNAME, O_RDONLY);

Signature · 1 line

1.8.3.1

Cyril Hrubis

2017-10-09 14:43 UTC #2 lore raw

PATCH v2 2/2 security/dirtyc0w: synchronize parent and child

Hi!

testcases/kernel/security/dirtyc0w/dirtyc0w.c void dirtyc0w_test(void) · line 86

Quoted 2 lines

testcases/kernel/security/dirtyc0w/dirtyc0w.c +2 -0
+		(void)execvpe(TEST_APP, av, tst_ipc_envp);
+		tst_brk(TBROK|TERRNO, "exec failed");

Can we please add SAFE_EXECVPE() to the library?

Other than that it looks good to me, acked.

Signature · 2 lines

Cyril Hrubis
[email protected]

Jan Stancek

author

2017-10-10 12:17 UTC #3 lore raw

PATCH v2 2/2 security/dirtyc0w: synchronize parent and child

----- Original Message -----

Quoted 7 lines

Hi!
Review
Quoted 2 lines
(quoted diff) +2 -0
+		(void)execvpe(TEST_APP, av, tst_ipc_envp);
+		tst_brk(TBROK|TERRNO, "exec failed");
Can we please add SAFE_EXECVPE() to the library?
Other than that it looks good to me, acked.

I ran into a problem with older distros - execvpe() does not exist there. How about we stay with execlp() and export LTP_IPC_PATH by default, so that child inherits it?

Regards,
Jan

---

Subject: [PATCH/RFC] lib: add LTP_IPC_PATH to env. variables by default

Child started by exec() that needs to do tst_reinit() in order to use checkpoints will inherit LTP_IPC_PATH by default. Parent can choose to override/omit it in envp array when calling exec[lvp]e().

Signed-off-by Jan Stancek <[email protected]>

---
 lib/tst_test.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

lib/tst_test.c +4 -2

index 233b370794a6..27a0b7c36679 100644
@@ -100,10 +100,12 @@ static void setup_ipc(void)
        results = SAFE_MMAP(NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED, ipc_fd, 0);
 
        /* Checkpoints needs to be accessible from processes started by exec() */
-       if (tst_test->needs_checkpoints)
+       if (tst_test->needs_checkpoints) {
                sprintf(ipc_path, IPC_ENV_VAR "=%s", shm_path);
-       else
+               putenv(ipc_path);
+       } else {
                SAFE_UNLINK(shm_path);
+       }
 
        SAFE_CLOSE(ipc_fd);

Cyril Hrubis

2017-10-10 12:22 UTC #4 lore raw

PATCH v2 2/2 security/dirtyc0w: synchronize parent and child

Hi!

Quoted 3 lines

I ran into a problem with older distros - execvpe() does not exist there. How about we stay with execlp() and export LTP_IPC_PATH by default, so that child inherits it?

I guess that we can do setenv() in the test library once we initialize the IPC.

Signature · 2 lines

Cyril Hrubis
[email protected]