kern/185852: bad geom ctl request causes kernel panic

[Wolfgang B.]

>Number:         185852
>Category:       kern
>Synopsis:       bad geom ctl request causes kernel panic
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Fri Jan 17 16:10:03 UTC 2014
>Closed-Date:
>Last-Modified:
>Originator:     Wolfgang B.
>Release:        9.2 release memstick and 10-CURRENT from stable/10 git
>Organization:
>Environment:
FreeBSD new-host-2 9.2-RELEASE FreeBSD 9.2-RELEASE #0 r255898: Thu Sep 26 22:50:31 UTC 2013     root at bake.isc.freebsd.org:/usr/obj/usr/src/sys/GENERIC  amd64

and:
FreeBSD new-host-2 10.0-PRERELEASE FreeBSD 10.0-PRERELEASE #2 c80798a(stable/10)-dirty: Fri Jan 17 16:41:13 CET 2014     wry at wrybsd:/home/wry/FreeBSD/obj/amd64.amd64/home/wry/FreeBSD/freebsd/sys/GENERIC  x86_64
>Description:
Many of gctl_copyin's error cases don't actually set the request's nerror member.
If for instance the "class" value gets corrupted (or deliberately set to a bad value) (specifically to be non-null-terminated but GCTL_PARAM_ASCII added to its flags), the gctl_copyin will not get to setting kvalue, which is then NULL and later-on accessed in gctl_get_class, where it checks for a terminating null character.

Seems like the a lot of the geom subsystem could use some more error checking.
Ie GEOM_PARAM_KERNELVALUE never gets set on the request argument, so why aren't all accesses to its kvalue member masked by this flag if it exists?

(A kernel should not have to rely on the userspace being friendly to it. This is a lucky case in that you by default need to be in the operator group to issue such a request.)
>How-To-Repeat:
Send a libgeom request filled with garbage.
Example code:
http://users.archbsd.net/~blub/pastes/blub/geom_crash.c

compile with -lgeom and run with a 'doit' parameter to trigger the panic

>Fix:
make gctl_error or its callers (gctl_copyin) set req->nerror in all error cases.

The diff [1] is dirty and not the recommended way to go about this, but it did protect against the geom_crash.c file linked in the "How to repeat the problem" section. (got the correct "unterminated param value" error with it)
Which shows me that my assumption of how/where this is going bad is probably correct.

[1] <http://users.archbsd.net/~blub/pastes/blub/geom_ctl.diff>

>Release-Note:
>Audit-Trail:
>Unformatted: