Fix bugs in plugable CC algorithm and siftr sysctls.

[brooks]

Summary

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.

[brooks]

sys/netinet/cc/cc_chd.c

423 ↗

(On Diff #51631)

I really don't know what the right thing to do here is. This code only allows 0. The old code actually allowed 0 and values greater than INT_MAX because it cast to int rather than u_int...

[markj]

sys/netinet/cc/cc_chd.c

423 ↗

(On Diff #51631)

It looks like the intent is to only permit 0 and 1? Why do you say that it only allows 0?

[brooks]

sys/netinet/cc/cc_chd.c

423 ↗

(On Diff #51631)

You're right, for some reason I was parsing as >=1. The previous code failed to do anything sane, but this is ok.

[markj]

sys/netinet/cc/cc_chd.c

423 ↗

(On Diff #51631)

I think you could just use SYSCTL_BOOL and avoid the custom handler.

[brooks]

• Remove comment resulting from mis-reading code.

[brooks]

sys/netinet/cc/cc_chd.c

423 ↗

(On Diff #51631)

That's an API change for programmatic callers, but I suspect that's ok.

441 ↗

(On Diff #51631)

I do wonder if there are enough of these to warrant a SYSCTL_UINT_RANGE or the like.

[markj]

sys/netinet/cc/cc_cdg.c

362 ↗

(On Diff #51658)

In other handlers we read the constant directly, which I find a bit clearer.

sys/netinet/cc/cc_newreno.c

373 ↗

(On Diff #51658)

Coverity will probably complain about testing whether an unsigned quantity is <= 0.

sys/netinet/cc/cc_vegas.c

261 ↗

(On Diff #51658)

new == 0 || ... for consistency?

[brooks]

• Be more consistant in checking for valid values.

[brooks]

sys/netinet/cc/cc_cdg.c

362 ↗

(On Diff #51658)

I agree, but cdg_beta_handler is used twice (beta_loss and beta_delay) so we have to do it this way or duplicate the code.

[gordon]

Based on conversation with brooks, this doesn't need an advisory. Local DoS are exempt from SAs and the information leak is very low quality.

[brooks]

@thj requested transport review before commit so make the them blocking so I don't forget.

[bz]

I volunteered to look at this to get the transport unblocked. Can do tomorrow together in case I'd have a question.