Added PR_SET_PROCTITLE_AREA option for prctl()
in [Kernel]
|
Prev: pciehp_driver.c: PCIe hot plug for Intel iommu - call IOMMU API in hot remove
Next: 2.6.31 regression sis5513 PIO Mode 0 hang
From: KOSAKI Motohiro on 3 Nov 2009 09:10 > >+ 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; > > Is this safe? You're assigning a user-space pointer to kernel space... > Don't we need copy_from_user()? mm->arg_start, arg_end are defined so. Please see current implementation. > >+ mm->arg_end = addr + len; > > Since you already have 'end', no need to caculate this again. :) Good catch :) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Américo Wang on 3 Nov 2009 11:20 On Wed, Nov 04, 2009 at 12:26:44AM +0900, KOSAKI Motohiro wrote: >> > >+ 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; >> > >> > Is this safe? You're assigning a user-space pointer to kernel space... >> > Don't we need copy_from_user()? >> >> mm->arg_start, arg_end are defined so. >> Please see current implementation. Hmm, yeah. >> >> >> > >+ mm->arg_end = addr + len; >> > >> > Since you already have 'end', no need to caculate this again. :) >> >> Good catch :) >> >> > >Fixed. > > >ChangeLog > v4 -> v5 > - nit: kill duplicate calculation in prctl() > v3 -> v4 > - Use mutex instead seq_lock. > >======================================== > >Subject: [PATCH v5] Added PR_SET_PROCTITLE_AREA option for prctl() >From: Timo Sirainen <tss(a)iki.fi> > >Currently glibc2 doesn't have setproctitle(3), so several userland >daemons attempt to emulate it by doing some brutal stack modifications. >This works most of the time, but it has problems. For example: > > % ps -ef |grep avahi-daemon > avahi 1679 1 0 09:20 ? 00:00:00 avahi-daemon: running [kosadesk.local] > > # cat /proc/1679/cmdline > avahi-daemon: running [kosadesk.local] > >This looks good, but the process has also overwritten its environment >area and made the environ file useless: > > # cat /proc/1679/environ > adesk.local] > >Another problem is that the process title length is limited by the size of >the environment. Security conscious people try to avoid potential information >leaks by clearing most of the environment before running a daemon: > > # env - MINIMUM_NEEDED_VAR=foo /path/to/daemon > >The resulting environment size may be too small to fit the wanted process >titles. > >This patch makes it possible for userspace to implement setproctitle() >cleanly. It adds a new PR_SET_PROCTITLE_AREA option for prctl(), which >updates task's mm_struct->arg_start and arg_end to the given area. > > test_setproctitle.c > ================================================ > #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 "); > } > > void main(void){ > long i; > char buf[1024]; > > for (i = 0; i < 10000000000LL; i++){ > sprintf(buf, "loooooooooooooooooooooooong string %d",i); > settitle(buf); > } > } > ================================================== > >Cc: Bryan Donlan <bdonlan(a)gmail.com> >Cc: Ulrich Drepper <drepper(a)redhat.com> >Signed-off-by: KOSAKI Motohiro <kosaki.motohiro(a)jp.fujitsu.com> >Signed-off-by: Timo Sirainen <tss(a)iki.fi> This version looks good, Reviewed-by: WANG Cong <xiyou.wangcong(a)gmail.com> Thanks! >--- > fs/proc/base.c | 31 ++++++++++++++++++++++--------- > include/linux/mm_types.h | 2 ++ > include/linux/prctl.h | 3 +++ > kernel/fork.c | 1 + > kernel/sys.c | 22 ++++++++++++++++++++++ > 5 files changed, 50 insertions(+), 9 deletions(-) > >diff --git a/fs/proc/base.c b/fs/proc/base.c >index 837469a..ac800b4 100644 >--- a/fs/proc/base.c >+++ b/fs/proc/base.c >@@ -255,32 +255,45 @@ 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 (mm->arg_end != mm->env_start) >+ /* prctl(PR_SET_PROCTITLE_AREA) used */ >+ goto out_unlock; > >- // If the nul at the end of args has been overwritten, then >- // assume application is using setproctitle(3). >+ /* >+ * If the nul at the end of args has been overwritten, then assume >+ * application is using sendmail's SPT_REUSEARGV style argv override. >+ */ > if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) { > len = strnlen(buffer, res); >- if (len < res) { >- res = len; >- } else { >+ 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 += access_process_vm(task, mm->env_start, >+ buffer+res, len, 0); > res = strnlen(buffer, res); > } > } >+ >+out_unlock: >+ 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..da47542 100644 >--- a/include/linux/prctl.h >+++ b/include/linux/prctl.h >@@ -90,4 +90,7 @@ > > #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 4c20fff..881a6b4 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..bde6957 100644 >--- a/kernel/sys.c >+++ b/kernel/sys.c >@@ -1564,6 +1564,28 @@ 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 = end; >+ mutex_unlock(&mm->arg_lock); >+ >+ return 0; >+ } > default: > error = -EINVAL; > break; >-- >1.6.2.5 > > > > -- Live like a child, think like the god. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Andrew Morton on 9 Nov 2009 17:50 On Wed, 4 Nov 2009 00:26:44 +0900 (JST) KOSAKI Motohiro <kosaki.motohiro(a)jp.fujitsu.com> wrote: > ======================================== > > Subject: [PATCH v5] Added PR_SET_PROCTITLE_AREA option for prctl() > From: Timo Sirainen <tss(a)iki.fi> > > Currently glibc2 doesn't have setproctitle(3), so several userland > daemons attempt to emulate it by doing some brutal stack modifications. > This works most of the time, but it has problems. For example: > > % ps -ef |grep avahi-daemon > avahi 1679 1 0 09:20 ? 00:00:00 avahi-daemon: running [kosadesk.local] > > # cat /proc/1679/cmdline > avahi-daemon: running [kosadesk.local] > > This looks good, but the process has also overwritten its environment > area and made the environ file useless: > > # cat /proc/1679/environ > adesk.local] > > Another problem is that the process title length is limited by the size of > the environment. Security conscious people try to avoid potential information > leaks by clearing most of the environment before running a daemon: > > # env - MINIMUM_NEEDED_VAR=foo /path/to/daemon > > The resulting environment size may be too small to fit the wanted process > titles. > > This patch makes it possible for userspace to implement setproctitle() > cleanly. It adds a new PR_SET_PROCTITLE_AREA option for prctl(), which > updates task's mm_struct->arg_start and arg_end to the given area. > > test_setproctitle.c > ================================================ > #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 "); > } > > void main(void){ > long i; > char buf[1024]; > > for (i = 0; i < 10000000000LL; i++){ > sprintf(buf, "loooooooooooooooooooooooong string %d",i); > settitle(buf); > } > } What happens if userspace unmaps the memory after telling the kernel to use it? Will processes which try to read the command line get an error reading /proc? If so, do all the commandline-reading programs in the world handle this in an appropriate fashion? > diff --git a/fs/proc/base.c b/fs/proc/base.c > index 837469a..ac800b4 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -255,32 +255,45 @@ 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 (mm->arg_end != mm->env_start) > + /* prctl(PR_SET_PROCTITLE_AREA) used */ > + goto out_unlock; > > - // If the nul at the end of args has been overwritten, then > - // assume application is using setproctitle(3). > + /* > + * If the nul at the end of args has been overwritten, then assume > + * application is using sendmail's SPT_REUSEARGV style argv override. > + */ > if (res > 0 && buffer[res-1] != '\0' && len < PAGE_SIZE) { > len = strnlen(buffer, res); > - if (len < res) { > - res = len; > - } else { > + 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 += access_process_vm(task, mm->env_start, > + buffer+res, len, 0); > res = strnlen(buffer, res); > } > } > + > +out_unlock: > + 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 */ Please document the role of arg_lock with a code comment here. > diff --git a/include/linux/prctl.h b/include/linux/prctl.h > index 9311505..da47542 100644 > --- a/include/linux/prctl.h > +++ b/include/linux/prctl.h > @@ -90,4 +90,7 @@ > > #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 4c20fff..881a6b4 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..bde6957 100644 > --- a/kernel/sys.c > +++ b/kernel/sys.c > @@ -1564,6 +1564,28 @@ 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; It's unobvious (to me) why this access_ok() check is here. If that wasn't totally dumb of me, please add a comment so the next reader won't be similarly mystified. > + mutex_lock(&mm->arg_lock); > + mm->arg_start = addr; > + mm->arg_end = end; > + mutex_unlock(&mm->arg_lock); > + > + return 0; > + } > default: > error = -EINVAL; > break; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Bryan Donlan on 9 Nov 2009 19:10 On Mon, Nov 9, 2009 at 5:47 PM, Andrew Morton <akpm(a)linux-foundation.org> wrote: > What happens if userspace unmaps the memory after telling the kernel to > use it? > > Will processes which try to read the command line get an error reading > /proc? �If so, do all the commandline-reading programs in the world > handle this in an appropriate fashion? This case can already occur in the current code; the userspace process would have to munmap() the top of its stack, but it certainly can do so if it tries. In any case, access_process_vm() then returns 0 because of the fault, and thus /proc/pid/cmdline is seen to have zero length. Since a zero-length /proc/pid/cmdline occurs with kernel threads as well, we know this isn't a problem. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: KOSAKI Motohiro on 10 Nov 2009 06:10
2009/11/10 Bryan Donlan <bdonlan(a)gmail.com>: > On Mon, Nov 9, 2009 at 5:47 PM, Andrew Morton <akpm(a)linux-foundation.org> wrote: > >> What happens if userspace unmaps the memory after telling the kernel to >> use it? >> >> Will processes which try to read the command line get an error reading >> /proc? �If so, do all the commandline-reading programs in the world >> handle this in an appropriate fashion? > > This case can already occur in the current code; the userspace process > would have to munmap() the top of its stack, but it certainly can do > so if it tries. In any case, access_process_vm() then returns 0 > because of the fault, and thus /proc/pid/cmdline is seen to have zero > length. Since a zero-length /proc/pid/cmdline occurs with kernel > threads as well, we know this isn't a problem. Plus, ps can read under exiting process. In this case, task->mm is NULL and proc_pid_cmdline return 0. procps tools are already NUL safe since long time ago. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |