From owner-freebsd-bugs@FreeBSD.ORG Tue May 20 08:14:59 2014 Return-Path: Delivered-To: freebsd-bugs@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 60B28973; Tue, 20 May 2014 08:14:59 +0000 (UTC) Received: from mail105.syd.optusnet.com.au (mail105.syd.optusnet.com.au [211.29.132.249]) by mx1.freebsd.org (Postfix) with ESMTP id 0C6172A2D; Tue, 20 May 2014 08:14:58 +0000 (UTC) Received: from c122-106-147-133.carlnfd1.nsw.optusnet.com.au (c122-106-147-133.carlnfd1.nsw.optusnet.com.au [122.106.147.133]) by mail105.syd.optusnet.com.au (Postfix) with ESMTPS id 5C5FB1045800; Tue, 20 May 2014 18:14:45 +1000 (EST) Date: Tue, 20 May 2014 18:14:43 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Peter Holm Subject: Re: kern/189941: getgroups(2) implements first argument as unsigned int In-Reply-To: <201405190921.s4J9LtP3089031@cgiserv.freebsd.org> Message-ID: <20140520181439.U1106@besplex.bde.org> References: <201405190921.s4J9LtP3089031@cgiserv.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=U6SrU4bu c=1 sm=1 tr=0 a=7NqvjVvQucbO2RlWB8PEog==:117 a=PO7r1zJSAAAA:8 a=6VvQiweIh_QA:10 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=FsfUT3zU3IdbKml9gW4A:9 a=FxWBkiClJFFsMZP5:21 a=z5PliHWsOR6rrUGH:21 a=CjuIK1q_8ugA:10 Cc: freebsd-bugs@freebsd.org, freebsd-gnats-submit@freebsd.org X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 20 May 2014 08:14:59 -0000 On Mon, 19 May 2014, Peter Holm wrote: >> Description: > Passing -1 as gidsetlen is not detected. Discovered by ATF. Caught on Ubuntu and OS/X. A typical error from abusing an unsigned variable as a counter. This doesn't even match the API. This bug was in 4.4BSD. The fix seems to be almost as simple as changing the u_int for getgroups() in syscalls.master to int. Also change 3 corresponding u_int's in kern_prot.c setgroups() has the same API design error, but there it reduces to an obfuscated way to check for invalid counts (for getgroups(), negative counts are automatically detected as invalid, since they are too small to hold any actual number of groups). For setgroups(), negative counts are are type-punned to become large unsigned counts, so they are obfuscatedly detected as invalid because they are larger than any actual number for {NGROUPS_MAX}. The check for that should for negative counts. Grepping for u_ in syscalls.master shows abuse of u_int for almost all uses: - dup, dup2: not even wrong, modulo type puns being benign. These syscalls take int args, but syscalls. master says that they take u_int args. This converts the ints to u_ints using a type pun. But the kernel converts these u_ints back to ints at the top level. Obfuscated tests for negative values for file descriptors using the unsigned hack have (all?) been fixed in at least kern_descrip.c too. - profil: correct - getlogin: POSIX specifies size_t for getlogin_r(3), but FreeBSD still uses int. The getlogin syscall is not directly available, but is closer to getlogin_r(3) than getlogin(3). It uses u_int where the FreeBSD API says int. This sort of works. It is an obfuscated way of giving the POSIX semantics instead of the documented semantics. An arg of -1 doesn't mean -1, but means SIZE_MAX in POSIX and UINT_MAX in FreeBSD. This is a physically impossible size (except on 64-bit systems while the punned FreeBSD semantics), but the POSIX semantics don't allow detecting it as an error, and the FreeBSD behaviour is to reduce it to MAXLOGNAME. The behaviour is undefined in most cases if the arg is -1, but in practice the syscall will do the right thing if the buffer has size >= MAXLOGNAME. - getgroups, setgroups: see above - getitimer, setitimer: like setgroups (just the unsigned hack, but the range checking is slightly more obfuscated: the valid values are 0, 1 and 2, so checking for the unsigned value being <= {the one that happens to be 2} gives an obfuscated quick check for the value being one of these three. - gethostname: POSIX specifies size_t. FreeBSD documents size_t, but actually uses u_int. On 64-bit arches, the buffer may have the silly but valid size of 2**32. This is blindly truncated to 0 so the syscall fails. The arg is converted back to size_t at the top level in the kernel so as to pass the address of a size_t to another function, but it has already been truncated then. Similar truncation breaks the accidental change to POSIX semantics in some cases in getlogin_r. - sethostname: not in POSIX. FreeBSD still documents int, but still actually uses u_int. The arg is converted back to size_t too late to preserve it, as for gethostname. - readv, writev: POSIX and FreeBSD document that the iovcnt arg is int. FreeBSD type puns it to u_int, then converts this to size_t before using it as an arg for copyin*(). No errror checking is done except in copyin*() where the error checking is adequate. -1 becomes UINT_MAX vua the type pun, but is not increased further to SIZE_MAX on 64-bit arches. U_INT is usually too large, so the result is probably EFAULT. - getrlimit, setrlimit. Like getitimer and setitimer, except the there is a macro to de-obfuscate the upper limit. In the same file, the 'which' variable for get/setpriority is handled quite differently. It is left as an int and checked using a case statement. - getdirentries: just the unsigned hack - __sysctl: at least matches the documented API. The API uses u_int for small counts and size_t for large counts. Old APIs don't have the design error of using u_int where int would do. New APIs mostly use size_t excessively. My grep doesn't find these. It finds this one since it has a strange mixture of u_int and size_t. - poll: correct, I think. POSIX specifies nfds_t, and IIRC has the design errors of requiring this to be unsigned and having excessive typedefs for the function. FreeBSD spells it u_int to avoid some namespace pollution. It should spell it nfds_t except in syscalls.master and files generated from it. - preadv, pwritev: like readv, writev (?) - __getcwd: POSIX specifies size_t and FreeBSD documents size_t for getcwd(). Using u_int probably gives the usual truncation bugs. - *audit*: not checked. The only set of new APIs that uses u_int. About 25 years anachronistic with C90's and POSIX's converting even old APIs to use size_t. - _umtx_op: not checked - *cap*: not checked. Mostly not count args or plain u_int. It mispells uint64_t as u_int64_t. The correct spelling uint* is used for just 1 syscall, and my grep for u_ didn't find it. This use seems to be to avoid namespace pollution, so it is correct. This points to further uses of unsigned types which were hidden by althernative spellings. The style bug of using 'unsigned' is used a bit: - nmount: uses 'unsigned int'. Otherwise like writev (?), except the documented API also has this bug. - *ksem*, *kmq*: uses 'unsigned int'. Not checked. Seems to be undocumented. - jail_get, jail_set: like nmount. Stress and regression tests could try to find bugs in all of the above, but I code inspection only found the harmless truncation bugs for u_int instead of size_t in addition to the one in the PR. The one in the PR is a rare case where the unsigned comparison hack doesn't give fail-safe behaviour. Bruce