svn commit: r277806 - head/sys/dev/vt

[Bruce Evans]

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