Date: Fri, 24 Sep 2010 14:53:11 +0300 From: Kostik Belousov <kostikbel@gmail.com> To: pluknet <pluknet@gmail.com> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Dag-Erling Smorgrav <des@freebsd.org> Subject: Re: svn commit: r212723 - head/sys/compat/linprocfs Message-ID: <20100924115311.GH34228@deviant.kiev.zoral.com.ua> In-Reply-To: <AANLkTi=20QDe6o2YxM8PTKOLXB3ZuxUCNQEPtWy9P4Rc@mail.gmail.com> References: <201009160756.o8G7uZrg065332@svn.freebsd.org> <AANLkTi=20QDe6o2YxM8PTKOLXB3ZuxUCNQEPtWy9P4Rc@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--c7hkjup166d4FzgN Content-Type: text/plain; charset=koi8-r Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Sep 24, 2010 at 02:17:29PM +0400, pluknet wrote: > On 16 September 2010 11:56, Dag-Erling Smorgrav <des@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: > > =9AImplement proc/$$/environment. > > > [...] >=20 > > =9A/* > > =9A* Filler function for proc/pid/environ > > =9A*/ > > =9Astatic int > > =9Alinprocfs_doprocenviron(PFS_FILL_ARGS) > > =9A{ > > + =9A =9A =9A int ret; > > > > - =9A =9A =9A sbuf_printf(sb, "doprocenviron\n%c", '\0'); > > - =9A =9A =9A return (0); > > + =9A =9A =9A PROC_LOCK(p); >=20 > With this change I observe the following sleepable after non-sleepable: >=20 > 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:35= 25 > 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 =3D 0x80074134c, rsp =3D > 0x7fffffffe9c8, rbp =3D 0 --- >=20 >=20 > > + > > + =9A =9A =9A if ((ret =3D p_cansee(td, p)) !=3D 0) { > > + =9A =9A =9A =9A =9A =9A =9A PROC_UNLOCK(p); > > + =9A =9A =9A =9A =9A =9A =9A return ret; > > + =9A =9A =9A } > > + > > + =9A =9A =9A ret =3D linprocfs_doargv(td, p, sb, ps_string_env); > > + =9A =9A =9A PROC_UNLOCK(p); > > + =9A =9A =9A return (ret); > > =9A} 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/linpro= cfs.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, st= ruct 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; - - =20 #define UIO_HELPER(uio, iov, base, len, cnt, offset, sz, flg, rw, td) \ do { \ @@ -971,6 +968,10 @@ do { \ uio.uio_rw =3D (rw); \ uio.uio_td =3D (td); \ } while (0) +#define VALID_USER_ADDR(addr) \ + ((addr) >=3D p->p_sysent->sv_minuser && (addr) < p->p_sysent->sv_maxuser) + + env_vector =3D malloc(sizeof(char *) * MAX_ARGV_STR, M_TEMP, M_WAITOK); =20 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 { \ =20 ret =3D proc_rwmem(p, &tmp_uio); if (ret !=3D 0) - return ret; + goto done; =20 /* Get the array address and the number of elements */ resolver(pss, &addr, &n_elements); =20 /* Consistent with lib/libkvm/kvm_proc.c */ - if (n_elements > MAX_ARGV_STR || (u_long)addr < VM_MIN_ADDRESS || - (u_long)addr >=3D 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 =3D EFAULT; + goto done; } =20 - 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); =20 ret =3D proc_rwmem(p, &tmp_uio); if (ret !=3D 0) - return ret; + goto done; =20 /* Now we can iterate through the list of strings */ for (i =3D 0; i < n_elements; i++) { found_end =3D 0; - pbegin =3D env_vector[i]; - while(!found_end) { + pbegin =3D (vm_offset_t)env_vector[i]; + while (!found_end) { + if (!VALID_USER_ADDR(pbegin) || + !VALID_USER_ADDR(pbegin + sizeof(env_string))) { + ret =3D 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); =20 ret =3D proc_rwmem(p, &tmp_uio); if (ret !=3D 0) - return ret; + goto done; =20 if (!strvalid(env_string, UIO_CHUNK_SZ)) { /* @@ -1017,7 +1022,7 @@ do { \ * the pointer */ sbuf_bcat(sb, env_string, UIO_CHUNK_SZ); - pbegin =3D &(*pbegin) + UIO_CHUNK_SZ; + pbegin +=3D UIO_CHUNK_SZ; } else { found_end =3D 1; } @@ -1025,9 +1030,12 @@ do { \ sbuf_printf(sb, "%s", env_string); } =20 +#undef VALID_USER_ADDR #undef UIO_HELPER =20 - return (0); +done: + free(env_vector, M_TEMP); + return (ret); } =20 static void @@ -1052,9 +1060,11 @@ linprocfs_doprocenviron(PFS_FILL_ARGS) PROC_UNLOCK(p); return ret; } + _PHOLD(p); + PROC_UNLOCK(p); =20 ret =3D linprocfs_doargv(td, p, sb, ps_string_env); - PROC_UNLOCK(p); + PRELE(p); return (ret); } =20 --c7hkjup166d4FzgN Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (FreeBSD) iEYEARECAAYFAkyckScACgkQC3+MBN1Mb4gV3ACglhzFjvkwxkueLStOkEcmkot0 dJYAoJUZRbmAxaWGJZRO5f6TmOCPxOiH =/lkV -----END PGP SIGNATURE----- --c7hkjup166d4FzgN--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100924115311.GH34228>