From owner-svn-src-all@FreeBSD.ORG Fri Sep 24 13:17:36 2010 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id E5D3C1065670; Fri, 24 Sep 2010 13:17:35 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id A1E028FC17; Fri, 24 Sep 2010 13:17:35 +0000 (UTC) Received: from bigwig.baldwin.cx (66.111.2.69.static.nyinternet.net [66.111.2.69]) by cyrus.watson.org (Postfix) with ESMTPSA id 30D6846B35; Fri, 24 Sep 2010 09:17:35 -0400 (EDT) Received: from jhbbsd.localnet (smtp.hudson-trading.com [209.249.190.9]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 326558A03C; Fri, 24 Sep 2010 09:17:34 -0400 (EDT) From: John Baldwin To: Kostik Belousov Date: Fri, 24 Sep 2010 08:34:16 -0400 User-Agent: KMail/1.13.5 (FreeBSD/7.3-CBSD-20100819; KDE/4.4.5; amd64; ; ) References: <201009160756.o8G7uZrg065332@svn.freebsd.org> <20100924115311.GH34228@deviant.kiev.zoral.com.ua> In-Reply-To: <20100924115311.GH34228@deviant.kiev.zoral.com.ua> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Message-Id: <201009240834.16786.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.0.1 (bigwig.baldwin.cx); Fri, 24 Sep 2010 09:17:34 -0400 (EDT) X-Virus-Scanned: clamav-milter 0.95.1 at bigwig.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-2.6 required=4.2 tests=AWL,BAYES_00 autolearn=ham version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on bigwig.baldwin.cx Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, pluknet , src-committers@freebsd.org, Dag-Erling Smorgrav Subject: Re: svn commit: r212723 - head/sys/compat/linprocfs X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 24 Sep 2010 13:17:36 -0000 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 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