Bug 172675 - [netinet] [patch] sysctl_tcp_hc_list (net.inet.tcp.hostcache.list) race condition causing memory corruption
Summary: [netinet] [patch] sysctl_tcp_hc_list (net.inet.tcp.hostcache.list) race condi...
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 8.3-RELEASE
Hardware: Any Any
: Normal Affects Some People
Assignee: freebsd-net (Nobody)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-13 23:00 UTC by Jason Wolfe
Modified: 2015-04-09 20:19 UTC (History)
2 users (show)

See Also:


Attachments
file.diff (2.38 KB, patch)
2012-10-13 23:00 UTC, Jason Wolfe
no flags Details | Diff
sbuf_hc_list.patch (2.10 KB, patch)
2015-01-13 20:58 UTC, John Baldwin
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jason Wolfe 2012-10-13 23:00:00 UTC
It appears that in sysctl_tcp_hc_list, the function used for 'sysctl net.inet.tcp.hostcache.list', there is a somewhat rare chance of cache_count being lower than the actual number of entries.  Not sure if this is happening by a race condition after execution time, or if the value just isn't updated constantly enough.  On heavily loaded boxes (1000+ more or less unique req/s) it's not too tough to cause memory corruption and a panic running 'sysctl net.inet.tcp.hostcache.list' with the current code.

A colleague of mine had spotted the issue, and believes this patch would do the trick.  I've been testing it by simply running the hostcache.list in a loop, and have had success where prior it would have caused a panic in fairly short order.

Patch by: Nils McCarthy <nils@shkoo.com>

Fix: Possible patch supplied

Patch attached with submission follows:
How-To-Repeat: Run sysctl net.inet.tcp.hostcache.list continuously.
Comment 1 Mark Linimon freebsd_committer freebsd_triage 2012-10-13 23:26:12 UTC
Responsible Changed
From-To: freebsd-bugs->freebsd-net

Over to maintainer(s).
Comment 2 jason 2013-06-08 09:11:21 UTC
It appears this was committed to HEAD verbatim in r241735.  Not sure who 
'az' is though :)
Comment 3 jason 2013-06-08 10:20:48 UTC
My mistake, this is still an open issue, only the small RTT value patch 
of this diff is live.

-                            (RTM_RTTUNIT / (hz * TCP_RTT_SCALE))),
+                                (RTM_RTTUNIT / (hz * TCP_RTTVAR_SCALE))),
Comment 4 Anton Yuzhaninov 2014-12-17 17:26:56 UTC
This typo was commited in r238083
Comment 5 Anton Yuzhaninov 2014-12-17 17:31:41 UTC
Race in cache_count change is still not fixed after more than two years :(

atomic_add_int / atomic_subtract_int is expensive operations. Better to use counter(9) API.
Comment 6 John Baldwin freebsd_committer freebsd_triage 2015-01-13 20:58:07 UTC
Created attachment 151584 [details]
sbuf_hc_list.patch

It is true that cache_count updates are very racy, and it might be worth fixing them to be atomic.  However, those would not prevent the corruption.  If the cache grew during the sysctl even with the atomic ops you could still overflow the buffer.

I started out by adding explicit checks of 'p - buf >= bufsize' to break out of the loop.  However, the sbuf(9) API already provides a nice way to handle variable output into a fixed size buffer while handling overflow correctly, etc.  I've attached a patch which takes this route and converts the sysctl to use an sbuf instead.  Please test.
Comment 7 commit-hook freebsd_committer freebsd_triage 2015-01-25 19:46:12 UTC
A commit references this bug:

Author: jhb
Date: Sun Jan 25 19:45:45 UTC 2015
New revision: 277709
URL: https://svnweb.freebsd.org/changeset/base/277709

Log:
  Use an sbuf to generate the output of the net.inet.tcp.hostcache.list
  sysctl to avoid a possible buffer overflow if the cache grows while the
  text is being generated.

  PR:		172675
  MFC after:	2 weeks

Changes:
  head/sys/netinet/tcp_hostcache.c
Comment 8 commit-hook freebsd_committer freebsd_triage 2015-02-10 21:42:37 UTC
A commit references this bug:

Author: jhb
Date: Tue Feb 10 21:41:57 UTC 2015
New revision: 278534
URL: https://svnweb.freebsd.org/changeset/base/278534

Log:
  MFC 277709:
  Use an sbuf to generate the output of the net.inet.tcp.hostcache.list
  sysctl to avoid a possible buffer overflow if the cache grows while the
  text is being generated.

  PR:		172675

Changes:
_U  stable/10/
  stable/10/sys/netinet/tcp_hostcache.c
_U  stable/9/sys/
  stable/9/sys/netinet/tcp_hostcache.c
Comment 9 Jason Wolfe 2015-04-09 20:19:46 UTC
John's fix was brought to 9 / 10, where I can confirm it works as intended.  Thanks!