Date: Fri, 24 Sep 2010 16:06:44 +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: <20100924130644.GI34228@deviant.kiev.zoral.com.ua> In-Reply-To: <AANLkTimd9oqJ2sFMvTpGBXUZwJmYeex1frRnQTJ_tgt%2B@mail.gmail.com> References: <201009160756.o8G7uZrg065332@svn.freebsd.org> <AANLkTi=20QDe6o2YxM8PTKOLXB3ZuxUCNQEPtWy9P4Rc@mail.gmail.com> <20100924115311.GH34228@deviant.kiev.zoral.com.ua> <AANLkTimd9oqJ2sFMvTpGBXUZwJmYeex1frRnQTJ_tgt%2B@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--+Z7/5fzWRHDJ0o7Q Content-Type: text/plain; charset=koi8-r Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Sep 24, 2010 at 04:24:18PM +0400, pluknet wrote: > 2010/9/24 Kostik Belousov <kostikbel@gmail.com>: > > On Fri, Sep 24, 2010 at 02:17:29PM +0400, pluknet wrote: > >> On 16 September 2010 11:56, Dag-Erling Smorgrav <des@freebsd.org> wrot= e: > >> > 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. > >> > > >> [...] > >> > >> > =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); > >> > >> With this change I observe the following sleepable after non-sleepable: > >> > [LOR there] > >> > >> > >> > + > >> > + =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. > > >=20 > Thanks for quick reaction. >=20 > 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/linpro= cfs.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__ */ =20 +#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, s= truct 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 =20 #define UIO_HELPER(uio, iov, base, len, cnt, offset, sz, flg, rw, td) \ do { \ @@ -971,63 +976,112 @@ do { \ uio.uio_rw =3D (rw); \ uio.uio_td =3D (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 =3D proc_rwmem(p, &tmp_uio); - if (ret !=3D 0) - return ret; +#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); + +#ifdef COMPAT_FREEBSD32 + if ((p->p_sysent->sv_flags & SV_ILP32) !=3D 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 =3D proc_rwmem(p, &tmp_uio); + if (ret !=3D 0) + goto done; + pss.ps_argvstr =3D PTRIN(pss32.ps_argvstr); + pss.ps_nargvstr =3D pss32.ps_nargvstr; + pss.ps_envstr =3D PTRIN(pss32.ps_envstr); + pss.ps_nenvstr =3D pss32.ps_nenvstr; + + elm_len =3D 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 =3D proc_rwmem(p, &tmp_uio); + if (ret !=3D 0) + goto done; + + elm_len =3D MAX_ARGV_STR * sizeof(char *); +#ifdef COMPAT_FREEBSD32 + } +#endif =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 + elm_len)) { + ret =3D EFAULT; + goto done; } =20 - 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) !=3D 0) { + for (i =3D 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 =3D proc_rwmem(p, &tmp_uio); + if (ret !=3D 0) + goto done; + env_vector[i] =3D 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 =3D proc_rwmem(p, &tmp_uio); + if (ret !=3D 0) + goto done; +#ifdef COMPAT_FREEBSD32 + } +#endif =20 - ret =3D proc_rwmem(p, &tmp_uio); - if (ret !=3D 0) - return ret; =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; + ret =3D proc_rwmem(p, &tmp_uio); + if (ret !=3D 0) { + ret =3D 0; + goto done; + } =20 - 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 =3D &(*pbegin) + UIO_CHUNK_SZ; - } else { + pbegin +=3D UIO_CHUNK_SZ; + } else found_end =3D 1; - } } 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 +1106,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 --+Z7/5fzWRHDJ0o7Q Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (FreeBSD) iEYEARECAAYFAkycomQACgkQC3+MBN1Mb4gm+wCg5iz9Y7aJiRsIeucwFPQ3D/HC bewAoLN9HnfvWULhs4Vm6a36njMwqkcV =V696 -----END PGP SIGNATURE----- --+Z7/5fzWRHDJ0o7Q--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100924130644.GI34228>