On Fri, Sep 24, 2010 at 02:17:29PM +0400, pluknet wrote: > On 16 September 2010 11:56, Dag-Erling Smorgrav <d...@freebsd.org> wrote: > > Author: des > > Date: Thu Sep 16 07:56:34 2010 > > New Revision: 212723 > > URL: http://svn.freebsd.org/changeset/base/212723 > > > > Log: > > Implement proc/$$/environment. > > > [...] > > > /* > > * Filler function for proc/pid/environ > > */ > > static int > > linprocfs_doprocenviron(PFS_FILL_ARGS) > > { > > + int ret; > > > > - sbuf_printf(sb, "doprocenviron\n%c", '\0'); > > - return (0); > > + PROC_LOCK(p); > > With this change I observe the following sleepable after non-sleepable: > > 1st 0xffffff000290b558 process lock (process lock) @ > /usr/src/sys/modules/linprocfs/../../compat/linprocfs/linprocfs.c:1049 > 2nd 0xffffff00028f8848 user map (user map) @ /usr/src/sys/vm/vm_map.c:3525 > KDB: stack backtrace: > db_trace_self_wrapper() at db_trace_self_wrapper+0x2a > _witness_debugger() at _witness_debugger+0x2e > witness_checkorder() at witness_checkorder+0x807 > _sx_slock() at _sx_slock+0x55 > vm_map_lookup() at vm_map_lookup+0x55 > vm_fault() at vm_fault+0x113 > proc_rwmem() at proc_rwmem+0x7a > linprocfs_doprocenviron() at linprocfs_doprocenviron+0x122 > pfs_read() at pfs_read+0x45b > vn_read() at vn_read+0x256 > dofileread() at dofileread+0xa1 > kern_readv() at kern_readv+0x60 > read() at read+0x55 > syscallenter() at syscallenter+0x1aa > syscall() at syscall+0x4c > Xfast_syscall() at Xfast_syscall+0xe2 > --- syscall (3, FreeBSD ELF64, read), rip = 0x80074134c, rsp = > 0x7fffffffe9c8, rbp = 0 --- > > > > + > > + if ((ret = p_cansee(td, p)) != 0) { > > + PROC_UNLOCK(p); > > + return ret; > > + } > > + > > + ret = linprocfs_doargv(td, p, sb, ps_string_env); > > + PROC_UNLOCK(p); > > + return (ret); > > }
This is easy to fix, isn't it ? But there seems to be much more nits.
First, allocating 512 * sizeof(char *)-byte object on the stack is not
good.
Second, the initialization of iov_len for reading the array
of string pointers misses '* sizeof(char *)'.
And third (probably fatal) is the lack of checks that the end of
array and each string fits into the user portion of the map. I do not
see why addr that already has u_long type is casted to u_long. Also,
VM_MIN_ADDRESS, VM_MAXUSER_ADDRESS constants are for the native host
FreeBSD ABI, they may differ from the target process limits.
Formatting is separate issue, let postpone it.
diff --git a/sys/compat/linprocfs/linprocfs.c b/sys/compat/linprocfs/linprocfs.c
index c7fe158..fd62b1c 100644
--- a/sys/compat/linprocfs/linprocfs.c
+++ b/sys/compat/linprocfs/linprocfs.c
@@ -952,12 +952,9 @@ linprocfs_doargv(struct thread *td, struct proc *p, struct
sbuf *sb,
struct uio tmp_uio;
struct ps_strings pss;
int ret, i, n_elements, found_end;
- u_long addr;
- char* env_vector[MAX_ARGV_STR];
+ u_long addr, pbegin;
+ char **env_vector;
char env_string[UIO_CHUNK_SZ];
- char *pbegin;
-
-
#define UIO_HELPER(uio, iov, base, len, cnt, offset, sz, flg, rw, td)
\
do { \
@@ -971,6 +968,10 @@ do {
\
uio.uio_rw = (rw); \
uio.uio_td = (td); \
} while (0)
+#define VALID_USER_ADDR(addr)
\
+ ((addr) >= p->p_sysent->sv_minuser && (addr) < p->p_sysent->sv_maxuser)
+
+ env_vector = malloc(sizeof(char *) * MAX_ARGV_STR, M_TEMP, M_WAITOK);
UIO_HELPER(tmp_uio, iov, &pss, sizeof(struct ps_strings), 1,
(off_t)(p->p_sysent->sv_psstrings), sizeof(struct ps_strings),
@@ -978,37 +979,41 @@ do {
\
ret = proc_rwmem(p, &tmp_uio);
if (ret != 0)
- return ret;
+ goto done;
/* Get the array address and the number of elements */
resolver(pss, &addr, &n_elements);
/* Consistent with lib/libkvm/kvm_proc.c */
- if (n_elements > MAX_ARGV_STR || (u_long)addr < VM_MIN_ADDRESS ||
- (u_long)addr >= VM_MAXUSER_ADDRESS) {
- /* What error code should we return? */
- return 0;
+ if (n_elements > MAX_ARGV_STR || !VALID_USER_ADDR(addr) ||
+ !VALID_USER_ADDR(addr + MAX_ARGV_STR * sizeof(char *))) {
+ ret = EFAULT;
+ goto done;
}
- UIO_HELPER(tmp_uio, iov, env_vector, MAX_ARGV_STR, 1,
+ UIO_HELPER(tmp_uio, iov, env_vector, MAX_ARGV_STR * sizeof(char *), 1,
(vm_offset_t)(addr), iov.iov_len, UIO_SYSSPACE, UIO_READ, td);
ret = proc_rwmem(p, &tmp_uio);
if (ret != 0)
- return ret;
+ goto done;
/* Now we can iterate through the list of strings */
for (i = 0; i < n_elements; i++) {
found_end = 0;
- pbegin = env_vector[i];
- while(!found_end) {
+ pbegin = (vm_offset_t)env_vector[i];
+ while (!found_end) {
+ if (!VALID_USER_ADDR(pbegin) ||
+ !VALID_USER_ADDR(pbegin + sizeof(env_string))) {
+ ret = EFAULT;
+ goto done;
+ }
UIO_HELPER(tmp_uio, iov, env_string, sizeof(env_string), 1,
- (vm_offset_t) pbegin, iov.iov_len, UIO_SYSSPACE,
- UIO_READ, td);
+ pbegin, iov.iov_len, UIO_SYSSPACE, UIO_READ, td);
ret = proc_rwmem(p, &tmp_uio);
if (ret != 0)
- return ret;
+ goto done;
if (!strvalid(env_string, UIO_CHUNK_SZ)) {
/*
@@ -1017,7 +1022,7 @@ do {
\
* the pointer
*/
sbuf_bcat(sb, env_string, UIO_CHUNK_SZ);
- pbegin = &(*pbegin) + UIO_CHUNK_SZ;
+ pbegin += UIO_CHUNK_SZ;
} else {
found_end = 1;
}
@@ -1025,9 +1030,12 @@ do {
\
sbuf_printf(sb, "%s", env_string);
}
+#undef VALID_USER_ADDR
#undef UIO_HELPER
- return (0);
+done:
+ free(env_vector, M_TEMP);
+ return (ret);
}
static void
@@ -1052,9 +1060,11 @@ linprocfs_doprocenviron(PFS_FILL_ARGS)
PROC_UNLOCK(p);
return ret;
}
+ _PHOLD(p);
+ PROC_UNLOCK(p);
ret = linprocfs_doargv(td, p, sb, ps_string_env);
- PROC_UNLOCK(p);
+ PRELE(p);
return (ret);
}
pgpxbIRcek8rk.pgp
Description: PGP signature
