Bug 185852

Summary: bad geom ctl request causes kernel panic
Product: Base System Reporter: Wolfgang B. <blub>
Component: kernAssignee: Andrey V. Elsukov <ae>
Status: Closed FIXED    
Severity: Affects Only Me    
Priority: Normal    
Version: Unspecified   
Hardware: Any   
OS: Any   

Description Wolfgang B. 2014-01-17 16:10:03 UTC 
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.)

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>
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
Comment 1 Andrey V. Elsukov freebsd_committer freebsd_triage 2014-01-23 06:52:52 UTC 
Responsible Changed
From-To: freebsd-bugs->ae

Take it.
Comment 2 dfilter service freebsd_committer freebsd_triage 2014-01-23 19:31:24 UTC 
Author: ae
Date: Thu Jan 23 19:31:17 2014
New Revision: 261085
URL: http://svnweb.freebsd.org/changeset/base/261085

Log:
  Fix typo in r261084.
  Add to the gctl_error() an ability to specify error description even
  if numeric error code is already specified. Also by default set
  error code to EINVAL.
  
  PR:		185852
  MFC after:	1 week

Modified:
  head/sys/geom/geom_ctl.c

Modified: head/sys/geom/geom_ctl.c
==============================================================================
--- head/sys/geom/geom_ctl.c	Thu Jan 23 19:07:22 2014	(r261084)
+++ head/sys/geom/geom_ctl.c	Thu Jan 23 19:31:17 2014	(r261085)
@@ -84,8 +84,8 @@ g_ctl_init(void)
 }
 
 /*
- * Report an error back to the user in ascii format.  Return whatever copyout
- * returned, or EINVAL if it succeeded.
+ * Report an error back to the user in ascii format.  Return nerror
+ * or EINVAL if nerror isn't specified.
  */
 int
 gctl_error(struct gctl_req *req, const char *fmt, ...)
@@ -99,9 +99,10 @@ gctl_error(struct gctl_req *req, const c
 	if (sbuf_done(req->serror)) {
 		if (!req->nerror)
 			req->nerror = EEXIST;
-	}
-	if (req->nerror)
 		return (req->nerror);
+	}
+	if (!req->nerror)
+		req->nerror = EINVAL;
 
 	va_start(ap, fmt);
 	sbuf_vprintf(req->serror, fmt, ap);
@@ -109,7 +110,7 @@ gctl_error(struct gctl_req *req, const c
 	sbuf_finish(req->serror);
 	if (g_debugflags & G_F_CTLDUMP)
 		printf("gctl %p error \"%s\"\n", req, sbuf_data(req->serror));
-	return (0);
+	return (req->nerror);
 }
 
 /*
@@ -122,7 +123,7 @@ geom_alloc_copyin(struct gctl_req *req, 
 	void *ptr;
 
 	ptr = g_malloc(len, M_WAITOK);
-	nreq->nerror = copyin(uaddr, ptr, len);
+	req->nerror = copyin(uaddr, ptr, len);
 	if (!req->nerror)
 		return (ptr);
 	if (ptr != NULL)
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 3 Andrey V. Elsukov freebsd_committer freebsd_triage 2014-01-23 21:11:45 UTC 
State Changed
From-To: open->patched

Patched in head/. Thanks!
Comment 4 dfilter service freebsd_committer freebsd_triage 2014-01-30 10:53:36 UTC 
Author: ae
Date: Thu Jan 30 10:53:29 2014
New Revision: 261285
URL: http://svnweb.freebsd.org/changeset/base/261285

Log:
  MFC r261084:
    malloc() with M_WAITOK doesn't return NULL.
  
  MFC r261085:
    Fix typo in r261084.
    Add to the gctl_error() an ability to specify error description even
    if numeric error code is already specified. Also by default set
    error code to EINVAL.
  
    PR:		185852
  
  MFC r261086:
    In gctl_copyin() remove unused error variable.
    geom_alloc_copyin() can't return ENOMEM, so describe its fail as bad
    control request. Add check for NULL pointer in gctl_dump(), since it
    can be NULL when geom_alloc_copyin() failed.
  
  MFC r261089:
    Remove another unneeded NULL check from geom_alloc_copyin().
    Do copyout in case of gctl version mismatch and fix sbuf leak in
    g_ctl_ioctl_ctl().
  
  MFC r261091:
    Always free sbuf in gctl_free().

Modified:
  stable/10/sys/geom/geom_ctl.c
Directory Properties:
  stable/10/   (props changed)

Modified: stable/10/sys/geom/geom_ctl.c
==============================================================================
--- stable/10/sys/geom/geom_ctl.c	Thu Jan 30 08:37:23 2014	(r261284)
+++ stable/10/sys/geom/geom_ctl.c	Thu Jan 30 10:53:29 2014	(r261285)
@@ -84,8 +84,8 @@ g_ctl_init(void)
 }
 
 /*
- * Report an error back to the user in ascii format.  Return whatever copyout
- * returned, or EINVAL if it succeeded.
+ * Report an error back to the user in ascii format.  Return nerror
+ * or EINVAL if nerror isn't specified.
  */
 int
 gctl_error(struct gctl_req *req, const char *fmt, ...)
@@ -99,9 +99,10 @@ gctl_error(struct gctl_req *req, const c
 	if (sbuf_done(req->serror)) {
 		if (!req->nerror)
 			req->nerror = EEXIST;
-	}
-	if (req->nerror)
 		return (req->nerror);
+	}
+	if (!req->nerror)
+		req->nerror = EINVAL;
 
 	va_start(ap, fmt);
 	sbuf_vprintf(req->serror, fmt, ap);
@@ -109,7 +110,7 @@ gctl_error(struct gctl_req *req, const c
 	sbuf_finish(req->serror);
 	if (g_debugflags & G_F_CTLDUMP)
 		printf("gctl %p error \"%s\"\n", req, sbuf_data(req->serror));
-	return (0);
+	return (req->nerror);
 }
 
 /*
@@ -122,27 +123,23 @@ geom_alloc_copyin(struct gctl_req *req, 
 	void *ptr;
 
 	ptr = g_malloc(len, M_WAITOK);
-	if (ptr == NULL)
-		req->nerror = ENOMEM;
-	else
-		req->nerror = copyin(uaddr, ptr, len);
+	req->nerror = copyin(uaddr, ptr, len);
 	if (!req->nerror)
 		return (ptr);
-	if (ptr != NULL)
-		g_free(ptr);
+	g_free(ptr);
 	return (NULL);
 }
 
 static void
 gctl_copyin(struct gctl_req *req)
 {
-	int error, i;
 	struct gctl_req_arg *ap;
 	char *p;
+	int i;
 
 	ap = geom_alloc_copyin(req, req->arg, req->narg * sizeof(*ap));
 	if (ap == NULL) {
-		req->nerror = ENOMEM;
+		gctl_error(req, "bad control request");
 		req->arg = NULL;
 		return;
 	}
@@ -154,10 +151,9 @@ gctl_copyin(struct gctl_req *req)
 		ap[i].kvalue = NULL;
 	}
 
-	error = 0;
 	for (i = 0; i < req->narg; i++) {
 		if (ap[i].nlen < 1 || ap[i].nlen > SPECNAMELEN) {
-			error = gctl_error(req,
+			gctl_error(req,
 			    "wrong param name length %d: %d", i, ap[i].nlen);
 			break;
 		}
@@ -165,14 +161,14 @@ gctl_copyin(struct gctl_req *req)
 		if (p == NULL)
 			break;
 		if (p[ap[i].nlen - 1] != '\0') {
-			error = gctl_error(req, "unterminated param name");
+			gctl_error(req, "unterminated param name");
 			g_free(p);
 			break;
 		}
 		ap[i].name = p;
 		ap[i].flag |= GCTL_PARAM_NAMEKERNEL;
 		if (ap[i].len <= 0) {
-			error = gctl_error(req, "negative param length");
+			gctl_error(req, "negative param length");
 			break;
 		}
 		p = geom_alloc_copyin(req, ap[i].value, ap[i].len);
@@ -180,7 +176,7 @@ gctl_copyin(struct gctl_req *req)
 			break;
 		if ((ap[i].flag & GCTL_PARAM_ASCII) &&
 		    p[ap[i].len - 1] != '\0') {
-			error = gctl_error(req, "unterminated param value");
+			gctl_error(req, "unterminated param value");
 			g_free(p);
 			break;
 		}
@@ -218,6 +214,7 @@ gctl_free(struct gctl_req *req)
 {
 	int i;
 
+	sbuf_delete(req->serror);
 	if (req->arg == NULL)
 		return;
 	for (i = 0; i < req->narg; i++) {
@@ -228,15 +225,14 @@ gctl_free(struct gctl_req *req)
 			g_free(req->arg[i].kvalue);
 	}
 	g_free(req->arg);
-	sbuf_delete(req->serror);
 }
 
 static void
 gctl_dump(struct gctl_req *req)
 {
+	struct gctl_req_arg *ap;
 	u_int i;
 	int j;
-	struct gctl_req_arg *ap;
 
 	printf("Dump of gctl request at %p:\n", req);
 	if (req->nerror > 0) {
@@ -244,6 +240,8 @@ gctl_dump(struct gctl_req *req)
 		if (sbuf_len(req->serror) > 0)
 			printf("  error:\t\"%s\"\n", sbuf_data(req->serror));
 	}
+	if (req->arg == NULL)
+		return;
 	for (i = 0; i < req->narg; i++) {
 		ap = &req->arg[i];
 		if (!(ap->flag & GCTL_PARAM_NAMEKERNEL))
@@ -464,30 +462,31 @@ g_ctl_ioctl_ctl(struct cdev *dev, u_long
 
 	req = (void *)data;
 	req->nerror = 0;
-	req->serror = sbuf_new_auto();
 	/* It is an error if we cannot return an error text */
 	if (req->lerror < 2)
 		return (EINVAL);
 	if (!useracc(req->error, req->lerror, VM_PROT_WRITE))
 		return (EINVAL);
 
+	req->serror = sbuf_new_auto();
 	/* Check the version */
-	if (req->version != GCTL_VERSION)
-		return (gctl_error(req,
-		    "kernel and libgeom version mismatch."));
-	
-	/* Get things on board */
-	gctl_copyin(req);
-
-	if (g_debugflags & G_F_CTLDUMP)
-		gctl_dump(req);
-
-	if (!req->nerror) {
-		g_waitfor_event(g_ctl_req, req, M_WAITOK, NULL);
-		gctl_copyout(req);
+	if (req->version != GCTL_VERSION) {
+		gctl_error(req, "kernel and libgeom version mismatch.");
+		req->arg = NULL;
+	} else {
+		/* Get things on board */
+		gctl_copyin(req);
+
+		if (g_debugflags & G_F_CTLDUMP)
+			gctl_dump(req);
+
+		if (!req->nerror) {
+			g_waitfor_event(g_ctl_req, req, M_WAITOK, NULL);
+			gctl_copyout(req);
+		}
 	}
 	if (sbuf_done(req->serror)) {
-		req->nerror = copyout(sbuf_data(req->serror), req->error,
+		copyout(sbuf_data(req->serror), req->error,
 		    imin(req->lerror, sbuf_len(req->serror) + 1));
 	}
 
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 5 dfilter service freebsd_committer freebsd_triage 2014-01-30 10:54:19 UTC 
Author: ae
Date: Thu Jan 30 10:54:11 2014
New Revision: 261286
URL: http://svnweb.freebsd.org/changeset/base/261286

Log:
  MFC r261084:
    malloc() with M_WAITOK doesn't return NULL.
  
  MFC r261085:
    Fix typo in r261084.
    Add to the gctl_error() an ability to specify error description even
    if numeric error code is already specified. Also by default set
    error code to EINVAL.
  
    PR:		185852
  
  MFC r261086:
    In gctl_copyin() remove unused error variable.
    geom_alloc_copyin() can't return ENOMEM, so describe its fail as bad
    control request. Add check for NULL pointer in gctl_dump(), since it
    can be NULL when geom_alloc_copyin() failed.
  
  MFC r261089:
    Remove another unneeded NULL check from geom_alloc_copyin().
    Do copyout in case of gctl version mismatch and fix sbuf leak in
    g_ctl_ioctl_ctl().
  
  MFC r261091:
    Always free sbuf in gctl_free().

Modified:
  stable/9/sys/geom/geom_ctl.c
Directory Properties:
  stable/9/sys/   (props changed)

Modified: stable/9/sys/geom/geom_ctl.c
==============================================================================
--- stable/9/sys/geom/geom_ctl.c	Thu Jan 30 10:53:29 2014	(r261285)
+++ stable/9/sys/geom/geom_ctl.c	Thu Jan 30 10:54:11 2014	(r261286)
@@ -84,8 +84,8 @@ g_ctl_init(void)
 }
 
 /*
- * Report an error back to the user in ascii format.  Return whatever copyout
- * returned, or EINVAL if it succeeded.
+ * Report an error back to the user in ascii format.  Return nerror
+ * or EINVAL if nerror isn't specified.
  */
 int
 gctl_error(struct gctl_req *req, const char *fmt, ...)
@@ -99,9 +99,10 @@ gctl_error(struct gctl_req *req, const c
 	if (sbuf_done(req->serror)) {
 		if (!req->nerror)
 			req->nerror = EEXIST;
-	}
-	if (req->nerror)
 		return (req->nerror);
+	}
+	if (!req->nerror)
+		req->nerror = EINVAL;
 
 	va_start(ap, fmt);
 	sbuf_vprintf(req->serror, fmt, ap);
@@ -109,7 +110,7 @@ gctl_error(struct gctl_req *req, const c
 	sbuf_finish(req->serror);
 	if (g_debugflags & G_F_CTLDUMP)
 		printf("gctl %p error \"%s\"\n", req, sbuf_data(req->serror));
-	return (0);
+	return (req->nerror);
 }
 
 /*
@@ -122,27 +123,23 @@ geom_alloc_copyin(struct gctl_req *req, 
 	void *ptr;
 
 	ptr = g_malloc(len, M_WAITOK);
-	if (ptr == NULL)
-		req->nerror = ENOMEM;
-	else
-		req->nerror = copyin(uaddr, ptr, len);
+	req->nerror = copyin(uaddr, ptr, len);
 	if (!req->nerror)
 		return (ptr);
-	if (ptr != NULL)
-		g_free(ptr);
+	g_free(ptr);
 	return (NULL);
 }
 
 static void
 gctl_copyin(struct gctl_req *req)
 {
-	int error, i;
 	struct gctl_req_arg *ap;
 	char *p;
+	int i;
 
 	ap = geom_alloc_copyin(req, req->arg, req->narg * sizeof(*ap));
 	if (ap == NULL) {
-		req->nerror = ENOMEM;
+		gctl_error(req, "bad control request");
 		req->arg = NULL;
 		return;
 	}
@@ -154,10 +151,9 @@ gctl_copyin(struct gctl_req *req)
 		ap[i].kvalue = NULL;
 	}
 
-	error = 0;
 	for (i = 0; i < req->narg; i++) {
 		if (ap[i].nlen < 1 || ap[i].nlen > SPECNAMELEN) {
-			error = gctl_error(req,
+			gctl_error(req,
 			    "wrong param name length %d: %d", i, ap[i].nlen);
 			break;
 		}
@@ -165,14 +161,14 @@ gctl_copyin(struct gctl_req *req)
 		if (p == NULL)
 			break;
 		if (p[ap[i].nlen - 1] != '\0') {
-			error = gctl_error(req, "unterminated param name");
+			gctl_error(req, "unterminated param name");
 			g_free(p);
 			break;
 		}
 		ap[i].name = p;
 		ap[i].flag |= GCTL_PARAM_NAMEKERNEL;
 		if (ap[i].len <= 0) {
-			error = gctl_error(req, "negative param length");
+			gctl_error(req, "negative param length");
 			break;
 		}
 		p = geom_alloc_copyin(req, ap[i].value, ap[i].len);
@@ -180,7 +176,7 @@ gctl_copyin(struct gctl_req *req)
 			break;
 		if ((ap[i].flag & GCTL_PARAM_ASCII) &&
 		    p[ap[i].len - 1] != '\0') {
-			error = gctl_error(req, "unterminated param value");
+			gctl_error(req, "unterminated param value");
 			g_free(p);
 			break;
 		}
@@ -218,6 +214,7 @@ gctl_free(struct gctl_req *req)
 {
 	int i;
 
+	sbuf_delete(req->serror);
 	if (req->arg == NULL)
 		return;
 	for (i = 0; i < req->narg; i++) {
@@ -228,15 +225,14 @@ gctl_free(struct gctl_req *req)
 			g_free(req->arg[i].kvalue);
 	}
 	g_free(req->arg);
-	sbuf_delete(req->serror);
 }
 
 static void
 gctl_dump(struct gctl_req *req)
 {
+	struct gctl_req_arg *ap;
 	u_int i;
 	int j;
-	struct gctl_req_arg *ap;
 
 	printf("Dump of gctl request at %p:\n", req);
 	if (req->nerror > 0) {
@@ -244,6 +240,8 @@ gctl_dump(struct gctl_req *req)
 		if (sbuf_len(req->serror) > 0)
 			printf("  error:\t\"%s\"\n", sbuf_data(req->serror));
 	}
+	if (req->arg == NULL)
+		return;
 	for (i = 0; i < req->narg; i++) {
 		ap = &req->arg[i];
 		if (!(ap->flag & GCTL_PARAM_NAMEKERNEL))
@@ -464,30 +462,31 @@ g_ctl_ioctl_ctl(struct cdev *dev, u_long
 
 	req = (void *)data;
 	req->nerror = 0;
-	req->serror = sbuf_new_auto();
 	/* It is an error if we cannot return an error text */
 	if (req->lerror < 2)
 		return (EINVAL);
 	if (!useracc(req->error, req->lerror, VM_PROT_WRITE))
 		return (EINVAL);
 
+	req->serror = sbuf_new_auto();
 	/* Check the version */
-	if (req->version != GCTL_VERSION)
-		return (gctl_error(req,
-		    "kernel and libgeom version mismatch."));
-	
-	/* Get things on board */
-	gctl_copyin(req);
-
-	if (g_debugflags & G_F_CTLDUMP)
-		gctl_dump(req);
-
-	if (!req->nerror) {
-		g_waitfor_event(g_ctl_req, req, M_WAITOK, NULL);
-		gctl_copyout(req);
+	if (req->version != GCTL_VERSION) {
+		gctl_error(req, "kernel and libgeom version mismatch.");
+		req->arg = NULL;
+	} else {
+		/* Get things on board */
+		gctl_copyin(req);
+
+		if (g_debugflags & G_F_CTLDUMP)
+			gctl_dump(req);
+
+		if (!req->nerror) {
+			g_waitfor_event(g_ctl_req, req, M_WAITOK, NULL);
+			gctl_copyout(req);
+		}
 	}
 	if (sbuf_done(req->serror)) {
-		req->nerror = copyout(sbuf_data(req->serror), req->error,
+		copyout(sbuf_data(req->serror), req->error,
 		    imin(req->lerror, sbuf_len(req->serror) + 1));
 	}
 
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 6 Andrey V. Elsukov freebsd_committer freebsd_triage 2014-01-30 11:25:17 UTC 
State Changed
From-To: patched->closed

Merged to stable/9 and stable/10. Thanks!