Re: [resend][PATCH] Added PR_SET_PROCTITLE_AREA option for prctl()

[KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>]

[-- Attachment #1: Type: text/plain, Size: 2141 bytes --]

Hi

> > >>> The solution is to use the seqlock to detect this, and prevent the
> > >>> secret information from ever making it back to process B's userspace.
> > >>> Note that it's not enough to just recheck arg_start, as process A may
> > >>> reassign the proctitle area back to its original position after having
> > >>> it somewhere else for a while.
> > >>
> > >> Well seqlock is _a_ solution. __Another is to use a mutex or an rwsem
> > >> around the whole operation.
> > >>
> > >> With the code as you propose it, what happens if a process sits in a
> > >> tight loop running setproctitle? __Do other processes running `ps' get
> > >> stuck in a livelock until the offending process gets scheduled out?
> > >
> > > It does seem like a maximum spin count should be put in there - and
> > > maybe a timeout as well (since with FUSE etc it's possible to engineer
> > > page faults that take arbitrarily long).
> > > Also, it occurs to me that:
> > 
> > makes sense.
> > I like maximum spin rather than timeout.
> 
> Start simple.  What's wrong with mutex_lock() on the reader and writer
> sides?  rwsems might be OK too.
> 
> In both cases we should think about whether persistent readers can
> block the writer excessively though.

I thought your mention seems reasonable. then I mesured various locking
performance.

		no-contention	read-read contetion	read-write contention
w/o patch	4627 ms		 7575 ms		 N/A
mutex		5717 ms		33872 ms (!)		14793 ms
rw-semaphoe	6846 ms		10734 ms		36156 ms (!)
seqlock		4754 ms		 7558 ms		 9373 ms

Umm, seqlock is significantly better than other.

<testcase>
	readtitle.c	read proctitle 1,000,000 times
	setproctitle.c	infinite loop of setproctitle()

no-contention: 
	./readtitle 1

read-read contention:
	./readtitle 1 &; ./readtitle 1&; wait

read-write contention
	./setproctitle
	[switch other terminal]
	./readtitle `pidof setproctitle`


I agree this testcase is too pessimistic. ps doesn't read /proc/{pid}/cmdline
so frequently. however if we need to concern DoS attack, we need to mesure
pessimistic scenario.

Plus, this result indicate setproctitle-seqlock doesn't need timeout nor
max spin.


[-- Attachment #2: setproctitle-mutex.patch --]
[-- Type: application/octet-stream, Size: 3760 bytes --]

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 837469a..3470ee3 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -255,32 +255,44 @@ static int proc_pid_cmdline(struct task_struct *task, char * buffer)
 	int res = 0;
 	unsigned int len;
 	struct mm_struct *mm = get_task_mm(task);
+
 	if (!mm)
 		goto out;
+
+	/* The process was not constructed yet? */
 	if (!mm->arg_end)
 		goto out_mm;	/* Shh! No looking before we're done */
 
- 	len = mm->arg_end - mm->arg_start;
- 
+	mutex_lock(&mm->arg_lock);
+	len = mm->arg_end - mm->arg_start;
 	if (len > PAGE_SIZE)
 		len = PAGE_SIZE;
- 
+
 	res = access_process_vm(task, mm->arg_start, buffer, len, 0);
 
-	// If the nul at the end of args has been overwritten, then
-	// assume application is using setproctitle(3).
-	if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) {
+	if (mm->arg_end != mm->env_start)
+		/* PR_SET_PROCTITLE_AREA used */
+		res = strnlen(buffer, res);
+	else if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) {
+		/*
+		 * If the nul at the end of args has been overwritten,
+		 * then assume application is using sendmail's
+		 * SPT_REUSEARGV style argv override.
+		 */
 		len = strnlen(buffer, res);
 		if (len < res) {
-		    res = len;
+			res = len;
 		} else {
 			len = mm->env_end - mm->env_start;
 			if (len > PAGE_SIZE - res)
 				len = PAGE_SIZE - res;
-			res += access_process_vm(task, mm->env_start, buffer+res, len, 0);
+			res += access_process_vm(task, mm->env_start,
+						 buffer+res, len, 0);
 			res = strnlen(buffer, res);
 		}
 	}
+	mutex_unlock(&mm->arg_lock);
+
 out_mm:
 	mmput(mm);
 out:
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 84a524a..3e2a346 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -12,6 +12,7 @@
 #include <linux/completion.h>
 #include <linux/cpumask.h>
 #include <linux/page-debug-flags.h>
+#include <linux/mutex.h>
 #include <asm/page.h>
 #include <asm/mmu.h>
 
@@ -236,6 +237,7 @@ struct mm_struct {
 	unsigned long stack_vm, reserved_vm, def_flags, nr_ptes;
 	unsigned long start_code, end_code, start_data, end_data;
 	unsigned long start_brk, brk, start_stack;
+	struct mutex arg_lock;
 	unsigned long arg_start, arg_end, env_start, env_end;
 
 	unsigned long saved_auxv[AT_VECTOR_SIZE]; /* for /proc/PID/auxv */
diff --git a/include/linux/prctl.h b/include/linux/prctl.h
index 9311505..e80a11b 100644
--- a/include/linux/prctl.h
+++ b/include/linux/prctl.h
@@ -90,4 +90,8 @@
 
 #define PR_MCE_KILL	33
 
+
+/* Set process title memory area for setproctitle() */
+#define PR_SET_PROCTITLE_AREA 34
+
 #endif /* _LINUX_PRCTL_H */
diff --git a/kernel/fork.c b/kernel/fork.c
index 266c6af..f6ebe46 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -459,6 +459,7 @@ static struct mm_struct * mm_init(struct mm_struct * mm, struct task_struct *p)
 	mm->cached_hole_size = ~0UL;
 	mm_init_aio(mm);
 	mm_init_owner(mm, p);
+	mutex_init(&mm->arg_lock);
 
 	if (likely(!mm_alloc_pgd(mm))) {
 		mm->def_flags = 0;
diff --git a/kernel/sys.c b/kernel/sys.c
index 255475d..e401e54 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1564,6 +1564,29 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 			error = 0;
 			break;
 
+		case PR_SET_PROCTITLE_AREA: {
+			struct mm_struct *mm = current->mm;
+			unsigned long addr = arg2;
+			unsigned long len = arg3;
+			unsigned long end = arg2 + arg3;
+
+			if (len > PAGE_SIZE)
+				return -EINVAL;
+
+			if (addr >= end)
+				return -EINVAL;
+
+			if (!access_ok(VERIFY_READ, addr, len)) {
+				return -EFAULT;
+			}
+
+			mutex_lock(&mm->arg_lock);
+			mm->arg_start = addr;
+			mm->arg_end = addr + len;
+			mutex_unlock(&mm->arg_lock);
+
+			return 0;
+		}
 		default:
 			error = -EINVAL;
 			break;

[-- Attachment #3: setproctitle-rwmutex.patch --]
[-- Type: application/octet-stream, Size: 3378 bytes --]

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 837469a..e95878a 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -255,21 +255,30 @@ static int proc_pid_cmdline(struct task_struct *task, char * buffer)
 	int res = 0;
 	unsigned int len;
 	struct mm_struct *mm = get_task_mm(task);
+
 	if (!mm)
 		goto out;
+
+	/* The process was not constructed yet? */
 	if (!mm->arg_end)
 		goto out_mm;	/* Shh! No looking before we're done */
 
- 	len = mm->arg_end - mm->arg_start;
- 
+	down_read(&mm->arg_sem);
+	len = mm->arg_end - mm->arg_start;
 	if (len > PAGE_SIZE)
 		len = PAGE_SIZE;
- 
+
 	res = access_process_vm(task, mm->arg_start, buffer, len, 0);
 
-	// If the nul at the end of args has been overwritten, then
-	// assume application is using setproctitle(3).
-	if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) {
+	if (mm->arg_end != mm->env_start)
+		/* PR_SET_PROCTITLE_AREA used */
+		res = strnlen(buffer, res);
+	else if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) {
+		/*
+		 * If the nul at the end of args has been overwritten,
+		 * then assume application is using sendmail's
+		 * SPT_REUSEARGV style argv override.
+		 */
 		len = strnlen(buffer, res);
 		if (len < res) {
 		    res = len;
@@ -281,6 +290,8 @@ static int proc_pid_cmdline(struct task_struct *task, char * buffer)
 			res = strnlen(buffer, res);
 		}
 	}
+	up_read(&mm->arg_sem);
+
 out_mm:
 	mmput(mm);
 out:
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 84a524a..aff2e8d 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -236,6 +236,7 @@ struct mm_struct {
 	unsigned long stack_vm, reserved_vm, def_flags, nr_ptes;
 	unsigned long start_code, end_code, start_data, end_data;
 	unsigned long start_brk, brk, start_stack;
+	struct rw_semaphore arg_sem;
 	unsigned long arg_start, arg_end, env_start, env_end;
 
 	unsigned long saved_auxv[AT_VECTOR_SIZE]; /* for /proc/PID/auxv */
diff --git a/include/linux/prctl.h b/include/linux/prctl.h
index 9311505..e80a11b 100644
--- a/include/linux/prctl.h
+++ b/include/linux/prctl.h
@@ -90,4 +90,8 @@
 
 #define PR_MCE_KILL	33
 
+
+/* Set process title memory area for setproctitle() */
+#define PR_SET_PROCTITLE_AREA 34
+
 #endif /* _LINUX_PRCTL_H */
diff --git a/kernel/fork.c b/kernel/fork.c
index 266c6af..014999c 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -459,6 +459,7 @@ static struct mm_struct * mm_init(struct mm_struct * mm, struct task_struct *p)
 	mm->cached_hole_size = ~0UL;
 	mm_init_aio(mm);
 	mm_init_owner(mm, p);
+	init_rwsem(&mm->arg_sem);
 
 	if (likely(!mm_alloc_pgd(mm))) {
 		mm->def_flags = 0;
diff --git a/kernel/sys.c b/kernel/sys.c
index 255475d..cd69a06 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1564,6 +1564,29 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 			error = 0;
 			break;
 
+		case PR_SET_PROCTITLE_AREA: {
+			struct mm_struct *mm = current->mm;
+			unsigned long addr = arg2;
+			unsigned long len = arg3;
+			unsigned long end = arg2 + arg3;
+
+			if (len > PAGE_SIZE)
+				return -EINVAL;
+
+			if (addr >= end)
+				return -EINVAL;
+
+			if (!access_ok(VERIFY_READ, addr, len)) {
+				return -EFAULT;
+			}
+
+			down_write(&mm->arg_sem);
+			mm->arg_start = addr;
+			mm->arg_end = addr + len;
+			up_write(&mm->arg_sem);
+
+			return 0;
+		}
 		default:
 			error = -EINVAL;
 			break;

[-- Attachment #4: setproctitle-seqlock.patch --]
[-- Type: application/octet-stream, Size: 4138 bytes --]

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 837469a..5ac6ece 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -255,32 +255,47 @@ static int proc_pid_cmdline(struct task_struct *task, char * buffer)
 	int res = 0;
 	unsigned int len;
 	struct mm_struct *mm = get_task_mm(task);
+	unsigned seq;
+
 	if (!mm)
 		goto out;
+
+	/* The process was not constructed yet? */
 	if (!mm->arg_end)
-		goto out_mm;	/* Shh! No looking before we're done */
+		goto out_mm;
 
- 	len = mm->arg_end - mm->arg_start;
- 
-	if (len > PAGE_SIZE)
-		len = PAGE_SIZE;
- 
-	res = access_process_vm(task, mm->arg_start, buffer, len, 0);
-
-	// If the nul at the end of args has been overwritten, then
-	// assume application is using setproctitle(3).
-	if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) {
-		len = strnlen(buffer, res);
-		if (len < res) {
-		    res = len;
-		} else {
-			len = mm->env_end - mm->env_start;
-			if (len > PAGE_SIZE - res)
-				len = PAGE_SIZE - res;
-			res += access_process_vm(task, mm->env_start, buffer+res, len, 0);
+	do {
+		seq = read_seqbegin(&mm->arg_lock);
+
+		len = mm->arg_end - mm->arg_start;
+		if (len > PAGE_SIZE)
+			len = PAGE_SIZE;
+
+		res = access_process_vm(task, mm->arg_start, buffer, len, 0);
+
+		if (mm->arg_end != mm->env_start)
+			/* PR_SET_PROCTITLE_AREA used */
 			res = strnlen(buffer, res);
+		else if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) {
+			/*
+			 * If the nul at the end of args has been overwritten,
+			 * then assume application is using sendmail's
+			 * SPT_REUSEARGV style argv override.
+			 */
+			len = strnlen(buffer, res);
+			if (len < res) {
+				res = len;
+			} else {
+				len = mm->env_end - mm->env_start;
+				if (len > PAGE_SIZE - res)
+					len = PAGE_SIZE - res;
+				res += access_process_vm(task, mm->env_start,
+							 buffer+res, len, 0);
+				res = strnlen(buffer, res);
+			}
 		}
-	}
+	} while (read_seqretry(&mm->arg_lock, seq));
+
 out_mm:
 	mmput(mm);
 out:
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 84a524a..279d620 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -12,6 +12,7 @@
 #include <linux/completion.h>
 #include <linux/cpumask.h>
 #include <linux/page-debug-flags.h>
+#include <linux/seqlock.h>
 #include <asm/page.h>
 #include <asm/mmu.h>
 
@@ -236,6 +237,7 @@ struct mm_struct {
 	unsigned long stack_vm, reserved_vm, def_flags, nr_ptes;
 	unsigned long start_code, end_code, start_data, end_data;
 	unsigned long start_brk, brk, start_stack;
+	seqlock_t arg_lock;
 	unsigned long arg_start, arg_end, env_start, env_end;
 
 	unsigned long saved_auxv[AT_VECTOR_SIZE]; /* for /proc/PID/auxv */
diff --git a/include/linux/prctl.h b/include/linux/prctl.h
index 9311505..e80a11b 100644
--- a/include/linux/prctl.h
+++ b/include/linux/prctl.h
@@ -90,4 +90,8 @@
 
 #define PR_MCE_KILL	33
 
+
+/* Set process title memory area for setproctitle() */
+#define PR_SET_PROCTITLE_AREA 34
+
 #endif /* _LINUX_PRCTL_H */
diff --git a/kernel/fork.c b/kernel/fork.c
index 266c6af..13089e3 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -459,6 +459,7 @@ static struct mm_struct * mm_init(struct mm_struct * mm, struct task_struct *p)
 	mm->cached_hole_size = ~0UL;
 	mm_init_aio(mm);
 	mm_init_owner(mm, p);
+	seqlock_init(&mm->arg_lock);
 
 	if (likely(!mm_alloc_pgd(mm))) {
 		mm->def_flags = 0;
diff --git a/kernel/sys.c b/kernel/sys.c
index 255475d..83724d2 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1563,7 +1563,29 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 			}
 			error = 0;
 			break;
+ 		case PR_SET_PROCTITLE_AREA: {
+			struct mm_struct *mm = current->mm;
+			unsigned long addr = arg2;
+			unsigned long len = arg3;
+			unsigned long end = arg2 + arg3;
 
+			if (len > PAGE_SIZE)
+				return -EINVAL;
+
+			if (addr >= end)
+				return -EINVAL;
+
+			if (!access_ok(VERIFY_READ, addr, len)) {
+				return -EFAULT;
+			}
+
+			write_seqlock(&mm->arg_lock);
+			mm->arg_start = addr;
+			mm->arg_end = addr + len;
+			write_sequnlock(&mm->arg_lock);
+
+			return 0;
+		}
 		default:
 			error = -EINVAL;
 			break;

[-- Attachment #5: readtitle.c --]
[-- Type: application/octet-stream, Size: 672 bytes --]

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <sys/time.h>

#define LOOP (1000000)

int main(int argc, char** argv)
{
	char path[128];
	char buf[1024];
	FILE* file;
	int i;
	struct timeval start, end;
	long long elaps;

	sprintf(path, "/proc/%s/cmdline", argv[1]);

	gettimeofday(&start, NULL);

	file = fopen(path, "r");
	for (i=0; i<LOOP; i++) {
		fread(buf, 1024, 1, file);
	}

	gettimeofday(&end, NULL);
	printf("%s\n", buf);

	elaps = end.tv_sec * 1000 + end.tv_usec /1000;
	elaps -= start.tv_sec * 1000;
	elaps -= start.tv_usec / 1000;

	printf("time = %lld ms\n", elaps);

	return 0;
}

[-- Attachment #6: setproctitle.c --]
[-- Type: application/octet-stream, Size: 571 bytes --]

 #include <string.h>
 #include <stdlib.h>
 #include <unistd.h>
 #include <stdio.h>
 #include <sys/prctl.h>

 #define ERR(str) (perror(str), exit(1))

 void settitle(char* title){
         int err;

         err = prctl(34, title, strlen(title)+1);
         if (err < 0)
                 ERR("prctl ");
 }

 int main(void){
         long i;
         char buf[1024];

         for (i = 0; i < 10000000000LL; i++){
                 sprintf(buf, "loooooooooooooooooooooooong string %ld",i);
                 settitle(buf);
         }
	 return 0;
 }