Date: Sat, 15 Jan 2011 21:34:02 -0800 From: mdf@FreeBSD.org To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Garrett Cooper <gcooper@freebsd.org> Subject: Re: svn commit: r217369 - in head/sys: cam/scsi sys Message-ID: <AANLkTikwaoUQM7HoxbGxuv6VWp3j2Q58%2Bqx-xQFCudBF@mail.gmail.com> In-Reply-To: <20110116132842.W10642@besplex.bde.org> References: <201101131820.p0DIKXip059402@svn.freebsd.org> <AANLkTinh619WaGgq=5fFxTvEX0JPir34k8xb%2Bs6oSH8Y@mail.gmail.com> <20110114174719.D28159@besplex.bde.org> <AANLkTikVwuSO3h8tKeYXCvC6zqYVHVxdY5Abrzo-Ks2R@mail.gmail.com> <20110115133929.D16210@besplex.bde.org> <AANLkTi=B5mZCJ_bhe=Gf1pLUmWsTswi3O3U2ZLnHMODV@mail.gmail.com> <20110115170529.T16715@besplex.bde.org> <AANLkTimoT9c6q2=K=C7dSnxJXkT=qZx=wPbAf6k68hWZ@mail.gmail.com> <20110116013102.E21684@besplex.bde.org> <AANLkTimLgRijO6yeKd%2Bs2PokGDixL6oMJq_Zhdmco7-k@mail.gmail.com> <20110116132842.W10642@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, Jan 15, 2011 at 7:06 PM, Bruce Evans <brde@optusnet.com.au> wrote: > On Sat, 15 Jan 2011 mdf@freebsd.org wrote: >> On Sat, Jan 15, 2011 at 6:55 AM, Bruce Evans <brde@optusnet.com.au> wrot= e: > >> The printing is done entirely in user-space, so it's not too bad. =A0I >> had figured to upcast everything to [u]intmax_t anyways, and what to >> cast from would just be [u]int32_t and [u]int64_t depending on >> reported size. =A0Anything smaller should be copyout(9)'d as a 4 bit >> quantity. > > I think it shouldn't be converted in the kernel. =A0Applications using > sysctl already have to deal with fields shorter than int in structs > returned by sysctl(). =A0They don't need to deal with integers shorter > than int only since sysctl() doesn't support such integers yet. =A0Short > integers could still be returned as essentially themselves by packing > them into a struct with nothing else. > >> But, there's two factors at play here. =A0The first is sysctl(8) which >> asks for the size when reading and manages it. =A0The second is >> sysctl(2) where we have code like SCTL_MASK32 so a long on a 64-bit >> kernel looks like a long to a 32-bit app. =A0It would be simpler to >> expose this as an 8-byte quantity on 64-bit kernel, and sysctl(8) will >> deal with this just fine, but that would break some uses of sysctl(2). > > What are the uses? =A0I guess they are not-so-hard-coding data types as > int and long, etc., and hard-coding them as int32_t and int64_t. =A0Then > you get an error when the kernel returns an unexpected length, but > unexpected lengths are expected for 32-bit apps on 64-bit kernels. > > BTW, short writes are still horribly broken. =A0Suppose there is a size > mismatch causing an application tries to write 32 bits to a 64-bit > kernel variable. =A0Then no error is detected, and the kernel variable > ois left with garbage in its top bits. =A0The error detection last worked > with 4.4BSD sysctl, and is still documented to work: > > % =A0 =A0 =A0[EINVAL] =A0 =A0 =A0 =A0 =A0 A non-null newp is given and it= s specified length > in > % =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 newlen is too large or = too small. > > I think the case where newlen is too large still works. > > I guess the bug is potentially larger for 32 bit compat applications, > but it is rarely seen for those too since only system management > applications should be writing to kernel variables and there is no > reason to run such applications in 32 bit mode. At Isilon all of userland is still 32-bit only. The reason is that for a while we supported 32-bit and 64-bit kernels in the field, and in fact for various reasons on install the 32-bit kernel always came up first and a reboot after install was forced for 64-bit capable hardware. But we needed the same userspace to work on both kernels. >> However, I *think* it is safe to check the sysctl_req's oldidx/oldlen >> to see what size the user-space expects and cast a kernel long to int >> before SYSCTL_OUT. > > Maybe if the value fits. =A0The application might be probing for a size > that works. =A0Then it shouldn't care what size the value is returned in, > provided the value fits. =A0Writing is only slightly more delicate. =A0Th= e > application must use a size in which the value fits. =A0Then the kernel > can expand the size if necessary. =A0It just has to be careful to fill > the top bits and not have sign extension bugs. =A0sysctl_handle_i() can > probably handle both directions. =A0Lower-level sysctl code should still > return an error if there is a size mismatch. Probing for a size that works should be passing in NULL for the old ptr to get a hint, and for scalars the sie doesn't change. It would be a somewhat malformed app that probes for the size anywhere below e.g. 512 bytes or 1 page. > If a value doesn't fit, then the application will just have to detect > the error and try a larger size (or fail if it doesn't support larger > size). =A0An example might be a 32-bit app reading 64-bit kernel pointers= . > These probably have the high bits set, so they won't fit in 32-bit sizes. > But the application can easily read them using 64-bit sizes. =A0Integers > and pointers larger than 64 bits would be more interesting, since a > compat application might not have any integer data type larger enough > to represent them. > > So I changed my mind about converting in the kernel. =A0Just try to conve= rt > to whatever size the application is trying to use. =A0Don't even force >= =3D 32 > bit sizes on applications. The proposed code does attempt to convert to whatever the app uses, but it assumes the app is using at least a 32-bit quantity. :-) More on the other thread. Thanks, matthew
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTikwaoUQM7HoxbGxuv6VWp3j2Q58%2Bqx-xQFCudBF>