Skip to content

Commit

Permalink
MFC r342125:
Browse files Browse the repository at this point in the history 
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.

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
  • Loading branch information
@brooksdavis
brooksdavis committed Dec 18, 2018
1 parent e1522ef commit 92b6550
Show file tree
Hide file tree
Showing 7 changed files with 60 additions and 58 deletions.
35 changes: 24 additions & 11 deletions sys/netinet/cc/cc_cdg.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,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

Expand Down Expand Up @@ -358,22 +356,37 @@ cdg_cb_destroy(struct cc_var *ccv)
static int
cdg_beta_handler(SYSCTL_HANDLER_ARGS)
{
int error;
uint32_t new;

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;
}

if (req->newptr != NULL &&
(CAST_PTR_INT(req->newptr) == 0 || CAST_PTR_INT(req->newptr) > 100))
return (EINVAL);

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;

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;
}

if (req->newptr != NULL && CAST_PTR_INT(req->newptr) < 1)
return (EINVAL);

return (sysctl_handle_int(oidp, arg1, arg2, req));
return (error);
}

static inline uint32_t
Expand Down
9 changes: 3 additions & 6 deletions sys/netinet/cc/cc_chd.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,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.
Expand Down Expand Up @@ -421,7 +419,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;
Expand All @@ -439,8 +437,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;
Expand All @@ -458,7 +455,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;
Expand Down
8 changes: 3 additions & 5 deletions sys/netinet/cc/cc_dctcp.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
VNET_DEFINE_STATIC(uint32_t, dctcp_alpha) = 0;
#define V_dctcp_alpha VNET(dctcp_alpha)
Expand Down Expand Up @@ -400,7 +398,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)
Expand All @@ -422,7 +420,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;
Expand All @@ -440,7 +438,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;
Expand Down
10 changes: 3 additions & 7 deletions sys/netinet/cc/cc_hd.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,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

Expand Down Expand Up @@ -188,8 +186,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;
Expand All @@ -207,7 +204,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;
Expand All @@ -225,8 +222,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;
Expand Down
18 changes: 11 additions & 7 deletions sys/netinet/cc/cc_newreno.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,6 @@ __FBSDID("$FreeBSD$");
static MALLOC_DEFINE(M_NEWRENO, "newreno data",
"newreno beta values");

#define CAST_PTR_INT(X) (*((int*)(X)))

static void newreno_cb_destroy(struct cc_var *ccv);
static void newreno_ack_received(struct cc_var *ccv, uint16_t type);
static void newreno_after_idle(struct cc_var *ccv);
Expand Down Expand Up @@ -364,15 +362,21 @@ newreno_ctl_output(struct cc_var *ccv, struct sockopt *sopt, void *buf)
static int
newreno_beta_handler(SYSCTL_HANDLER_ARGS)
{
int error;
uint32_t new;

if (req->newptr != NULL ) {
new = *(uint32_t *)arg1;
error = sysctl_handle_int(oidp, &new, 0, req);
if (error == 0 && req->newptr != NULL ) {
if (arg1 == &VNET_NAME(newreno_beta_ecn) && !V_cc_do_abe)
return (EACCES);
if (CAST_PTR_INT(req->newptr) <= 0 || CAST_PTR_INT(req->newptr) > 100)
return (EINVAL);
error = EACCES;
else if (new == 0 || new > 100)
error = EINVAL;
else
*(uint32_t *)arg1 = new;
}

return (sysctl_handle_int(oidp, arg1, arg2, req));
return (error);
}

SYSCTL_DECL(_net_inet_tcp_cc_newreno);
Expand Down
8 changes: 2 additions & 6 deletions sys/netinet/cc/cc_vegas.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,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.
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down
30 changes: 14 additions & 16 deletions sys/netinet/siftr.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,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)

Expand Down Expand Up @@ -1442,22 +1440,22 @@ siftr_manage_ops(uint8_t action)
static int
siftr_sysctl_enabled_handler(SYSCTL_HANDLER_ARGS)
{
if (req->newptr == NULL)
goto skip;

/* 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);
int error;
uint32_t new;

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);
}


Expand Down

0 comments on commit 92b6550

Please sign in to comment.