From owner-svn-src-head@FreeBSD.ORG Wed Jan 28 07:38:28 2015 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 3F5FC132; Wed, 28 Jan 2015 07:38:28 +0000 (UTC) Received: from mail106.syd.optusnet.com.au (mail106.syd.optusnet.com.au [211.29.132.42]) by mx1.freebsd.org (Postfix) with ESMTP id C4DBA8DB; Wed, 28 Jan 2015 07:38:27 +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 mail106.syd.optusnet.com.au (Postfix) with ESMTPS id 3856B3C0850; Wed, 28 Jan 2015 18:38:14 +1100 (AEDT) Date: Wed, 28 Jan 2015 18:38:05 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Xin LI Subject: Re: svn commit: r277806 - head/sys/dev/vt In-Reply-To: <201501271935.t0RJZgbo015250@svn.freebsd.org> Message-ID: <20150128183748.X985@besplex.bde.org> References: <201501271935.t0RJZgbo015250@svn.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=C77Ql2/+ c=1 sm=1 tr=0 a=7NqvjVvQucbO2RlWB8PEog==:117 a=PO7r1zJSAAAA:8 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=fugkxVe1s3-v6o3yGx4A:9 a=CjuIK1q_8ugA:10 Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 28 Jan 2015 07:38:28 -0000 On Tue, 27 Jan 2015, Xin LI wrote: > Log: > Use unsigned int for index value. > > Without this change a local attacker could trigger a panic by > tricking the kernel into accessing undefined kernel memory. Why not fix the range check instead of using even more type hacks/errors? > Modified: head/sys/dev/vt/vt_core.c > ============================================================================== > --- head/sys/dev/vt/vt_core.c Tue Jan 27 19:35:38 2015 (r277805) > +++ head/sys/dev/vt/vt_core.c Tue Jan 27 19:35:41 2015 (r277806) > @@ -2367,20 +2367,23 @@ skip_thunk: > } > VT_UNLOCK(vd); > return (EINVAL); > - case VT_WAITACTIVE: > + case VT_WAITACTIVE: { > + unsigned int idx; > + No comment on lexical style bugs. > error = 0; > > - i = *(unsigned int *)data; Bogus cast. The data type is int. (This is of course undocumented. It used to be caddr_t, from the use of _IO() to define VT_WAITACTIVE, and the bogus types and casts for that were even more confusing. Now, _IOWINT() is used and it only supports int, so the type is clearly int. In rare cases, you might need a type hack to get a u_int, but the complications from unsigned types are not needed here.) This code uses a bad type pun to get a u_int, then doesn't even use the u_int but converts without a type pun back to int. > - if (i > VT_MAXWINDOWS) Missing check of lower limit in the range check. Correct fix: just add the missing part: if (i < 0 || i > VT_MAXWINDOWS) > + idx = *(unsigned int *)data; Bogus cast. It uses a bad type pun to get a u_int for using in the subsequent obfuscated range check. The type pun is broken for at least -0 on 1's complement machines (other exotic cases are harder to find and/or understand). -0 should mean the same as plain 0, but the bad type pun makes it UINT_MAX in the 1's complement case. Correct conversion: idx = *(int *)data; (For -0, the RHS is -0 and properly converting that to u_int gives 0.) > + if (idx > VT_MAXWINDOWS) Obfuscated check for the lower limit. Conversion to u_int turns negative values into large unsigned ones that are seen as out of bounds since the upper limit is <= INT_MAX. This hack was a useful manual optimization for primitive compilers in 1984. > return (EINVAL); > - if (i != 0) > - vw = vd->vd_windows[i - 1]; > + if (idx > 0) > + vw = vd->vd_windows[idx - 1]; > > VT_LOCK(vd); > while (vd->vd_curwindow != vw && error == 0) > error = cv_wait_sig(&vd->vd_winswitch, &vd->vd_lock); > VT_UNLOCK(vd); > return (error); > + } > case VT_SETMODE: { /* set screen switcher mode */ > struct vt_mode *mode; > struct proc *p1; Lots of collateral changes from renaming the variable. If you want the obfuscation but not other changes, then just cast the old i in the range check: if ((unsigned int)i > VT_MAXWINDOWS) Small problems nearby: - VT_MAXWINDOWS should have type int so that you don't have to worry about sign extension bugs in the comparison or more cases. (You could make it u_int and not explicitly cast i to deepen the obfuscation. Then the compiler might warn about i < 0 being redundant.) It defaults to 12. But it can be set to the kernel option MAXCONS, and the user can put anything in that. The type of MAXCONS is of course undocumented. - a review of the corresponding code in syscons shows only a fairly benign overflow bug: SC case VT_WAITACTIVE: /* wait for switch to occur */ SC i = (*(int *)data == 0) ? scp->index : (*(int *)data - 1); SC if ((i < sc->first_vty) || (i >= sc->first_vty + sc->vtys)) SC return EINVAL; SC if (i == sc->cur_scp->index) SC return 0; syscons gets the range check right without really trying since its lower limit is variable and not obviously 0, so the unsigned obfuscation is not available. It handles the special case i == 0 differently by mapping i before the range check. There is an overflow bug in the mapping: i = INT_MIN is invalid and for this i, i - 1 overflows before it is checked. Normally it overflows to INT_MAX and is detected as invalid later. The early mapping is otherwise better than in vt. It allows a more natural check of the upper limit in the range check. The check in vt involves 3 or 4 layers of compensating adjustments by +-1. Bruce