On Fri, Sep 24, 2010 at 04:24:18PM +0400, pluknet wrote: > 2010/9/24 Kostik Belousov <kostik...@gmail.com>: > > 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: > >> > [LOR there] > >> > >> > >> > + > >> > + 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. > > > > Thanks for quick reaction. > > As for the latter, something doesn't quite work here. > I see EFAULT against i386 process running on amd64.
Right, the COMPAT_FREEBSD32 case should properly convert both
struct ps_strings and array of pointers to the host structures.
So this is fourth issue.
diff --git a/sys/compat/linprocfs/linprocfs.c b/sys/compat/linprocfs/linprocfs.c
index c7fe158..db837e2 100644
--- a/sys/compat/linprocfs/linprocfs.c
+++ b/sys/compat/linprocfs/linprocfs.c
@@ -96,6 +96,10 @@ __FBSDID("$FreeBSD$");
#include <machine/md_var.h>
#endif /* __i386__ || __amd64__ */
+#ifdef COMPAT_FREEBSD32
+#include <compat/freebsd32/freebsd32_util.h>
+#endif
+
#ifdef COMPAT_LINUX32 /* XXX */
#include <machine/../linux32/linux.h>
#else
@@ -951,13 +955,14 @@ linprocfs_doargv(struct thread *td, struct proc *p,
struct sbuf *sb,
struct iovec iov;
struct uio tmp_uio;
struct ps_strings pss;
- int ret, i, n_elements, found_end;
- u_long addr;
- char* env_vector[MAX_ARGV_STR];
+ int ret, i, n_elements, elm_len, found_end;
+ u_long addr, pbegin;
+ char **env_vector;
char env_string[UIO_CHUNK_SZ];
- char *pbegin;
-
-
+#ifdef COMPAT_FREEBSD32
+ struct freebsd32_ps_strings pss32;
+ uint32_t ptr;
+#endif
#define UIO_HELPER(uio, iov, base, len, cnt, offset, sz, flg, rw, td)
\
do { \
@@ -971,63 +976,112 @@ do {
\
uio.uio_rw = (rw); \
uio.uio_td = (td); \
} while (0)
-
- UIO_HELPER(tmp_uio, iov, &pss, sizeof(struct ps_strings), 1,
- (off_t)(p->p_sysent->sv_psstrings), sizeof(struct ps_strings),
- UIO_SYSSPACE, UIO_READ, td);
-
- ret = proc_rwmem(p, &tmp_uio);
- if (ret != 0)
- return ret;
+#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);
+
+#ifdef COMPAT_FREEBSD32
+ if ((p->p_sysent->sv_flags & SV_ILP32) != 0) {
+ UIO_HELPER(tmp_uio, iov, &pss32, sizeof(pss32), 1,
+ (off_t)(p->p_sysent->sv_psstrings),
+ sizeof(pss32), UIO_SYSSPACE, UIO_READ, td);
+ ret = proc_rwmem(p, &tmp_uio);
+ if (ret != 0)
+ goto done;
+ pss.ps_argvstr = PTRIN(pss32.ps_argvstr);
+ pss.ps_nargvstr = pss32.ps_nargvstr;
+ pss.ps_envstr = PTRIN(pss32.ps_envstr);
+ pss.ps_nenvstr = pss32.ps_nenvstr;
+
+ elm_len = MAX_ARGV_STR * sizeof(int32_t);
+ } else {
+#endif
+ UIO_HELPER(tmp_uio, iov, &pss, sizeof(pss), 1,
+ (off_t)(p->p_sysent->sv_psstrings),
+ sizeof(pss), UIO_SYSSPACE, UIO_READ, td);
+ ret = proc_rwmem(p, &tmp_uio);
+ if (ret != 0)
+ goto done;
+
+ elm_len = MAX_ARGV_STR * sizeof(char *);
+#ifdef COMPAT_FREEBSD32
+ }
+#endif
/* 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 + elm_len)) {
+ ret = EFAULT;
+ goto done;
}
- UIO_HELPER(tmp_uio, iov, env_vector, MAX_ARGV_STR, 1,
- (vm_offset_t)(addr), iov.iov_len, UIO_SYSSPACE, UIO_READ, td);
+#ifdef COMPAT_FREEBSD32
+ if ((p->p_sysent->sv_flags & SV_ILP32) != 0) {
+ for (i = 0; i < MAX_ARGV_STR; i++) {
+ UIO_HELPER(tmp_uio, iov, &ptr, sizeof(ptr), 1,
+ (vm_offset_t)(addr + i * sizeof(uint32_t)),
+ iov.iov_len, UIO_SYSSPACE, UIO_READ, td);
+ ret = proc_rwmem(p, &tmp_uio);
+ if (ret != 0)
+ goto done;
+ env_vector[i] = PTRIN(ptr);
+ }
+ } else {
+#endif
+ UIO_HELPER(tmp_uio, iov, env_vector, elm_len, 1,
+ (vm_offset_t)(addr), iov.iov_len, UIO_SYSSPACE, UIO_READ,
+ td);
+ ret = proc_rwmem(p, &tmp_uio);
+ if (ret != 0)
+ goto done;
+#ifdef COMPAT_FREEBSD32
+ }
+#endif
- ret = proc_rwmem(p, &tmp_uio);
- if (ret != 0)
- return ret;
/* 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;
+ ret = proc_rwmem(p, &tmp_uio);
+ if (ret != 0) {
+ ret = 0;
+ goto done;
+ }
- if (!strvalid(env_string, UIO_CHUNK_SZ)) {
+ if (!strvalid(env_string, UIO_CHUNK_SZ)) {
/*
- * We didn't find the end of the string
+ * We didn't find the end of the string.
* Add the string to the buffer and move
- * the pointer
+ * the pointer.
*/
sbuf_bcat(sb, env_string, UIO_CHUNK_SZ);
- pbegin = &(*pbegin) + UIO_CHUNK_SZ;
- } else {
+ pbegin += UIO_CHUNK_SZ;
+ } else
found_end = 1;
- }
}
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 +1106,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);
}
pgpGj1SxCF13s.pgp
Description: PGP signature
