Date: Wed, 18 May 2016 12:41:51 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: yBryan Drewery <bdrewery@freebsd.org> Cc: Gleb Smirnoff <glebius@freebsd.org>, 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 Message-ID: <20160518124147.V7042@besplex.bde.org> In-Reply-To: <38ca6091-5607-5796-9f6e-7f2d6c117707@FreeBSD.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>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <table>.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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160518124147.V7042>