Date: Sat, 21 May 2016 09:02:21 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Konstantin Belousov <kib@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r300332 - in head/sys: amd64/amd64 i386/i386 Message-ID: <20160521081930.I1098@besplex.bde.org> In-Reply-To: <201605201950.u4KJoWA5028092@repo.freebsd.org> References: <201605201950.u4KJoWA5028092@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 20 May 2016, Konstantin Belousov wrote:
> Log:
> Check for overflow and return EINVAL if detected. Backport this and
> r300305 to i386.
>
> PR: 209661
> Reported and reviewed by: cturt
> Sponsored by: The FreeBSD Foundation
> MFC after: 3 days
>
> Modified:
> head/sys/amd64/amd64/sys_machdep.c
> head/sys/i386/i386/sys_machdep.c
>
> Modified: head/sys/i386/i386/sys_machdep.c
> ==============================================================================
> --- head/sys/i386/i386/sys_machdep.c Fri May 20 19:46:25 2016 (r300331)
> +++ head/sys/i386/i386/sys_machdep.c Fri May 20 19:50:32 2016 (r300332)
> @@ -315,8 +315,9 @@ i386_set_ioperm(td, uap)
> struct thread *td;
> struct i386_ioperm_args *uap;
> {
> - int i, error;
> char *iomap;
> + u_int i;
> + int error;
>
> if ((error = priv_check(td, PRIV_IO)) != 0)
> return (error);
> @@ -334,7 +335,8 @@ i386_set_ioperm(td, uap)
> return (error);
> iomap = (char *)td->td_pcb->pcb_ext->ext_iomap;
>
> - if (uap->start + uap->length > IOPAGES * PAGE_SIZE * NBBY)
> + if (uap->start > uap->start + uap->length ||
> + uap->start + uap->length > IOPAGES * PAGE_SIZE * NBBY)
> return (EINVAL);
>
> for (i = uap->start; i < uap->start + uap->length; i++) {
I don't like using u_int for a small index. After the bounds checking
fix, the range fits in a small signed integer. However, uap->start
and uap->length already use bad type u_int, so it is natural to keep
using that type. Except, i386_get/set_ioperm() is undocumented on
amd64, so no one knows what its types are. amd64 documents sysarch(2)
and that refers to i386_get_ioperm(2), i386_get_ldt(2) and i386_vm86(2),
but these man pages are not installed. They are installed on i386.
I think syscalls for them exist ecept for vm86. Man pages are also
missing for newer sysarch() calls, and amd64 has more of these:
{AMD64,I386}_{GET,SET}_XFPUSTATE, {AMD64,I386}_{GET,SET}_{FS,GS}BASE.
apropos can't find anything related.
It would be even more natural to do separate range checks for the
start and length: start >= 0 && start <= IOPAGES * PAGE_SIZE * NBBY &&
length >= 0 && length <= IOPAGES * PAGE_SIZE * NBBY - start. It
isn't clear whether to allow the silly null range of start =
= IOPAGES * PAGE_SIZE * NBBY && length == 0. Initialization doesn't
allow this. Other null ranges should be allowed.
Didn't the old versions already have adequate bounds checking even without
the fixes? The fixes just avoid depending on undefined^Wimplementation-
defined behaviour, and return EINVAL instead of doing nothing for bad ranges:
X if (uap->start + uap->length > IOPAGES * PAGE_SIZE * NBBY)
X return (EINVAL);
X
X for (i = uap->start; i < uap->start + uap->length; i++) {
Suppose (uap->start > uap->start + uap->length). Then if i is unsigned,
the intial i is after the end so nothing is done, and if i is signed
then the same is true if uap->start < INT_MAX; otherwise assignment
overflows to an implementation-defined value which is usually negative
and comparision turns this into a value which is much larger than the
end, so again nothing is done.
Changing a type from signed to unsigned is neither necessary nor
sufficient for getting this right. You have to know that the entire
range fits in the type. The analysis for it fitting in the range
[0, INT_MAX] is not much different for the analysis for it fitting
in the range [0, UINT_MAX], but is often slightly simpler because it
cannot depend on special properties of unsigned types.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160521081930.I1098>