Page MenuHomeFreeBSD

Fix bugs in plugable CC algorithm and siftr sysctls.
ClosedPublic

Authored by brooks on Thu, Dec 6, 12:25 AM.

Details

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.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

brooks created this revision.Thu, Dec 6, 12:25 AM
brooks marked an inline comment as done.Thu, Dec 6, 12:37 AM
brooks added inline comments.
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 added inline comments.Thu, Dec 6, 12:50 AM
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 marked an inline comment as done.Thu, Dec 6, 4:25 AM
brooks added inline comments.
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 added inline comments.Thu, Dec 6, 4:27 AM
sys/netinet/cc/cc_chd.c
423 ↗(On Diff #51631)

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

brooks updated this revision to Diff 51658.Thu, Dec 6, 4:14 PM
  • Remove comment resulting from mis-reading code.
brooks marked 3 inline comments as done.
brooks added inline comments.
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.

brooks edited the summary of this revision. (Show Details)Thu, Dec 6, 4:20 PM
markj accepted this revision.Thu, Dec 6, 4:29 PM
markj added inline comments.
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?

This revision is now accepted and ready to land.Thu, Dec 6, 4:29 PM
brooks updated this revision to Diff 51662.Thu, Dec 6, 4:54 PM
brooks marked an inline comment as done.
  • Be more consistant in checking for valid values.
This revision now requires review to proceed.Thu, Dec 6, 4:54 PM
brooks marked 2 inline comments as done.Thu, Dec 6, 4:54 PM
brooks added inline comments.
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.

markj accepted this revision.Thu, Dec 6, 4:55 PM
This revision is now accepted and ready to land.Thu, Dec 6, 4:55 PM
gordon accepted this revision as: secteam.Thu, Dec 6, 5:05 PM
gordon added a subscriber: 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 added 1 blocking reviewer(s): transport.EditedThu, Dec 6, 6:38 PM
brooks added a subscriber: tj.

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

This revision now requires review to proceed.Thu, Dec 6, 6:38 PM
brooks added a subscriber: thj.Thu, Dec 6, 6:45 PM
bz added a reviewer: bz.Thu, Dec 13, 5:23 PM
bz added a subscriber: bz.

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

bz accepted this revision.Fri, Dec 14, 8:22 PM
This revision is now accepted and ready to land.Fri, Dec 14, 8:22 PM
This revision was automatically updated to reflect the committed changes.