Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 18 Dec 2018 09:16:04 +0000 (UTC)
From:      Brooks Davis <brooks@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-11@freebsd.org
Subject:   svn commit: r342189 - in stable/11/sys/netinet: . cc
Message-ID:  <201812180916.wBI9G4Hk089142@repo.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: brooks
Date: Tue Dec 18 09:16:04 2018
New Revision: 342189
URL: https://svnweb.freebsd.org/changeset/base/342189

Log:
  Partial MFC of r342125:
  
  Fix bugs in plugable CC algorithm and siftr sysctls.
  
  Use the sysctl_handle_int() handler to write out the old value and read
  the new value into a temporary variable. Use the temporary variable
  for any checks of values rather than using the CAST_PTR_INT() macro on
  req->newptr. The prior usage read directly from userspace memory if the
  sysctl() was called correctly. This is unsafe and doesn't work at all on
  some architectures (at least i386.)
  
  In some cases, the code could also be tricked into reading from kernel
  memory and leaking limited information about the contents or crashing
  the system. This was true for CDG, newreno, and siftr on all platforms
  and true for i386 in all cases. The impact of this bug is largest in
  VIMAGE jails which have been configured to allow writing to these
  sysctls.
  
  Per discussion with the security officer, we will not be issuing an
  advisory for this issue as root access and a non-default config are
  required to be impacted.
  
  Changes to sys/netinet/cc/cc_newreno.c are not merged as the sysctl's
  had not previously been merged.
  
  Reviewed by:	markj, bz
  Discussed with:	gordon (security officer)
  Security:	kernel information leak, local DoS (both require root)
  Differential Revision:	https://reviews.freebsd.org/D18443

Modified:
  stable/11/sys/netinet/cc/cc_cdg.c
  stable/11/sys/netinet/cc/cc_chd.c
  stable/11/sys/netinet/cc/cc_dctcp.c
  stable/11/sys/netinet/cc/cc_hd.c
  stable/11/sys/netinet/cc/cc_vegas.c
  stable/11/sys/netinet/siftr.c
Directory Properties:
  stable/11/   (props changed)

Modified: stable/11/sys/netinet/cc/cc_cdg.c
==============================================================================
--- stable/11/sys/netinet/cc/cc_cdg.c	Tue Dec 18 09:13:50 2018	(r342188)
+++ stable/11/sys/netinet/cc/cc_cdg.c	Tue Dec 18 09:16:04 2018	(r342189)
@@ -78,8 +78,6 @@ __FBSDID("$FreeBSD$");
 
 #define	CDG_VERSION "0.1"
 
-#define	CAST_PTR_INT(X) (*((int*)(X)))
-
 /* Private delay-gradient induced congestion control signal. */
 #define	CC_CDG_DELAY 0x01000000
 
@@ -356,22 +354,37 @@ cdg_cb_destroy(struct cc_var *ccv)
 static int
 cdg_beta_handler(SYSCTL_HANDLER_ARGS)
 {
+	int error;
+	uint32_t new;
 
-	if (req->newptr != NULL &&
-	    (CAST_PTR_INT(req->newptr) == 0 || CAST_PTR_INT(req->newptr) > 100))
-		return (EINVAL);
+	new = *(uint32_t *)arg1;
+	error = sysctl_handle_int(oidp, &new, 0, req);
+	if (error == 0 && req->newptr != NULL) {
+		if (new == 0 || new > 100)
+			error = EINVAL;
+		else
+			*(uint32_t *)arg1 = new;
+	}
 
-	return (sysctl_handle_int(oidp, arg1, arg2, req));
+	return (error);
 }
 
 static int
 cdg_exp_backoff_scale_handler(SYSCTL_HANDLER_ARGS)
 {
+	int error;
+	uint32_t new;
 
-	if (req->newptr != NULL && CAST_PTR_INT(req->newptr) < 1)
-		return (EINVAL);
+	new = *(uint32_t *)arg1;
+	error = sysctl_handle_int(oidp, &new, 0, req);
+	if (error == 0 && req->newptr != NULL) {
+		if (new < 1)
+			error = EINVAL;
+		else
+			*(uint32_t *)arg1 = new;
+	}
 
-	return (sysctl_handle_int(oidp, arg1, arg2, req));
+	return (error);
 }
 
 static inline unsigned long

Modified: stable/11/sys/netinet/cc/cc_chd.c
==============================================================================
--- stable/11/sys/netinet/cc/cc_chd.c	Tue Dec 18 09:13:50 2018	(r342188)
+++ stable/11/sys/netinet/cc/cc_chd.c	Tue Dec 18 09:16:04 2018	(r342189)
@@ -76,8 +76,6 @@ __FBSDID("$FreeBSD$");
 
 #include <netinet/khelp/h_ertt.h>
 
-#define	CAST_PTR_INT(X)	(*((int*)(X)))
-
 /*
  * Private signal type for rate based congestion signal.
  * See <netinet/cc.h> for appropriate bit-range to use for private signals.
@@ -420,7 +418,7 @@ chd_loss_fair_handler(SYSCTL_HANDLER_ARGS)
 	new = V_chd_loss_fair;
 	error = sysctl_handle_int(oidp, &new, 0, req);
 	if (error == 0 && req->newptr != NULL) {
-		if (CAST_PTR_INT(req->newptr) > 1)
+		if (new > 1)
 			error = EINVAL;
 		else
 			V_chd_loss_fair = new;
@@ -438,8 +436,7 @@ chd_pmax_handler(SYSCTL_HANDLER_ARGS)
 	new = V_chd_pmax;
 	error = sysctl_handle_int(oidp, &new, 0, req);
 	if (error == 0 && req->newptr != NULL) {
-		if (CAST_PTR_INT(req->newptr) == 0 ||
-		    CAST_PTR_INT(req->newptr) > 100)
+		if (new == 0 || new > 100)
 			error = EINVAL;
 		else
 			V_chd_pmax = new;
@@ -457,7 +454,7 @@ chd_qthresh_handler(SYSCTL_HANDLER_ARGS)
 	new = V_chd_qthresh;
 	error = sysctl_handle_int(oidp, &new, 0, req);
 	if (error == 0 && req->newptr != NULL) {
-		if (CAST_PTR_INT(req->newptr) <= V_chd_qmin)
+		if (new <= V_chd_qmin)
 			error = EINVAL;
 		else
 			V_chd_qthresh = new;

Modified: stable/11/sys/netinet/cc/cc_dctcp.c
==============================================================================
--- stable/11/sys/netinet/cc/cc_dctcp.c	Tue Dec 18 09:13:50 2018	(r342188)
+++ stable/11/sys/netinet/cc/cc_dctcp.c	Tue Dec 18 09:16:04 2018	(r342189)
@@ -56,8 +56,6 @@ __FBSDID("$FreeBSD$");
 #include <netinet/cc/cc.h>
 #include <netinet/cc/cc_module.h>
 
-#define	CAST_PTR_INT(X)	(*((int*)(X)))
-
 #define MAX_ALPHA_VALUE 1024
 static VNET_DEFINE(uint32_t, dctcp_alpha) = 0;
 #define V_dctcp_alpha	    VNET(dctcp_alpha)
@@ -401,7 +399,7 @@ dctcp_alpha_handler(SYSCTL_HANDLER_ARGS)
 	new = V_dctcp_alpha;
 	error = sysctl_handle_int(oidp, &new, 0, req);
 	if (error == 0 && req->newptr != NULL) {
-		if (CAST_PTR_INT(req->newptr) > 1)
+		if (new > 1)
 			error = EINVAL;
 		else {
 			if (new > MAX_ALPHA_VALUE)
@@ -423,7 +421,7 @@ dctcp_shift_g_handler(SYSCTL_HANDLER_ARGS)
 	new = V_dctcp_shift_g;
 	error = sysctl_handle_int(oidp, &new, 0, req);
 	if (error == 0 && req->newptr != NULL) {
-		if (CAST_PTR_INT(req->newptr) > 1)
+		if (new > 1)
 			error = EINVAL;
 		else
 			V_dctcp_shift_g = new;
@@ -441,7 +439,7 @@ dctcp_slowstart_handler(SYSCTL_HANDLER_ARGS)
 	new = V_dctcp_slowstart;
 	error = sysctl_handle_int(oidp, &new, 0, req);
 	if (error == 0 && req->newptr != NULL) {
-		if (CAST_PTR_INT(req->newptr) > 1)
+		if (new > 1)
 			error = EINVAL;
 		else
 			V_dctcp_slowstart = new;

Modified: stable/11/sys/netinet/cc/cc_hd.c
==============================================================================
--- stable/11/sys/netinet/cc/cc_hd.c	Tue Dec 18 09:13:50 2018	(r342188)
+++ stable/11/sys/netinet/cc/cc_hd.c	Tue Dec 18 09:16:04 2018	(r342189)
@@ -77,8 +77,6 @@ __FBSDID("$FreeBSD$");
 
 #include <netinet/khelp/h_ertt.h>
 
-#define	CAST_PTR_INT(X)	(*((int*)(X)))
-
 /* Largest possible number returned by random(). */
 #define	RANDOM_MAX	INT_MAX
 
@@ -186,8 +184,7 @@ hd_pmax_handler(SYSCTL_HANDLER_ARGS)
 	new = V_hd_pmax;
 	error = sysctl_handle_int(oidp, &new, 0, req);
 	if (error == 0 && req->newptr != NULL) {
-		if (CAST_PTR_INT(req->newptr) == 0 ||
-		    CAST_PTR_INT(req->newptr) > 100)
+		if (new == 0 || new > 100)
 			error = EINVAL;
 		else
 			V_hd_pmax = new;
@@ -205,7 +202,7 @@ hd_qmin_handler(SYSCTL_HANDLER_ARGS)
 	new = V_hd_qmin;
 	error = sysctl_handle_int(oidp, &new, 0, req);
 	if (error == 0 && req->newptr != NULL) {
-		if (CAST_PTR_INT(req->newptr) > V_hd_qthresh)
+		if (new > V_hd_qthresh)
 			error = EINVAL;
 		else
 			V_hd_qmin = new;
@@ -223,8 +220,7 @@ hd_qthresh_handler(SYSCTL_HANDLER_ARGS)
 	new = V_hd_qthresh;
 	error = sysctl_handle_int(oidp, &new, 0, req);
 	if (error == 0 && req->newptr != NULL) {
-		if (CAST_PTR_INT(req->newptr) < 1 ||
-		    CAST_PTR_INT(req->newptr) < V_hd_qmin)
+		if (new == 0 || new < V_hd_qmin)
 			error = EINVAL;
 		else
 			V_hd_qthresh = new;

Modified: stable/11/sys/netinet/cc/cc_vegas.c
==============================================================================
--- stable/11/sys/netinet/cc/cc_vegas.c	Tue Dec 18 09:13:50 2018	(r342188)
+++ stable/11/sys/netinet/cc/cc_vegas.c	Tue Dec 18 09:16:04 2018	(r342189)
@@ -77,8 +77,6 @@ __FBSDID("$FreeBSD$");
 
 #include <netinet/khelp/h_ertt.h>
 
-#define	CAST_PTR_INT(X)	(*((int*)(X)))
-
 /*
  * Private signal type for rate based congestion signal.
  * See <netinet/cc.h> for appropriate bit-range to use for private signals.
@@ -260,8 +258,7 @@ vegas_alpha_handler(SYSCTL_HANDLER_ARGS)
 	new = V_vegas_alpha;
 	error = sysctl_handle_int(oidp, &new, 0, req);
 	if (error == 0 && req->newptr != NULL) {
-		if (CAST_PTR_INT(req->newptr) < 1 ||
-		    CAST_PTR_INT(req->newptr) > V_vegas_beta)
+		if (new == 0 || new > V_vegas_beta)
 			error = EINVAL;
 		else
 			V_vegas_alpha = new;
@@ -279,8 +276,7 @@ vegas_beta_handler(SYSCTL_HANDLER_ARGS)
 	new = V_vegas_beta;
 	error = sysctl_handle_int(oidp, &new, 0, req);
 	if (error == 0 && req->newptr != NULL) {
-		if (CAST_PTR_INT(req->newptr) < 1 ||
-		    CAST_PTR_INT(req->newptr) < V_vegas_alpha)
+		if (new == 0 || new < V_vegas_alpha)
 			 error = EINVAL;
 		else
 			V_vegas_beta = new;

Modified: stable/11/sys/netinet/siftr.c
==============================================================================
--- stable/11/sys/netinet/siftr.c	Tue Dec 18 09:13:50 2018	(r342188)
+++ stable/11/sys/netinet/siftr.c	Tue Dec 18 09:16:04 2018	(r342189)
@@ -150,8 +150,6 @@ __FBSDID("$FreeBSD$");
 #endif
 
 /* useful macros */
-#define CAST_PTR_INT(X) (*((int*)(X)))
-
 #define UPPER_SHORT(X)	(((X) & 0xFFFF0000) >> 16)
 #define LOWER_SHORT(X)	((X) & 0x0000FFFF)
 
@@ -1440,22 +1438,22 @@ siftr_manage_ops(uint8_t action)
 static int
 siftr_sysctl_enabled_handler(SYSCTL_HANDLER_ARGS)
 {
-	if (req->newptr == NULL)
-		goto skip;
+	int error;
+	uint32_t new;
 
-	/* If the value passed in isn't 0 or 1, return an error. */
-	if (CAST_PTR_INT(req->newptr) != 0 && CAST_PTR_INT(req->newptr) != 1)
-		return (1);
-
-	/* If we are changing state (0 to 1 or 1 to 0). */
-	if (CAST_PTR_INT(req->newptr) != siftr_enabled )
-		if (siftr_manage_ops(CAST_PTR_INT(req->newptr))) {
-			siftr_manage_ops(SIFTR_DISABLE);
-			return (1);
+	new = siftr_enabled;
+	error = sysctl_handle_int(oidp, &new, 0, req);
+	if (error != 0 && req->newptr != NULL) {
+		if (new > 1)
+			return (EINVAL);
+		else if (new != siftr_enabled) {
+			error = siftr_manage_ops(new);
+			if (error != 0)
+				siftr_manage_ops(SIFTR_DISABLE);
 		}
+	}
 
-skip:
-	return (sysctl_handle_int(oidp, arg1, arg2, req));
+	return (error);
 }
 
 



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201812180916.wBI9G4Hk089142>