From owner-svn-src-all@freebsd.org Wed May 18 02:41:58 2016 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 47445B4075D; Wed, 18 May 2016 02:41:58 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail107.syd.optusnet.com.au (mail107.syd.optusnet.com.au [211.29.132.53]) by mx1.freebsd.org (Postfix) with ESMTP id EB75112F6; Wed, 18 May 2016 02:41:57 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c122-106-149-109.carlnfd1.nsw.optusnet.com.au (c122-106-149-109.carlnfd1.nsw.optusnet.com.au [122.106.149.109]) by mail107.syd.optusnet.com.au (Postfix) with ESMTPS id EDC19D426F7; Wed, 18 May 2016 12:41:54 +1000 (AEST) Date: Wed, 18 May 2016 12:41:51 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: yBryan Drewery cc: Gleb Smirnoff , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-releng@freebsd.org Subject: Re: svn commit: r300088 - in releng/9.3: . sys/conf sys/dev/kbd In-Reply-To: <38ca6091-5607-5796-9f6e-7f2d6c117707@FreeBSD.org> Message-ID: <20160518124147.V7042@besplex.bde.org> References: <201605172228.u4HMSbhj012124@repo.freebsd.org> <14a8d29d-bc14-3f96-57a4-81f1b6dfdd82@FreeBSD.org> <20160517230710.GB1015@FreeBSD.org> <38ca6091-5607-5796-9f6e-7f2d6c117707@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=TuMb/2jh c=1 sm=1 tr=0 a=R/f3m204ZbWUO/0rwPSMPw==:117 a=L9H7d07YOLsA:10 a=9cW_t1CCXrUA:10 a=s5jvgZ67dGcA:10 a=kj9zAlcOel0A:10 a=6I5d2MoRAAAA:8 a=RrjDVB9uFQ6jx_tRzx8A:9 a=xs5e6R2pbfrhoDf7:21 a=vVAk96Y2HvFUoVgZ:21 a=CjuIK1q_8ugA:10 a=IjZwj45LgO3ly-622nXo:22 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 18 May 2016 02:41:58 -0000 On Tue, 17 May 2016, Bryan Drewery wrote: > On 5/17/16 4:07 PM, Gleb Smirnoff wrote: >> On Tue, May 17, 2016 at 03:59:26PM -0700, Bryan Drewery wrote: >> B> > Author: glebius >> B> > Date: Tue May 17 22:28:36 2016 >> B> > New Revision: 300088 >> B> > URL: https://svnweb.freebsd.org/changeset/base/300088 >> B> > >> B> > Log: >> B> > - Use unsigned version of min() when handling arguments of SETFKEY ioctl. >> B> > - Validate that user supplied control message length in sendmsg(2) >> B> > is not negative. >> B> >> B> The sendmsg(2) change is not included here (9.3) nor in the advisory but >> B> is in the commit log. Was it intended to be changed in 9.3? >> >> That was my failure to mention SA-16:19 in commit message for 9.3. It doesn't >> apply to 9.x. >> >> B> Plus the only consumer I see is sendit() which seems to be protected >> B> already from negative values when not using COMPAT_43: >> B> >> B> > if (mp->msg_controllen < sizeof(struct cmsghdr) >> B> > #ifdef COMPAT_OLDSOCK >> B> > && mp->msg_flags != MSG_COMPAT >> B> > #endif >> B> > ) { >> B> > error = EINVAL; >> B> > goto bad; >> B> > } >> B> > error = sockargs(&control, mp->msg_control, >> B> > mp->msg_controllen, MT_CONTROL); >> >> No, it isn't protected. In the comparison (mp->msg_controllen < sizeof(struct cmsghdr)) >> both values are unsigned. Later in sockargs() it is treated as signed. But it is protected (except on exotic unsupported arches). The above is a complete bounds check for mp->msg_controllen, written in an obfuscated way using an unsigned type botch/hack. Negative values are normally promoted to large unsigned values, so they fail this check and sockargs() is never called with them. On exotic arches, the analysis is more complicated and the hack doesn't work. It isn't true in general that both values are unsigned (after promotion). E.g., size_t might be uint31_t and int int32_t. Then the binary promotions give int for both operands. Negative values always pass the bounds check then. Part of the botch is the design error that sizeof() is unsigned. This makes it hard to use. It poisons nearby signed types worse than const poisons pointer types. > Ah, I see the (u_int)buflen casts on the older code now. Thanks. That is a different way of writing the botch/hack. It ensures that the hack works for a left operand that has signed type int or small and a right operand that is >= 0 and is representable as u_int. It ensures that both operands are promoted to u_int, with negative values becoming large unsigned ones. I think. This requires int to not have a very large negative range. (u_int)-1 is UINT_MAX, but it isn't so clear what (u_int)INT_MIN is. In fact, I think it can by 0 with 31-bit u_int padded to 32 bits and 32-bit int with INT_MIN = 0x80000000. If this is allowed, then (u_int)INT_MIN is 0. The botch/hack should never be used. Just check for negative values like sockargs() now does. But it is probably better to check in the caller (not using the botch/hack). I also don't like the change from imin() to min() in kbd.c. One of the args is a small integer (MAXFK = 16). Since this doesn't use sizeof(), it doesn't encourage an unsigned botch. The other arg is 'char flen'. char should never be used for numeric values, but this is an old API written before int8_t was available. Using int8_t instead of simply int might be reasonable packing. flen seems to be only initialized once, from .len. This already gives undefined behaviour from overflow, since len has type u_char. The bounds check should be before this assignment, or just use the same type. Using the unsigned botch for len is probably not justified, but u_char is good for packing. If the common type is int8_t or signed char (or plain char to maximise complications), then a check that the length >= 0 will be needed later if the table is under user control. Perhaps the length needs to be strictly > 0 so you need to check the lower bound even using the unsigned botch. The packing using chars is actually just at the end. struct fkeytab is uchar [16] followed by 1 u_char for 'len' at the end. 4 bytes would be natural. On x86, this gives a 17-byte struct which gives a bad layout in arrays, and on other arches it gives portaility problems. struct fkeyarg is u_short, then char [16], then 1 char for 'flen' at the end. 2 bytes would be natural. Bruce