"ps -e" without procfs(5)
Hi, I have a patch that makes kvm_uread() read from user space using ptrace(2). http://people.freebsd.org/~trociny/kvm_uread.ptrace.patch With this change 'ps -e' does not requires procfs(5). Do you like it or there might be some reasons why it is a bad idea? Grepping sources it looks like currently only ps uses kvm_getenvv(3) (and thus kvm_uread()). Note, when reading from its own user space it just does bcopy(3), so if a wrong address range is passed to kvm_uread() the program will segfault. Do we need some protection here and what? Masking SIGSEGV? -- Mikolaj Golub ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: "ps -e" without procfs(5)
On Sun, Oct 16, 2011 at 07:57:57PM +0300, Mikolaj Golub wrote: > Hi, > > I have a patch that makes kvm_uread() read from user space using ptrace(2). > > http://people.freebsd.org/~trociny/kvm_uread.ptrace.patch > > With this change 'ps -e' does not requires procfs(5). > > Do you like it or there might be some reasons why it is a bad idea? > > Grepping sources it looks like currently only ps uses kvm_getenvv(3) (and thus > kvm_uread()). > > Note, when reading from its own user space it just does bcopy(3), so if a > wrong address range is passed to kvm_uread() the program will segfault. Do we > need some protection here and what? Masking SIGSEGV? Ptracing a random process may have a disastrous consequences for the traced process, caller or system. For the process, the ptrace(2) can cause spurious signal delivery or EINTR returns. If the process you attached to is critical for the system operation, you can get hung or failed system. In my opinion, the way to implement the feature is to (re)use linprocfs_doargv() and provide another kern.proc sysctl to retrieve the argv and env vectors. Then, ps(1) and procstat(1) can use it, as well as procfs and linprocfs inside the kernel. While you are at the code, it would be useful to also export the auxv vector, which is immediately before env. pgpn6KWHsiqIL.pgp Description: PGP signature
Re: "ps -e" without procfs(5)
On Sun, 16 Oct 2011 20:10:05 +0300 Kostik Belousov wrote: KB> In my opinion, the way to implement the feature is to (re)use KB> linprocfs_doargv() and provide another kern.proc sysctl to retrieve the KB> argv and env vectors. Then, ps(1) and procstat(1) can use it, as well as KB> procfs and linprocfs inside the kernel. Thanks! I am testing a patch (without auxv vector so far) and have some questions. Original ps -e returns environment only for user owned processes (the access is restricted by the permissions of /proc/pid/mem file). My kern.proc.env sysctl does not have such a restriction. I suppose I should add it? What function I could use for this? BTW, linprocfs allows to read other user's environment. KB> While you are at the code, it would be useful to also export the auxv vector, KB> which is immediately before env. It looks I can find the location of auxv but what about the size? Or do you propose to extend struct ps_strings to store location and size of auxv? I could do this way... -- Mikolaj Golub ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: "ps -e" without procfs(5)
On Tue, Oct 25, 2011 at 12:13:10AM +0300, Mikolaj Golub wrote:
>
> On Sun, 16 Oct 2011 20:10:05 +0300 Kostik Belousov wrote:
>
> KB> In my opinion, the way to implement the feature is to (re)use
> KB> linprocfs_doargv() and provide another kern.proc sysctl to retrieve the
> KB> argv and env vectors. Then, ps(1) and procstat(1) can use it, as well as
> KB> procfs and linprocfs inside the kernel.
>
> Thanks! I am testing a patch (without auxv vector so far) and have some
> questions.
>
> Original ps -e returns environment only for user owned processes (the access
> is
> restricted by the permissions of /proc/pid/mem file). My kern.proc.env sysctl
> does not have such a restriction. I suppose I should add it? What function I
> could use for this?
>
> BTW, linprocfs allows to read other user's environment.
linprocfs uses p_cansee() to check the permissions. There are sysctls
security.bsd.see_other_{ug}ids that control the behaviour.
I believe that the new sysctl shall use the same check.
>
> KB> While you are at the code, it would be useful to also export the auxv
> vector,
> KB> which is immediately before env.
>
> It looks I can find the location of auxv but what about the size? Or do you
> propose to extend struct ps_strings to store location and size of auxv? I
> could do this way...
No, extending ps_strings is not needed and it is too radical change.
The auxv vector must end by the AT_NULL aux entry. You can also artificially
limit the amount of read aux vectors to, say, 256, which is much more then
it is currently defined.
pgpHLn9etVmXO.pgp
Description: PGP signature
Re: "ps -e" without procfs(5)
On 25 Oct 2011, at 09:24, Kostik Belousov wrote:
> On Tue, Oct 25, 2011 at 12:13:10AM +0300, Mikolaj Golub wrote:
>>
>> On Sun, 16 Oct 2011 20:10:05 +0300 Kostik Belousov wrote:
>>
>> KB> In my opinion, the way to implement the feature is to (re)use
>> KB> linprocfs_doargv() and provide another kern.proc sysctl to retrieve the
>> KB> argv and env vectors. Then, ps(1) and procstat(1) can use it, as well as
>> KB> procfs and linprocfs inside the kernel.
>>
>> Thanks! I am testing a patch (without auxv vector so far) and have some
>> questions.
>>
>> Original ps -e returns environment only for user owned processes (the access
>> is
>> restricted by the permissions of /proc/pid/mem file). My kern.proc.env sysctl
>> does not have such a restriction. I suppose I should add it? What function I
>> could use for this?
>>
>> BTW, linprocfs allows to read other user's environment.
> linprocfs uses p_cansee() to check the permissions. There are sysctls
> security.bsd.see_other_{ug}ids that control the behaviour.
>
> I believe that the new sysctl shall use the same check.
To be honest, I'd be far more comfortable if the environment check used
p_candebug(). Environmental variables sometimes contain passwords, etc, that
shouldn't be visible to other users on the system. Even showing command lines
is a bit dubious, but widely accepted, whereas seeing the contents of
environmental variables is not widely known in user communities.
Robert___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: "ps -e" without procfs(5)
On Tue, 25 Oct 2011 11:24:51 +0300 Kostik Belousov wrote:
KB> On Tue, Oct 25, 2011 at 12:13:10AM +0300, Mikolaj Golub wrote:
>>
>> On Sun, 16 Oct 2011 20:10:05 +0300 Kostik Belousov wrote:
>>
>> KB> In my opinion, the way to implement the feature is to (re)use
>> KB> linprocfs_doargv() and provide another kern.proc sysctl to retrieve the
>> KB> argv and env vectors. Then, ps(1) and procstat(1) can use it, as well
>> as
>> KB> procfs and linprocfs inside the kernel.
>>
>> Thanks! I am testing a patch (without auxv vector so far) and have some
>> questions.
>>
>> Original ps -e returns environment only for user owned processes (the
>> access is
>> restricted by the permissions of /proc/pid/mem file). My kern.proc.env
>> sysctl
>> does not have such a restriction. I suppose I should add it? What function I
>> could use for this?
>>
>> BTW, linprocfs allows to read other user's environment.
KB> linprocfs uses p_cansee() to check the permissions. There are sysctls
KB> security.bsd.see_other_{ug}ids that control the behaviour.
KB> I believe that the new sysctl shall use the same check.
This looks reasonable for me. But I just wanted to be sure that this would be
ok for other people, as my patch changes the system behavior: currently with
security.bsd.see_other_{ug}ids and procfs (not linprocfs) mounted a user can
see other users args but not env; after the change a user will see both args
and env (until security.bsd.see_other_{ug}ids is off).
>>
>> KB> While you are at the code, it would be useful to also export the auxv
>> vector,
>> KB> which is immediately before env.
>>
>> It looks I can find the location of auxv but what about the size? Or do you
>> propose to extend struct ps_strings to store location and size of auxv? I
>> could do this way...
KB> No, extending ps_strings is not needed and it is too radical change.
KB> The auxv vector must end by the AT_NULL aux entry. You can also
artificially
KB> limit the amount of read aux vectors to, say, 256, which is much more then
KB> it is currently defined.
Thanks.
--
Mikolaj Golub
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: "ps -e" without procfs(5)
On Tue, 25 Oct 2011 11:24:51 +0300 Kostik Belousov wrote:
KB> On Tue, Oct 25, 2011 at 12:13:10AM +0300, Mikolaj Golub wrote:
>>
>> On Sun, 16 Oct 2011 20:10:05 +0300 Kostik Belousov wrote:
>>
>> KB> In my opinion, the way to implement the feature is to (re)use
>> KB> linprocfs_doargv() and provide another kern.proc sysctl to retrieve the
>> KB> argv and env vectors. Then, ps(1) and procstat(1) can use it, as well
>> as
>> KB> procfs and linprocfs inside the kernel.
>>
>> Thanks! I am testing a patch (without auxv vector so far) and have some
>> questions.
>>
>> Original ps -e returns environment only for user owned processes (the
>> access is
>> restricted by the permissions of /proc/pid/mem file). My kern.proc.env
>> sysctl
>> does not have such a restriction. I suppose I should add it? What function I
>> could use for this?
>>
>> BTW, linprocfs allows to read other user's environment.
KB> linprocfs uses p_cansee() to check the permissions. There are sysctls
KB> security.bsd.see_other_{ug}ids that control the behaviour.
KB> I believe that the new sysctl shall use the same check.
>>
>> KB> While you are at the code, it would be useful to also export the auxv
>> vector,
>> KB> which is immediately before env.
>>
>> It looks I can find the location of auxv but what about the size? Or do you
>> propose to extend struct ps_strings to store location and size of auxv? I
>> could do this way...
KB> No, extending ps_strings is not needed and it is too radical change.
KB> The auxv vector must end by the AT_NULL aux entry. You can also
artificially
KB> limit the amount of read aux vectors to, say, 256, which is much more then
KB> it is currently defined.
What do you think about the attached patch? This is a kernel
part. COMPAT_FREEBSD32 has not been tested after the last update (just checked
that it compiles): it looks I will not have access to amd64 box for testing
during the weekend. I will test it after the weekend.
Both kernel and userland parts are available here:
http://people.freebsd.org/~trociny/env.sys.patch
http://people.freebsd.org/~trociny/env.user.patch
Currently there is an issue with procstat -x: if one tried to run it on 64 bit
for a 32 bit process it would not detect this so would output a garbage. Could
somebody recommend a way how to get this info about a process from userlend?
--
Mikolaj Golub
diff --git a/sys/sys/proc.h b/sys/sys/proc.h
index fb97913..4949f98 100644
--- a/sys/sys/proc.h
+++ b/sys/sys/proc.h
@@ -168,6 +168,7 @@ struct p_sched;
struct proc;
struct procdesc;
struct racct;
+struct sbuf;
struct sleepqueue;
struct td_sched;
struct thread;
@@ -843,6 +844,10 @@ int p_canwait(struct thread *td, struct proc *p);
struct pargs *pargs_alloc(int len);
void pargs_drop(struct pargs *pa);
void pargs_hold(struct pargs *pa);
+int proc_getargv(struct thread *td, struct proc *p, struct sbuf *sb,
+ size_t nchr);
+int proc_getenvv(struct thread *td, struct proc *p, struct sbuf *sb,
+ size_t nchr);
void procinit(void);
void proc_linkup0(struct proc *p, struct thread *td);
void proc_linkup(struct proc *p, struct thread *td);
diff --git a/sys/sys/sysctl.h b/sys/sys/sysctl.h
index 1e879f5..99ea342 100644
--- a/sys/sys/sysctl.h
+++ b/sys/sys/sysctl.h
@@ -559,6 +559,8 @@ SYSCTL_ALLOWED_TYPES(UINT64, uint64_t *a; unsigned long long *b; );
#define KERN_PROC_VMMAP 32 /* VM map entries for process */
#define KERN_PROC_FILEDESC 33 /* File descriptors for process */
#define KERN_PROC_GROUPS 34 /* process groups */
+#define KERN_PROC_ENV 35 /* get environment */
+#define KERN_PROC_AUXV 36 /* get ELF auxiliary vector */
/*
* KERN_IPC identifiers
diff --git a/sys/kern/kern_proc.c b/sys/kern/kern_proc.c
index 998e7ca..ef4055a 100644
--- a/sys/kern/kern_proc.c
+++ b/sys/kern/kern_proc.c
@@ -41,6 +41,7 @@ __FBSDID("$FreeBSD$");
#include
#include
+#include
#include
#include
#include
@@ -49,6 +50,7 @@ __FBSDID("$FreeBSD$");
#include
#include
#include
+#include
#include
#include
#include
@@ -66,6 +68,8 @@ __FBSDID("$FreeBSD$");
#include
#include
+#include
+
#ifdef DDB
#include
#endif
@@ -1356,6 +1360,218 @@ pargs_drop(struct pargs *pa)
pargs_free(pa);
}
+static int
+proc_read_mem(struct thread *td, struct proc *p, vm_offset_t offset, void* buf,
+size_t len)
+{
+ struct iovec iov;
+ struct uio uio;
+
+ iov.iov_base = (caddr_t)buf;
+ iov.iov_len = len;
+ uio.uio_iov = &iov;
+ uio.uio_iovcnt = 1;
+ uio.uio_offset = offset;
+ uio.uio_resid = (ssize_t)len;
+ uio.uio_segflg = UIO_SYSSPACE;
+ uio.uio_rw = UIO_READ;
+ uio.uio_td = td;
+
+ return (proc_rwmem(p, &uio));
+}
+
+#define PROC_AUXV_MAX 256 /* Limit on auxv size. */
+
+enum proc_vector_type {
+ PROC_ARG,
+ PROC_ENV,
+ PROC_AUX,
+};
+
+#ifdef COMPAT_FREEBSD32
+static int
+get_proc_vector32(struct thread *td, struct proc *p, char ***proc_vectorp,
+size_t *vsizep, enum proc_vector_type type)
+{
+ struct freebsd32_ps_strings pss;
Re: "ps -e" without procfs(5)
On Sat, Oct 29, 2011 at 01:32:39PM +0300, Mikolaj Golub wrote:
>
> On Tue, 25 Oct 2011 11:24:51 +0300 Kostik Belousov wrote:
>
> KB> On Tue, Oct 25, 2011 at 12:13:10AM +0300, Mikolaj Golub wrote:
> >>
> >> On Sun, 16 Oct 2011 20:10:05 +0300 Kostik Belousov wrote:
> >>
> >> KB> In my opinion, the way to implement the feature is to (re)use
> >> KB> linprocfs_doargv() and provide another kern.proc sysctl to retrieve
> the
> >> KB> argv and env vectors. Then, ps(1) and procstat(1) can use it, as
> well as
> >> KB> procfs and linprocfs inside the kernel.
> >>
> >> Thanks! I am testing a patch (without auxv vector so far) and have some
> >> questions.
> >>
> >> Original ps -e returns environment only for user owned processes (the
> access is
> >> restricted by the permissions of /proc/pid/mem file). My kern.proc.env
> sysctl
> >> does not have such a restriction. I suppose I should add it? What
> function I
> >> could use for this?
> >>
> >> BTW, linprocfs allows to read other user's environment.
> KB> linprocfs uses p_cansee() to check the permissions. There are sysctls
> KB> security.bsd.see_other_{ug}ids that control the behaviour.
>
> KB> I believe that the new sysctl shall use the same check.
>
> >>
> >> KB> While you are at the code, it would be useful to also export the
> auxv vector,
> >> KB> which is immediately before env.
> >>
> >> It looks I can find the location of auxv but what about the size? Or do
> you
> >> propose to extend struct ps_strings to store location and size of auxv? I
> >> could do this way...
>
> KB> No, extending ps_strings is not needed and it is too radical change.
> KB> The auxv vector must end by the AT_NULL aux entry. You can also
> artificially
> KB> limit the amount of read aux vectors to, say, 256, which is much more
> then
> KB> it is currently defined.
>
> What do you think about the attached patch? This is a kernel
> part. COMPAT_FREEBSD32 has not been tested after the last update (just checked
> that it compiles): it looks I will not have access to amd64 box for testing
> during the weekend. I will test it after the weekend.
>
> Both kernel and userland parts are available here:
>
> http://people.freebsd.org/~trociny/env.sys.patch
> http://people.freebsd.org/~trociny/env.user.patch
>
> Currently there is an issue with procstat -x: if one tried to run it on 64 bit
> for a 32 bit process it would not detect this so would output a garbage. Could
> somebody recommend a way how to get this info about a process from userlend?
I think it is better to use sys/elf.h over the machine/elf.h.
Please change the comment for PROC_AUXV_MAX to "Safety limit on auxv size".
Also, it worth adding a comment saying that we are reading aux vectors twice,
first to get a size, second time to fetch a content, for simplicity.
When reading aux vector, if the PROC_AUXV_MAX entries are iterated over,
and we still not reached AT_NULL, the return error is 0. Was it intended ?
For PROC_ARG and PROC_ENV, you blindly trust the read values of the arg and
env vector sizes. This can easily cause kernel panics due to unability to
malloc the requested memory. I recommend to put some clump, and twice
of (PATH_MAX + ARG_MAX) is probably enough (see kern_exec.c, in particular,
exec_alloc_args). Also, you might use the swappable memory for the strings
as well, in the style of exec_alloc_args().
I suspect this is my bug: Reading the GET_PS_STRINGS_CHUNK_SZ may validly
return EFAULT if the string is shorter than the chunk and aligned at
the end of the page, assuming the next page is not mapped. There should
be a fallback to fubyte() read loop. I remember that copyinstr() was
unsuitable.
The checks for P_WEXIT in the linprocfs routines look strange. Since
you are unlocking the process right after the check, it does not make
sense. In fact, the checks are not needed, I believe, since pseudofs
already did the hold (see e.g. pfs_read and pfs_visible).
pgpyO8CZBR4it.pgp
Description: PGP signature
Re: "ps -e" without procfs(5)
On Mon, 31 Oct 2011 11:49:48 +0200 Kostik Belousov wrote: KB> On Sat, Oct 29, 2011 at 01:32:39PM +0300, Mikolaj Golub wrote: >> >> What do you think about the attached patch? This is a kernel >> part. COMPAT_FREEBSD32 has not been tested after the last update (just >> checked >> that it compiles): it looks I will not have access to amd64 box for testing >> during the weekend. I will test it after the weekend. >> >> Both kernel and userland parts are available here: >> >> http://people.freebsd.org/~trociny/env.sys.patch >> http://people.freebsd.org/~trociny/env.user.patch >> >> Currently there is an issue with procstat -x: if one tried to run it on 64 >> bit >> for a 32 bit process it would not detect this so would output a garbage. >> Could >> somebody recommend a way how to get this info about a process from userlend? KB> I think it is better to use sys/elf.h over the machine/elf.h. KB> Please change the comment for PROC_AUXV_MAX to "Safety limit on auxv size". KB> Also, it worth adding a comment saying that we are reading aux vectors twice, KB> first to get a size, second time to fetch a content, for simplicity. KB> When reading aux vector, if the PROC_AUXV_MAX entries are iterated over, KB> and we still not reached AT_NULL, the return error is 0. Was it intended ? According to kern_exec.c it is possible that a process doesn't have auxv at all. I don't know a way how to detect this. So because PROC_AUXV_MAX is much larger than expected amount of aux entries and we have not reached AT_NULL it is most likely the process doesn't have auxv and 0 length array (without error) is returned. If you think I should return a error in this situation, I can add this. Please tell me the error code I should return :-). Also, may be there is a sane way to check on auxv existence? KB> For PROC_ARG and PROC_ENV, you blindly trust the read values of the arg and KB> env vector sizes. This can easily cause kernel panics due to unability to KB> malloc the requested memory. I recommend to put some clump, and twice KB> of (PATH_MAX + ARG_MAX) is probably enough (see kern_exec.c, in particular, KB> exec_alloc_args). Also, you might use the swappable memory for the strings KB> as well, in the style of exec_alloc_args(). KB> I suspect this is my bug: Reading the GET_PS_STRINGS_CHUNK_SZ may validly KB> return EFAULT if the string is shorter than the chunk and aligned at KB> the end of the page, assuming the next page is not mapped. There should KB> be a fallback to fubyte() read loop. I remember that copyinstr() was KB> unsuitable. KB> The checks for P_WEXIT in the linprocfs routines look strange. Since KB> you are unlocking the process right after the check, it does not make KB> sense. In fact, the checks are not needed, I believe, since pseudofs KB> already did the hold (see e.g. pfs_read and pfs_visible). Ah, right. Unintentionally added when was adding the P_SYSTEM check. Thank you for all your comments. I will do this. -- Mikolaj Golub ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: "ps -e" without procfs(5)
On Mon, Oct 31, 2011 at 12:54:42PM +0200, Mikolaj Golub wrote: > > On Mon, 31 Oct 2011 11:49:48 +0200 Kostik Belousov wrote: > > KB> On Sat, Oct 29, 2011 at 01:32:39PM +0300, Mikolaj Golub wrote: > >> > >> What do you think about the attached patch? This is a kernel > >> part. COMPAT_FREEBSD32 has not been tested after the last update (just > checked > >> that it compiles): it looks I will not have access to amd64 box for > testing > >> during the weekend. I will test it after the weekend. > >> > >> Both kernel and userland parts are available here: > >> > >> http://people.freebsd.org/~trociny/env.sys.patch > >> http://people.freebsd.org/~trociny/env.user.patch > >> > >> Currently there is an issue with procstat -x: if one tried to run it on > 64 bit > >> for a 32 bit process it would not detect this so would output a garbage. > Could > >> somebody recommend a way how to get this info about a process from > userlend? > > KB> I think it is better to use sys/elf.h over the machine/elf.h. > > KB> Please change the comment for PROC_AUXV_MAX to "Safety limit on auxv > size". > KB> Also, it worth adding a comment saying that we are reading aux vectors > twice, > KB> first to get a size, second time to fetch a content, for simplicity. > > KB> When reading aux vector, if the PROC_AUXV_MAX entries are iterated over, > KB> and we still not reached AT_NULL, the return error is 0. Was it intended > ? > > According to kern_exec.c it is possible that a process doesn't have auxv at > all. I don't know a way how to detect this. So because PROC_AUXV_MAX is much > larger than expected amount of aux entries and we have not reached AT_NULL it > is most likely the process doesn't have auxv and 0 length array (without > error) is returned. > > If you think I should return a error in this situation, I can add this. Please > tell me the error code I should return :-). > > Also, may be there is a sane way to check on auxv existence? Hm. Even if the process ABI mandates the existence of auxv (i.e. the executing binary is ELF), the process could still cleared or mangled the corresponding stack page. I propose to designate some specific error code for the case of unparseable auxv region, ENOEXEC is not worse then any other. Also, please add the explanation you provided as a comment. pgpkaWejd97dy.pgp Description: PGP signature
Re: "ps -e" without procfs(5)
On Mon, 31 Oct 2011 11:49:48 +0200 Kostik Belousov wrote: KB> For PROC_ARG and PROC_ENV, you blindly trust the read values of the arg and KB> env vector sizes. This can easily cause kernel panics due to unability to KB> malloc the requested memory. I recommend to put some clump, and twice KB> of (PATH_MAX + ARG_MAX) is probably enough (see kern_exec.c, in particular, KB> exec_alloc_args). Also, you might use the swappable memory for the strings KB> as well, in the style of exec_alloc_args(). After looking at it more closely, I am not sure if I need to use exec_alloc_args. I malloc explicitly only for array vector (proc_vector). And actually it should be much smaller than 2 * (PATH_MAX + ARG_MAX). Currently in linprocfs the limit is 512 entries: #define MAX_ARGV_STR512 /* Max number of argv-like strings */ The same limit is in libkvm: /* * Check that there aren't an unreasonable number of arguments, * and that the address is in user space. Special test for * VM_MIN_ADDRESS as it evaluates to zero, but is not a simple zero * constant for some archs. We cannot use the pre-processor here and * for some archs the compiler would trigger a signedness warning. */ if (narg > 512 || addr + 1 < VM_MIN_ADDRESS + 1 || addr >= VM_MAXUSER_ADDRESS) return (0); (BTW, may be the VM_MIN_ADDRESS - VM_MAXUSER_ADDRESS is worth adding in my code too?) So it looks like I should use the same limit (512 * sizeof(char *)) for the allocated array. I could use exec_alloc_args() for the allocation but it would reqire some changes: I would have to free using kmem_free_wakeup(), which requires size of the region, while I return the number of entries. So I'd rather not use exec_alloc_args() for vector allocation because the benefit is not significant here. For strings I use sbuf and set it up using sbuf_new_for_sysctl. I could set it up manually as SBUF_FIXEDLEN allocating buf (up to 2 * (PATH_MAX + ARG_MAX)) with exec_alloc_args() but this would complicate things a little. Do you think it is worth doing? -- Mikolaj Golub ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: "ps -e" without procfs(5)
On Tue, Nov 01, 2011 at 09:07:11AM +0200, Mikolaj Golub wrote: > > On Mon, 31 Oct 2011 11:49:48 +0200 Kostik Belousov wrote: > > KB> For PROC_ARG and PROC_ENV, you blindly trust the read values of the arg > and > KB> env vector sizes. This can easily cause kernel panics due to unability to > KB> malloc the requested memory. I recommend to put some clump, and twice > KB> of (PATH_MAX + ARG_MAX) is probably enough (see kern_exec.c, in > particular, > KB> exec_alloc_args). Also, you might use the swappable memory for the > strings > KB> as well, in the style of exec_alloc_args(). > > After looking at it more closely, I am not sure if I need to use > exec_alloc_args. I malloc explicitly only for array vector (proc_vector). And > actually it should be much smaller than 2 * (PATH_MAX + ARG_MAX). Currently in > linprocfs the limit is 512 entries: > > #define MAX_ARGV_STR512 /* Max number of argv-like strings */ > > The same limit is in libkvm: > > /* > * Check that there aren't an unreasonable number of arguments, > * and that the address is in user space. Special test for > * VM_MIN_ADDRESS as it evaluates to zero, but is not a simple zero > * constant for some archs. We cannot use the pre-processor here and > * for some archs the compiler would trigger a signedness warning. > */ > if (narg > 512 || addr + 1 < VM_MIN_ADDRESS + 1 || addr >= > VM_MAXUSER_ADDRESS) > return (0); > > (BTW, may be the VM_MIN_ADDRESS - VM_MAXUSER_ADDRESS is worth adding in my > code too?) > > So it looks like I should use the same limit (512 * sizeof(char *)) for the > allocated array. I could use exec_alloc_args() for the allocation but it would > reqire some changes: I would have to free using kmem_free_wakeup(), which > requires size of the region, while I return the number of entries. So I'd > rather not use exec_alloc_args() for vector allocation because the benefit is > not significant here. > > For strings I use sbuf and set it up using sbuf_new_for_sysctl. I could set it > up manually as SBUF_FIXEDLEN allocating buf (up to 2 * (PATH_MAX + ARG_MAX)) > with exec_alloc_args() but this would complicate things a little. Do you think > it is worth doing? I mean using the swappable memory for strings, i.e. for the data you are currently store in sbuf. It indeed may be tricky, it was only an idea. pgp6emnWdCNQf.pgp Description: PGP signature
Re: "ps -e" without procfs(5)
On Mon, 31 Oct 2011 11:49:48 +0200 Kostik Belousov wrote:
KB> I think it is better to use sys/elf.h over the machine/elf.h.
KB> Please change the comment for PROC_AUXV_MAX to "Safety limit on auxv size".
KB> Also, it worth adding a comment saying that we are reading aux vectors
twice,
KB> first to get a size, second time to fetch a content, for simplicity.
KB> When reading aux vector, if the PROC_AUXV_MAX entries are iterated over,
KB> and we still not reached AT_NULL, the return error is 0. Was it intended ?
KB> For PROC_ARG and PROC_ENV, you blindly trust the read values of the arg and
KB> env vector sizes. This can easily cause kernel panics due to unability to
KB> malloc the requested memory. I recommend to put some clump, and twice
KB> of (PATH_MAX + ARG_MAX) is probably enough (see kern_exec.c, in particular,
KB> exec_alloc_args). Also, you might use the swappable memory for the strings
KB> as well, in the style of exec_alloc_args().
KB> I suspect this is my bug: Reading the GET_PS_STRINGS_CHUNK_SZ may validly
KB> return EFAULT if the string is shorter than the chunk and aligned at
KB> the end of the page, assuming the next page is not mapped. There should
KB> be a fallback to fubyte() read loop. I remember that copyinstr() was
KB> unsuitable.
KB> The checks for P_WEXIT in the linprocfs routines look strange. Since
KB> you are unlocking the process right after the check, it does not make
KB> sense. In fact, the checks are not needed, I believe, since pseudofs
KB> already did the hold (see e.g. pfs_read and pfs_visible).
Here is an updated version of the patch. Also available at
http://people.freebsd.org/~trociny/env.sys.1.patch
I decided to use the same constant (PROC_VECTOR_MAX) for limiting both the
number
of arg or env strings and the numbex of aux vectors.
Also I decided not to play with exec_alloc_args :-).
--
Mikolaj Golub
diff --git a/sys/sys/proc.h b/sys/sys/proc.h
index fb97913..4949f98 100644
--- a/sys/sys/proc.h
+++ b/sys/sys/proc.h
@@ -168,6 +168,7 @@ struct p_sched;
struct proc;
struct procdesc;
struct racct;
+struct sbuf;
struct sleepqueue;
struct td_sched;
struct thread;
@@ -843,6 +844,10 @@ int p_canwait(struct thread *td, struct proc *p);
struct pargs *pargs_alloc(int len);
void pargs_drop(struct pargs *pa);
void pargs_hold(struct pargs *pa);
+int proc_getargv(struct thread *td, struct proc *p, struct sbuf *sb,
+ size_t nchr);
+int proc_getenvv(struct thread *td, struct proc *p, struct sbuf *sb,
+ size_t nchr);
void procinit(void);
void proc_linkup0(struct proc *p, struct thread *td);
void proc_linkup(struct proc *p, struct thread *td);
diff --git a/sys/sys/sysctl.h b/sys/sys/sysctl.h
index 1e879f5..99ea342 100644
--- a/sys/sys/sysctl.h
+++ b/sys/sys/sysctl.h
@@ -559,6 +559,8 @@ SYSCTL_ALLOWED_TYPES(UINT64, uint64_t *a; unsigned long long *b; );
#define KERN_PROC_VMMAP 32 /* VM map entries for process */
#define KERN_PROC_FILEDESC 33 /* File descriptors for process */
#define KERN_PROC_GROUPS 34 /* process groups */
+#define KERN_PROC_ENV 35 /* get environment */
+#define KERN_PROC_AUXV 36 /* get ELF auxiliary vector */
/*
* KERN_IPC identifiers
diff --git a/sys/kern/kern_proc.c b/sys/kern/kern_proc.c
index 998e7ca..cc7c746 100644
--- a/sys/kern/kern_proc.c
+++ b/sys/kern/kern_proc.c
@@ -41,6 +41,8 @@ __FBSDID("$FreeBSD$");
#include
#include
+#include
+#include
#include
#include
#include
@@ -49,6 +51,7 @@ __FBSDID("$FreeBSD$");
#include
#include
#include
+#include
#include
#include
#include
@@ -75,6 +78,7 @@ __FBSDID("$FreeBSD$");
#include
#include
#include
+#include
#include
#ifdef COMPAT_FREEBSD32
@@ -1356,6 +1360,290 @@ pargs_drop(struct pargs *pa)
pargs_free(pa);
}
+static int
+proc_read_mem(struct thread *td, struct proc *p, vm_offset_t offset, void* buf,
+size_t len)
+{
+ struct iovec iov;
+ struct uio uio;
+
+ iov.iov_base = (caddr_t)buf;
+ iov.iov_len = len;
+ uio.uio_iov = &iov;
+ uio.uio_iovcnt = 1;
+ uio.uio_offset = offset;
+ uio.uio_resid = (ssize_t)len;
+ uio.uio_segflg = UIO_SYSSPACE;
+ uio.uio_rw = UIO_READ;
+ uio.uio_td = td;
+
+ return (proc_rwmem(p, &uio));
+}
+
+static int
+proc_read_string(struct thread *td, struct proc *p, const char *sptr, char *buf,
+size_t len)
+{
+ size_t i;
+ int error, c;
+
+ error = proc_read_mem(td, p, (vm_offset_t)sptr, buf, len);
+ /*
+ * Reading the chunk may validly return EFAULT if the string is shorter
+ * than the chunk and is aligned at the end of the page, assuming the
+ * next page is not mapped. So if EFAULT is returned do a fallback to
+ * fubyte() read loop.
+ */
+ if (error == EFAULT) {
+ for (i = 0; i < len; i++) {
+ c = fubyte(sptr + i);
+ if (c < 0)
+return (EFAULT);
+ buf[i] = (char)c;
+ if (c == '\0')
+break;
+ }
+ error = 0;
+ }
+ return error;
+}
+
+#define PROC_VECTOR_MAX 512 /* Safety limit on argv-like vector size. */
+
+enum proc_vector_type {
+ PROC_ARG,
+ PROC_ENV,
+
Re: "ps -e" without procfs(5)
On Mon, 31 Oct 2011 11:49:48 +0200 Kostik Belousov wrote:
KB> I suspect this is my bug: Reading the GET_PS_STRINGS_CHUNK_SZ may validly
KB> return EFAULT if the string is shorter than the chunk and aligned at
KB> the end of the page, assuming the next page is not mapped. There should
KB> be a fallback to fubyte() read loop. I remember that copyinstr() was
KB> unsuitable.
Hm, I thought that this issue was only for reading arg and env strings (which
could be shorter than GET_PS_STRINGS_CHUNK_SZ), but investigating the cases
when EFAULT was returned in my tests (running buildworld and procstat in loop)
I saw that it also returned when reading other objects (like struct
ps_strings), and a fallback to fubyte() read loop was successful in those
cases too.
So I updated the patch to do fallback for any type of read (although it does
not contain a good comment explaining why fubyte() read might succeed when
proc_rwmem() failed).
Also there were the cases when EFAULT was returned because arg vector
contained the NULL pointer. I observed this for sh processes. In
lib/libc/gen/setproctitle.c I found this comment:
oargc = ps_strings->ps_nargvstr;
oargv = ps_strings->ps_argvstr;
for (i = len = 0; i < oargc; i++) {
/*
* The program may have scribbled into its
* argv array, e.g., to remove some arguments.
* If that has happened, break out before
* trying to call strlen on a NULL pointer.
*/
if (oargv[i] == NULL) {
oargc = i;
break;
}
I have updated my patch to do the same.
Running buildworld test after these changes I have observed EFAULT only once,
for cc process, when argv contained a pointer to 0x40.
Also, for kern.proc.args some times errors like below are observed:
procstat: sysctl: kern.proc.args: 58002: 8: Exec format error
And for kern.proc.env:
procstat: sysctl: kern.proc.env: 81352: 16: Device busy
But I have not investigated these cases yet.
The update version:
http://people.freebsd.org/~trociny/env.sys.2.patch
--
Mikolaj Golub
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: "ps -e" without procfs(5)
On Wed, Nov 02, 2011 at 11:27:37PM +0200, Mikolaj Golub wrote:
>
> On Mon, 31 Oct 2011 11:49:48 +0200 Kostik Belousov wrote:
>
> KB> I think it is better to use sys/elf.h over the machine/elf.h.
>
> KB> Please change the comment for PROC_AUXV_MAX to "Safety limit on
> KB> auxv size". Also, it worth adding a comment saying that we are
> KB> reading aux vectors twice, first to get a size, second time to
> KB> fetch a content, for simplicity.
>
> KB> When reading aux vector, if the PROC_AUXV_MAX entries are
> KB> iterated over, and we still not reached AT_NULL, the return error
> KB> is 0. Was it intended ?
>
> KB> For PROC_ARG and PROC_ENV, you blindly trust the read values of
> KB> the arg and env vector sizes. This can easily cause kernel panics
> KB> due to unability to malloc the requested memory. I recommend to
> KB> put some clump, and twice of (PATH_MAX + ARG_MAX) is probably
> KB> enough (see kern_exec.c, in particular, exec_alloc_args). Also,
> KB> you might use the swappable memory for the strings as well, in
> KB> the style of exec_alloc_args().
>
> KB> I suspect this is my bug: Reading the GET_PS_STRINGS_CHUNK_SZ may
> KB> validly return EFAULT if the string is shorter than the chunk
> KB> and aligned at the end of the page, assuming the next page is
> KB> not mapped. There should be a fallback to fubyte() read loop. I
> KB> remember that copyinstr() was unsuitable.
>
> KB> The checks for P_WEXIT in the linprocfs routines look strange.
> KB> Since you are unlocking the process right after the check, it
> KB> does not make sense. In fact, the checks are not needed, I
> KB> believe, since pseudofs already did the hold (see e.g. pfs_read
> KB> and pfs_visible).
>
> Here is an updated version of the patch. Also available at
>
> http://people.freebsd.org/~trociny/env.sys.1.patch
>
> I decided to use the same constant (PROC_VECTOR_MAX) for limiting both
> the number of arg or env strings and the numbex of aux vectors.
>
> Also I decided not to play with exec_alloc_args :-).
+ if (error == EFAULT) {
+ for (i = 0; i < len; i++) {
+ c = fubyte(sptr + i);
+ if (c < 0)
As a purely stylistical issue, compare with -1.
+ return (EFAULT);
+ buf[i] = (char)c;
+ if (c == '\0')
+ break;
+ }
+ error = 0;
+ }
+ return error;
Put () around error.
+ /*
+* Check that that the address is in user space.
+*/
+ if (vptr + 1 < VM_MIN_ADDRESS + 1 || vptr >= VM_MAXUSER_ADDRESS)
+ return (ENOEXEC);
Why is this needed ?
I think that the aux vector must be naturally aligned. You can return
ENOEXEC early if vptr is not aligned.
Why the blank after the loop statement in get_ps_strings() ?
There shall be blank lines after the '{' in proc_getargv() and proc_getenvv().
Note that using cached pargs is somewhat inconsistent with the digging
into ps_strings.
procfs_doproccmdline() can benefit from your work.
pgpBc5VnV0yxI.pgp
Description: PGP signature
Re: "ps -e" without procfs(5)
On Sat, 5 Nov 2011 15:58:01 +0200 Kostik Belousov wrote:
KB> +if (error == EFAULT) {
KB> +for (i = 0; i < len; i++) {
KB> +c = fubyte(sptr + i);
KB> +if (c < 0)
KB> As a purely stylistical issue, compare with -1.
KB> +return (EFAULT);
KB> +buf[i] = (char)c;
KB> +if (c == '\0')
KB> +break;
KB> +}
KB> +error = 0;
KB> +}
KB> +return error;
KB> Put () around error.
Thanks.
KB> +/*
KB> + * Check that that the address is in user space.
KB> + */
KB> +if (vptr + 1 < VM_MIN_ADDRESS + 1 || vptr >= VM_MAXUSER_ADDRESS)
KB> +return (ENOEXEC);
KB> Why is this needed ?
I saw this check in libkvm for ps_argvstr and ps_envstr and decided to add it
too. Just some additional check that ps_string fields, which can be
overwritten by the user, look reasonable. If you think this is not very useful
I remove it.
KB> I think that the aux vector must be naturally aligned. You can return
KB> ENOEXEC early if vptr is not aligned.
Not sure I see what you mean. vptr for auxv is calculated just couple lines
above, and I check the result here, in the part common for all vector types.
BTW, investigating the cases when I got
procstat: sysctl: kern.proc.args: 58002: 8: Exec format error
they were because the PROC_VECTOR_MAX limit (512 entries, as it is in
linprocfs and libkvm) is small for real world cases:
get_proc_vector(pid = rm[47883], type = 0): vsize (3009) > PROC_VECTOR_MAX
(512))
get_proc_vector(pid = rm[47883], type = 0): vsize (3009) > PROC_VECTOR_MAX
(512))
get_proc_vector(pid = rm[47890], type = 0): vsize (3008) > PROC_VECTOR_MAX
(512))
get_proc_vector(pid = rm[47890], type = 0): vsize (3008) > PROC_VECTOR_MAX
(512))
get_proc_vector(pid = rm[47897], type = 0): vsize (4511) > PROC_VECTOR_MAX
(512))
get_proc_vector(pid = rm[47897], type = 0): vsize (4511) > PROC_VECTOR_MAX
(512))
get_proc_vector(pid = rm[47897], type = 0): vsize (4511) > PROC_VECTOR_MAX
(512))
get_proc_vector(pid = rm[48044], type = 0): vsize (611) > PROC_VECTOR_MAX (512))
get_proc_vector(pid = rm[52189], type = 0): vsize (772) > PROC_VECTOR_MAX (512))
get_proc_vector(pid = rm[52192], type = 0): vsize (1157) > PROC_VECTOR_MAX
(512))
get_proc_vector(pid = rm[55685], type = 0): vsize (1041) > PROC_VECTOR_MAX
(512))
get_proc_vector(pid = rm[55687], type = 0): vsize (1040) > PROC_VECTOR_MAX
(512))
get_proc_vector(pid = rm[55690], type = 0): vsize (1559) > PROC_VECTOR_MAX
(512))
So I am going to change it to ARG_MAX and use independent limit (256 entries)
for auxv.
KB> Why the blank after the loop statement in get_ps_strings() ?
Sorry, what blank you mean? I don't see it in get_ps_strings(). May be you
mean the blank line in get_proc_vector() before return?
KB> There shall be blank lines after the '{' in proc_getargv() and
proc_getenvv().
Ah, sure.
KB> Note that using cached pargs is somewhat inconsistent with the digging
KB> into ps_strings.
KB> procfs_doproccmdline() can benefit from your work.
Thanks, I will look at it :-).
--
Mikolaj Golub
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: "ps -e" without procfs(5)
On Sat, Nov 05, 2011 at 05:40:22PM +0200, Mikolaj Golub wrote:
>
> On Sat, 5 Nov 2011 15:58:01 +0200 Kostik Belousov wrote:
>
> KB> +if (error == EFAULT) {
> KB> +for (i = 0; i < len; i++) {
> KB> +c = fubyte(sptr + i);
> KB> +if (c < 0)
> KB> As a purely stylistical issue, compare with -1.
>
> KB> +return (EFAULT);
> KB> +buf[i] = (char)c;
> KB> +if (c == '\0')
> KB> +break;
> KB> +}
> KB> +error = 0;
> KB> +}
> KB> +return error;
> KB> Put () around error.
>
> Thanks.
>
> KB> +/*
> KB> + * Check that that the address is in user space.
> KB> + */
> KB> +if (vptr + 1 < VM_MIN_ADDRESS + 1 || vptr >= VM_MAXUSER_ADDRESS)
> KB> +return (ENOEXEC);
> KB> Why is this needed ?
>
> I saw this check in libkvm for ps_argvstr and ps_envstr and decided to add it
> too. Just some additional check that ps_string fields, which can be
> overwritten by the user, look reasonable. If you think this is not very useful
> I remove it.
The proc_rwmem() must handle access outside the user VA (and it does).
>
> KB> I think that the aux vector must be naturally aligned. You can return
> KB> ENOEXEC early if vptr is not aligned.
>
> Not sure I see what you mean. vptr for auxv is calculated just couple lines
> above, and I check the result here, in the part common for all vector types.
You do not check for the alignment. Am I wrong ?
>
> BTW, investigating the cases when I got
>
> procstat: sysctl: kern.proc.args: 58002: 8: Exec format error
>
> they were because the PROC_VECTOR_MAX limit (512 entries, as it is in
> linprocfs and libkvm) is small for real world cases:
>
> get_proc_vector(pid = rm[47883], type = 0): vsize (3009) > PROC_VECTOR_MAX
> (512))
> get_proc_vector(pid = rm[47883], type = 0): vsize (3009) > PROC_VECTOR_MAX
> (512))
> get_proc_vector(pid = rm[47890], type = 0): vsize (3008) > PROC_VECTOR_MAX
> (512))
> get_proc_vector(pid = rm[47890], type = 0): vsize (3008) > PROC_VECTOR_MAX
> (512))
> get_proc_vector(pid = rm[47897], type = 0): vsize (4511) > PROC_VECTOR_MAX
> (512))
> get_proc_vector(pid = rm[47897], type = 0): vsize (4511) > PROC_VECTOR_MAX
> (512))
> get_proc_vector(pid = rm[47897], type = 0): vsize (4511) > PROC_VECTOR_MAX
> (512))
> get_proc_vector(pid = rm[48044], type = 0): vsize (611) > PROC_VECTOR_MAX
> (512))
> get_proc_vector(pid = rm[52189], type = 0): vsize (772) > PROC_VECTOR_MAX
> (512))
> get_proc_vector(pid = rm[52192], type = 0): vsize (1157) > PROC_VECTOR_MAX
> (512))
> get_proc_vector(pid = rm[55685], type = 0): vsize (1041) > PROC_VECTOR_MAX
> (512))
> get_proc_vector(pid = rm[55687], type = 0): vsize (1040) > PROC_VECTOR_MAX
> (512))
> get_proc_vector(pid = rm[55690], type = 0): vsize (1559) > PROC_VECTOR_MAX
> (512))
>
> So I am going to change it to ARG_MAX and use independent limit (256 entries)
> for auxv.
Ok.
>
> KB> Why the blank after the loop statement in get_ps_strings() ?
>
> Sorry, what blank you mean? I don't see it in get_ps_strings(). May be you
> mean the blank line in get_proc_vector() before return?
+ for (sptr = proc_vector[i]; ; sptr += GET_PS_STRINGS_CHUNK_SZ) {
+
+ error = proc_read_mem(td, p, (vm_offset_t)sptr,
The line between for() and error = .
>
> KB> There shall be blank lines after the '{' in proc_getargv() and
> proc_getenvv().
>
> Ah, sure.
>
> KB> Note that using cached pargs is somewhat inconsistent with the digging
> KB> into ps_strings.
>
> KB> procfs_doproccmdline() can benefit from your work.
>
> Thanks, I will look at it :-).
>
> --
> Mikolaj Golub
pgpD3ZL2Z2fRc.pgp
Description: PGP signature
Re: "ps -e" without procfs(5)
On Sat, 5 Nov 2011 17:44:43 +0200 Kostik Belousov wrote: >> KB> I think that the aux vector must be naturally aligned. You can return >> KB> ENOEXEC early if vptr is not aligned. >> >> Not sure I see what you mean. vptr for auxv is calculated just couple lines >> above, and I check the result here, in the part common for all vector types. KB> You do not check for the alignment. Am I wrong ? I see now. If natural alignment means "addr % sizeof(aux) == 0" then the aux vectors are not naturally aligned. After adding this check: if (vptr % sizeof(aux) != 0) return (ENOEXEC); I started to observe many ENOEXEC errors. Adding printf showed that the vectors are half size aligned. On i386: get_proc_vector(pid = getty[3442], type = 2): vptr (2143284876) % sizeof(aux) (8) = 4) On amd64: get_proc_vector(pid = getty[2425], type = 2): vptr (140737488346568) % sizeof(aux) (16) = 8) Looking at exec_copyout_strings() from kern_exec.c, how destp is calculated, I think they are sizeof(char *) aligned. Do you think it is worth adding the check for sizeof(char *) alignment? if (vptr % (sizeof(char *) != 0) return (ENOEXEC); -- Mikolaj Golub ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: "ps -e" without procfs(5)
On Sat, Nov 05, 2011 at 08:59:21PM +0200, Mikolaj Golub wrote: > > On Sat, 5 Nov 2011 17:44:43 +0200 Kostik Belousov wrote: > > >> KB> I think that the aux vector must be naturally aligned. You can return > >> KB> ENOEXEC early if vptr is not aligned. > >> > >> Not sure I see what you mean. vptr for auxv is calculated just couple > lines > >> above, and I check the result here, in the part common for all vector > types. > KB> You do not check for the alignment. Am I wrong ? > > I see now. If natural alignment means "addr % sizeof(aux) == 0" then the aux > vectors are not naturally aligned. After adding this check: > > if (vptr % sizeof(aux) != 0) > return (ENOEXEC); No, the natural alignment of the structure is the alignment of the most demanding member. So it is 4 bytes on 32bit, and 8 bytes on 64. > > I started to observe many ENOEXEC errors. Adding printf showed that the > vectors are half size aligned. > > On i386: > > get_proc_vector(pid = getty[3442], type = 2): vptr (2143284876) % sizeof(aux) > (8) = 4) > > On amd64: > > get_proc_vector(pid = getty[2425], type = 2): vptr (140737488346568) % > sizeof(aux) (16) = 8) > > Looking at exec_copyout_strings() from kern_exec.c, how destp is calculated, I > think they are sizeof(char *) aligned. > > Do you think it is worth adding the check for sizeof(char *) alignment? > > if (vptr % (sizeof(char *) != 0) > return (ENOEXEC); I suggest to use #if __ELF_WORD_SIZE == 32 or 64. pgp0LGCE3rUZi.pgp Description: PGP signature
Re: "ps -e" without procfs(5)
On Sat, 5 Nov 2011 21:45:53 +0200 Kostik Belousov wrote: KB> On Sat, Nov 05, 2011 at 08:59:21PM +0200, Mikolaj Golub wrote: >> >> On Sat, 5 Nov 2011 17:44:43 +0200 Kostik Belousov wrote: >> >> >> KB> I think that the aux vector must be naturally aligned. You can >> return >> >> KB> ENOEXEC early if vptr is not aligned. >> >> >> >> Not sure I see what you mean. vptr for auxv is calculated just couple >> lines >> >> above, and I check the result here, in the part common for all vector >> types. >> KB> You do not check for the alignment. Am I wrong ? >> >> I see now. If natural alignment means "addr % sizeof(aux) == 0" then the aux >> vectors are not naturally aligned. After adding this check: >> >> if (vptr % sizeof(aux) != 0) >> return (ENOEXEC); KB> No, the natural alignment of the structure is the alignment of the most KB> demanding member. So it is 4 bytes on 32bit, and 8 bytes on 64. >> >> I started to observe many ENOEXEC errors. Adding printf showed that the >> vectors are half size aligned. >> >> On i386: >> >> get_proc_vector(pid = getty[3442], type = 2): vptr (2143284876) % >> sizeof(aux) (8) = 4) >> >> On amd64: >> >> get_proc_vector(pid = getty[2425], type = 2): vptr (140737488346568) % >> sizeof(aux) (16) = 8) >> >> Looking at exec_copyout_strings() from kern_exec.c, how destp is >> calculated, I >> think they are sizeof(char *) aligned. >> >> Do you think it is worth adding the check for sizeof(char *) alignment? >> >> if (vptr % (sizeof(char *) != 0) >> return (ENOEXEC); KB> I suggest to use #if __ELF_WORD_SIZE == 32 or 64. Thanks. The updated patch: http://people.freebsd.org/~trociny/env.sys.3.patch -- Mikolaj Golub ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: "ps -e" without procfs(5)
On Sat, 5 Nov 2011 15:58:01 +0200 Kostik Belousov wrote: KB> procfs_doproccmdline() can benefit from your work. Patch for procfs: http://people.freebsd.org/~trociny/procfs.patch -- Mikolaj Golub ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: "ps -e" without procfs(5)
On Sat, Nov 05, 2011 at 10:37:46PM +0200, Mikolaj Golub wrote: > > On Sat, 5 Nov 2011 21:45:53 +0200 Kostik Belousov wrote: > > KB> On Sat, Nov 05, 2011 at 08:59:21PM +0200, Mikolaj Golub wrote: > >> > >> On Sat, 5 Nov 2011 17:44:43 +0200 Kostik Belousov wrote: > >> > >> >> KB> I think that the aux vector must be naturally aligned. You can > return > >> >> KB> ENOEXEC early if vptr is not aligned. > >> >> > >> >> Not sure I see what you mean. vptr for auxv is calculated just couple > lines > >> >> above, and I check the result here, in the part common for all vector > types. > >> KB> You do not check for the alignment. Am I wrong ? > >> > >> I see now. If natural alignment means "addr % sizeof(aux) == 0" then the > aux > >> vectors are not naturally aligned. After adding this check: > >> > >> if (vptr % sizeof(aux) != 0) > >> return (ENOEXEC); > KB> No, the natural alignment of the structure is the alignment of the most > KB> demanding member. So it is 4 bytes on 32bit, and 8 bytes on 64. > > >> > >> I started to observe many ENOEXEC errors. Adding printf showed that the > >> vectors are half size aligned. > >> > >> On i386: > >> > >> get_proc_vector(pid = getty[3442], type = 2): vptr (2143284876) % > sizeof(aux) (8) = 4) > >> > >> On amd64: > >> > >> get_proc_vector(pid = getty[2425], type = 2): vptr (140737488346568) % > sizeof(aux) (16) = 8) > >> > >> Looking at exec_copyout_strings() from kern_exec.c, how destp is > calculated, I > >> think they are sizeof(char *) aligned. > >> > >> Do you think it is worth adding the check for sizeof(char *) alignment? > >> > >> if (vptr % (sizeof(char *) != 0) > >> return (ENOEXEC); > KB> I suggest to use #if __ELF_WORD_SIZE == 32 or 64. > > Thanks. The updated patch: > > http://people.freebsd.org/~trociny/env.sys.3.patch Oops, I missed this in the previous review. You cannot use fubyte in proc_read_mem(). fubyte reads a byte from the address space of the current process. The fix is easy, use proc_rwmem for 1 byte. I do not think that fall back to single byte read is warranted for proc_read_mem calls e.g. for ps_strings. Add a flag to indicate whether the proc_read_mem should fall back to byte read ? I would prefer using sizeof(uint64_t) and sizeof(uint32_t) instead of 8 and 4 constants in the align checks. Might be, add PROC_ASSERT_HELD() to get_ps_string() ? procfs patch looks good. pgp9ZRbUSX5B9.pgp Description: PGP signature
Re: "ps -e" without procfs(5)
On Sun, 6 Nov 2011 20:10:41 +0200 Kostik Belousov wrote: KB> On Sat, Nov 05, 2011 at 10:37:46PM +0200, Mikolaj Golub wrote: >> >> http://people.freebsd.org/~trociny/env.sys.3.patch KB> Oops, I missed this in the previous review. You cannot use fubyte in KB> proc_read_mem(). fubyte reads a byte from the address space of the current KB> process. The fix is easy, use proc_rwmem for 1 byte. KB> I do not think that fall back to single byte read is warranted for KB> proc_read_mem calls e.g. for ps_strings. Add a flag to indicate whether KB> the proc_read_mem should fall back to byte read ? KB> I would prefer using sizeof(uint64_t) and sizeof(uint32_t) instead of 8 KB> and 4 constants in the align checks. KB> Might be, add PROC_ASSERT_HELD() to get_ps_string() ? KB> procfs patch looks good. Thanks. The updated version: http://people.freebsd.org/~trociny/env.sys.4.patch Investigating cases when EFAULT was returned and if the fallback was successful I noticed that most of the cases were when p->p_comm changed during the read, so the process was in exec in that time. In order to avoid this error I added a check for P_INEXEC flag. After this I observed EFAULT (very rarely) only when reading arg or env strings and fallback was successful for those cases. So I modified the patch to do fallback only when reading strings (as it was in one of my earlier versions but with wrong fubyte), and returned your comment which explains why it may happen :-) Also in the procfs patch I have added the check for process state. The userland part has not been changed since my first report: http://people.freebsd.org/~trociny/env.user.patch -- Mikolaj Golub ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: "ps -e" without procfs(5)
On Tue, Nov 08, 2011 at 11:47:54PM +0200, Mikolaj Golub wrote: > > On Sun, 6 Nov 2011 20:10:41 +0200 Kostik Belousov wrote: > > KB> On Sat, Nov 05, 2011 at 10:37:46PM +0200, Mikolaj Golub wrote: > >> > >> http://people.freebsd.org/~trociny/env.sys.3.patch > > KB> Oops, I missed this in the previous review. You cannot use fubyte in > KB> proc_read_mem(). fubyte reads a byte from the address space of the > current > KB> process. The fix is easy, use proc_rwmem for 1 byte. > > KB> I do not think that fall back to single byte read is warranted for > KB> proc_read_mem calls e.g. for ps_strings. Add a flag to indicate whether > KB> the proc_read_mem should fall back to byte read ? > > KB> I would prefer using sizeof(uint64_t) and sizeof(uint32_t) instead of 8 > KB> and 4 constants in the align checks. > > KB> Might be, add PROC_ASSERT_HELD() to get_ps_string() ? > > KB> procfs patch looks good. > > Thanks. The updated version: > > http://people.freebsd.org/~trociny/env.sys.4.patch > > Investigating cases when EFAULT was returned and if the fallback was > successful I noticed that most of the cases were when p->p_comm changed during > the read, so the process was in exec in that time. In order to avoid this > error I added a check for P_INEXEC flag. And now you return success and nothing gets copied out for the process in P_INEXEC state. Either you should return an error like EAGAIN, or consider the P_INEXEC state as transitional and wait till process leaves it. Or, ignore the state as it was before, and return whatever error proc_rwmem generated (my preference). > > After this I observed EFAULT (very rarely) only when reading arg or env > strings and fallback was successful for those cases. So I modified the patch > to do fallback only when reading strings (as it was in one of my earlier > versions but with wrong fubyte), and returned your comment which explains why > it may happen :-) > > Also in the procfs patch I have added the check for process state. > > The userland part has not been changed since my first report: > > http://people.freebsd.org/~trociny/env.user.patch > > -- > Mikolaj Golub pgpLo6P15pt0f.pgp Description: PGP signature
Re: "ps -e" without procfs(5)
On Wed, Nov 09, 2011 at 02:44:55PM +0200, Kostik Belousov wrote: > On Tue, Nov 08, 2011 at 11:47:54PM +0200, Mikolaj Golub wrote: > > > > On Sun, 6 Nov 2011 20:10:41 +0200 Kostik Belousov wrote: > > > > KB> On Sat, Nov 05, 2011 at 10:37:46PM +0200, Mikolaj Golub wrote: > > >> > > >> http://people.freebsd.org/~trociny/env.sys.3.patch > > > > KB> Oops, I missed this in the previous review. You cannot use fubyte in > > KB> proc_read_mem(). fubyte reads a byte from the address space of the > > current > > KB> process. The fix is easy, use proc_rwmem for 1 byte. > > > > KB> I do not think that fall back to single byte read is warranted for > > KB> proc_read_mem calls e.g. for ps_strings. Add a flag to indicate whether > > KB> the proc_read_mem should fall back to byte read ? > > > > KB> I would prefer using sizeof(uint64_t) and sizeof(uint32_t) instead of 8 > > KB> and 4 constants in the align checks. > > > > KB> Might be, add PROC_ASSERT_HELD() to get_ps_string() ? > > > > KB> procfs patch looks good. > > > > Thanks. The updated version: > > > > http://people.freebsd.org/~trociny/env.sys.4.patch > > > > Investigating cases when EFAULT was returned and if the fallback was > > successful I noticed that most of the cases were when p->p_comm changed > > during > > the read, so the process was in exec in that time. In order to avoid this > > error I added a check for P_INEXEC flag. > And now you return success and nothing gets copied out for the process > in P_INEXEC state. Either you should return an error like EAGAIN, or > consider the P_INEXEC state as transitional and wait till process > leaves it. Or, ignore the state as it was before, and return whatever > error proc_rwmem generated (my preference). Forgot to say that the check does not change much because you drop process lock immediately after the check, so the process may enter the INEXEC state right after the check. I believe you already tried to do this with P_WEXIT. > > > > > After this I observed EFAULT (very rarely) only when reading arg or env > > strings and fallback was successful for those cases. So I modified the patch > > to do fallback only when reading strings (as it was in one of my earlier > > versions but with wrong fubyte), and returned your comment which explains > > why > > it may happen :-) > > > > Also in the procfs patch I have added the check for process state. > > > > The userland part has not been changed since my first report: > > > > http://people.freebsd.org/~trociny/env.user.patch > > > > -- > > Mikolaj Golub pgpArht5hTAqy.pgp Description: PGP signature
Re: "ps -e" without procfs(5)
On Wed, 9 Nov 2011 14:44:55 +0200 Kostik Belousov wrote: KB> On Tue, Nov 08, 2011 at 11:47:54PM +0200, Mikolaj Golub wrote: >> >> http://people.freebsd.org/~trociny/env.sys.4.patch >> >> Investigating cases when EFAULT was returned and if the fallback was >> successful I noticed that most of the cases were when p->p_comm changed >> during >> the read, so the process was in exec in that time. In order to avoid this >> error I added a check for P_INEXEC flag. KB> And now you return success and nothing gets copied out for the process KB> in P_INEXEC state. This looked ok for me: new arguments have not been created for the process yet, so return nothing :-) KB> Either you should return an error like EAGAIN, or consider the P_INEXEC KB> state as transitional and wait till process leaves it. Or, ignore the KB> state as it was before, and return whatever error proc_rwmem generated KB> (my preference). I prefer EAGAIN :-). Reading in the process space that is being currrently updated does not look good for me. And EAGAIN gives a hint that if I try it again I will probably get the result, while EFAULT looks mysterious. -- Mikolaj Golub ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: "ps -e" without procfs(5)
On Wed, 9 Nov 2011 14:53:29 +0200 Kostik Belousov wrote: >> And now you return success and nothing gets copied out for the process >> in P_INEXEC state. Either you should return an error like EAGAIN, or >> consider the P_INEXEC state as transitional and wait till process >> leaves it. Or, ignore the state as it was before, and return whatever >> error proc_rwmem generated (my preference). KB> Forgot to say that the check does not change much because you drop KB> process lock immediately after the check, so the process may enter KB> the INEXEC state right after the check. I believe you already tried KB> to do this with P_WEXIT. Good point :-). Although after adding the P_INEXEC I have not seen errors any more, while before they were often (when running 'procstat -ca' in loop and building world simultaneously). Thus it looks like the probability is much smaller. So, it still looks good for me to check for P_INEXEC and return EAGAIN, and add the comment why we do this and that it still racy. But if you still think that ignoring the state is the best option no problems for me to return it back. -- Mikolaj Golub ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: "ps -e" without procfs(5)
On Wed, 09 Nov 2011 15:31:26 +0200 Mikolaj Golub wrote:
MG> On Wed, 9 Nov 2011 14:53:29 +0200 Kostik Belousov wrote:
>>> And now you return success and nothing gets copied out for the process
>>> in P_INEXEC state. Either you should return an error like EAGAIN, or
>>> consider the P_INEXEC state as transitional and wait till process
>>> leaves it. Or, ignore the state as it was before, and return whatever
>>> error proc_rwmem generated (my preference).
KB>> Forgot to say that the check does not change much because you drop
KB>> process lock immediately after the check, so the process may enter
KB>> the INEXEC state right after the check. I believe you already tried
KB>> to do this with P_WEXIT.
MG> Good point :-). Although after adding the P_INEXEC I have not seen errors
any
MG> more, while before they were often (when running 'procstat -ca' in loop and
MG> building world simultaneously). Thus it looks like the probability is much
MG> smaller.
MG> So, it still looks good for me to check for P_INEXEC and return EAGAIN, and
MG> add the comment why we do this and that it still racy. But if you still
think
MG> that ignoring the state is the best option no problems for me to return it
MG> back.
Realted to this, sysctl_kern_proc_kstack() looks like has the similar issue.
But it returns ESRCH instead.
/* XXXRW: Not clear ESRCH is the right error during proc execve(). */
if (p->p_flag & P_WEXIT || p->p_flag & P_INEXEC) {
PROC_UNLOCK(p);
return (ESRCH);
}
...
_PHOLD(p);
PROC_UNLOCK(p);
--
Mikolaj Golub
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: "ps -e" without procfs(5)
On Wed, 9 Nov 2011 14:53:29 +0200 Kostik Belousov wrote: >> > http://people.freebsd.org/~trociny/env.sys.4.patch >> > >> > Investigating cases when EFAULT was returned and if the fallback was >> > successful I noticed that most of the cases were when p->p_comm changed >> > during >> > the read, so the process was in exec in that time. In order to avoid this >> > error I added a check for P_INEXEC flag. >> And now you return success and nothing gets copied out for the process >> in P_INEXEC state. Either you should return an error like EAGAIN, or >> consider the P_INEXEC state as transitional and wait till process >> leaves it. Or, ignore the state as it was before, and return whatever >> error proc_rwmem generated (my preference). KB> Forgot to say that the check does not change much because you drop KB> process lock immediately after the check, so the process may enter KB> the INEXEC state right after the check. I believe you already tried KB> to do this with P_WEXIT. Ok, eventually I decided not to check for P_INEXEC (as the simplest :-). The updated patch: http://people.freebsd.org/~trociny/env.sys.5.patch -- Mikolaj Golub ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: "ps -e" without procfs(5)
On Sat, Oct 29, 2011 at 01:32:39PM +0300, Mikolaj Golub wrote: > [KERN_PROC_AUXV requires just p_cansee()] If we are ever going to do ASLR, the AUXV information tells an attacker where the stack, executable and RTLD are located, which defeats much of the point of randomizing the addresses in the first place. Given that the AUXV information seems to be used by debuggers only anyway, I think it would be good to move it to p_candebug() now. The full virtual memory maps (KERN_PROC_VMMAP, procstat -v) are already under p_candebug(). -- Jilles Tjoelker ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: "ps -e" without procfs(5)
On 4 Dec 2011, at 14:31, Jilles Tjoelker wrote: > On Sat, Oct 29, 2011 at 01:32:39PM +0300, Mikolaj Golub wrote: >> [KERN_PROC_AUXV requires just p_cansee()] > > If we are ever going to do ASLR, the AUXV information tells an attacker > where the stack, executable and RTLD are located, which defeats much of > the point of randomizing the addresses in the first place. > > Given that the AUXV information seems to be used by debuggers only > anyway, I think it would be good to move it to p_candebug() now. > > The full virtual memory maps (KERN_PROC_VMMAP, procstat -v) are already > under p_candebug(). Agreed. In general, my view is that p_cansee() should be used for very few of our process inspection APIs. I like your example of ASLR especially, as it illustrates how debugging information can aid even local attacks (i.e., user vs. setuid binary). Robert___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: "ps -e" without procfs(5)
On Sun, 4 Dec 2011 15:57:06 + Robert N. M. Watson wrote: RNMW> On 4 Dec 2011, at 14:31, Jilles Tjoelker wrote: >> On Sat, Oct 29, 2011 at 01:32:39PM +0300, Mikolaj Golub wrote: >>> [KERN_PROC_AUXV requires just p_cansee()] >> >> If we are ever going to do ASLR, the AUXV information tells an attacker >> where the stack, executable and RTLD are located, which defeats much of >> the point of randomizing the addresses in the first place. >> >> Given that the AUXV information seems to be used by debuggers only >> anyway, I think it would be good to move it to p_candebug() now. >> >> The full virtual memory maps (KERN_PROC_VMMAP, procstat -v) are already >> under p_candebug(). RNMW> Agreed. In general, my view is that p_cansee() should be used for very RNMW> few of our process inspection APIs. I like your example of ASLR RNMW> especially, as it illustrates how debugging information can aid even RNMW> local attacks (i.e., user vs. setuid binary). Thanks! I will change it to p_candebug(). -- Mikolaj Golub ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: "ps -e" without procfs(5)
On Sun, 4 Dec 2011 15:57:06 + Robert N. M. Watson wrote: RNMW> On 4 Dec 2011, at 14:31, Jilles Tjoelker wrote: >> On Sat, Oct 29, 2011 at 01:32:39PM +0300, Mikolaj Golub wrote: >>> [KERN_PROC_AUXV requires just p_cansee()] >> >> If we are ever going to do ASLR, the AUXV information tells an attacker >> where the stack, executable and RTLD are located, which defeats much of >> the point of randomizing the addresses in the first place. >> >> Given that the AUXV information seems to be used by debuggers only >> anyway, I think it would be good to move it to p_candebug() now. >> >> The full virtual memory maps (KERN_PROC_VMMAP, procstat -v) are already >> under p_candebug(). RNMW> Agreed. In general, my view is that p_cansee() should be used for very RNMW> few of our process inspection APIs. I like your example of ASLR RNMW> especially, as it illustrates how debugging information can aid even RNMW> local attacks (i.e., user vs. setuid binary). What do you think about recently added kern.proc.ps_strings, which returns location of ps_strings structure? It uses p_cansee() too. The location is the same for all processes of the same ABI, so this does not look like sensitive information, on the other hand it also seems to be used by debuggers only. -- Mikolaj Golub ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: "ps -e" without procfs(5)
On Sun, Dec 04, 2011 at 10:58:10PM +0200, Mikolaj Golub wrote: > RNMW> Agreed. In general, my view is that p_cansee() should be used for very > RNMW> few of our process inspection APIs. I like your example of ASLR > RNMW> especially, as it illustrates how debugging information can aid even > RNMW> local attacks (i.e., user vs. setuid binary). > What do you think about recently added kern.proc.ps_strings, which > returns location of ps_strings structure? It uses p_cansee() too. The > location is the same for all processes of the same ABI, so this does > not look like sensitive information, on the other hand it also seems > to be used by debuggers only. With stack ASLR, the address will not be the same for every process of the same ABI and will be sensitive information. Therefore I think this should be locked down too. -- Jilles Tjoelker ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
