From owner-svn-src-stable-11@freebsd.org Tue Dec 18 09:16:06 2018 Return-Path: Delivered-To: svn-src-stable-11@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 93C9413488A8; Tue, 18 Dec 2018 09:16:06 +0000 (UTC) (envelope-from brooks@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 3705988F0E; Tue, 18 Dec 2018 09:16:06 +0000 (UTC) (envelope-from brooks@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 2682424B6A; Tue, 18 Dec 2018 09:16:06 +0000 (UTC) (envelope-from brooks@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id wBI9G553089147; Tue, 18 Dec 2018 09:16:05 GMT (envelope-from brooks@FreeBSD.org) Received: (from brooks@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id wBI9G4Hk089142; Tue, 18 Dec 2018 09:16:04 GMT (envelope-from brooks@FreeBSD.org) Message-Id: <201812180916.wBI9G4Hk089142@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: brooks set sender to brooks@FreeBSD.org using -f From: Brooks Davis Date: Tue, 18 Dec 2018 09:16:04 +0000 (UTC) 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 X-SVN-Group: stable-11 X-SVN-Commit-Author: brooks X-SVN-Commit-Paths: in stable/11/sys/netinet: . cc X-SVN-Commit-Revision: 342189 X-SVN-Commit-Repository: base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: 3705988F0E X-Spamd-Bar: -- Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-2.97 / 15.00]; local_wl_from(0.00)[FreeBSD.org]; NEURAL_HAM_MEDIUM(-1.00)[-0.997,0]; NEURAL_HAM_SHORT(-0.97)[-0.973,0]; ASN(0.00)[asn:11403, ipnet:2610:1c1:1::/48, country:US]; NEURAL_HAM_LONG(-1.00)[-0.996,0] X-BeenThere: svn-src-stable-11@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for only the 11-stable src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 18 Dec 2018 09:16:06 -0000 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 -#define CAST_PTR_INT(X) (*((int*)(X))) - /* * Private signal type for rate based congestion signal. * See 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 #include -#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 -#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 -#define CAST_PTR_INT(X) (*((int*)(X))) - /* * Private signal type for rate based congestion signal. * See 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); }