svn commit: r212723 - head/sys/compat/linprocfs
John Baldwin
jhb at freebsd.org
Fri Sep 24 13:17:36 UTC 2010
On Friday, September 24, 2010 7:53:11 am Kostik Belousov wrote:
> On Fri, Sep 24, 2010 at 02:17:29PM +0400, pluknet wrote:
> > On 16 September 2010 11:56, Dag-Erling Smorgrav <des at 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);
> }
You need to add a P_WEXIT check here. _PHOLD() / PRELE() do not work once
P_WEXIT is set (rather, exit1() may not notice and wait for a _PHOLD() invoked
to drain once P_WEXIT is set).
--
John Baldwin
More information about the svn-src-all
mailing list