Changeset View
Standalone View
sys/netinet/ip_icmp.c
Show First 20 Lines • Show All 83 Lines • ▼ Show 20 Lines | ||||||||||
* host table maintenance routines. | * host table maintenance routines. | |||||||||
*/ | */ | |||||||||
VNET_DEFINE_STATIC(int, icmplim) = 200; | VNET_DEFINE_STATIC(int, icmplim) = 200; | |||||||||
#define V_icmplim VNET(icmplim) | #define V_icmplim VNET(icmplim) | |||||||||
SYSCTL_INT(_net_inet_icmp, ICMPCTL_ICMPLIM, icmplim, CTLFLAG_VNET | CTLFLAG_RW, | SYSCTL_INT(_net_inet_icmp, ICMPCTL_ICMPLIM, icmplim, CTLFLAG_VNET | CTLFLAG_RW, | |||||||||
&VNET_NAME(icmplim), 0, | &VNET_NAME(icmplim), 0, | |||||||||
"Maximum number of ICMP responses per second"); | "Maximum number of ICMP responses per second"); | |||||||||
VNET_DEFINE_STATIC(int, icmplim_inc) = 3; | ||||||||||
cperciva: We probably want this to be 3 (so ICMPs are counted as 0, 1, or 2 units towards the limit). | ||||||||||
Done Inline ActionsWe could multiply the limit by (V_icmplim_jitter / 2) to keep the average number of packets approximately the same as the limit, regardless of how icmplim_jitter is adjusted? cem: We could multiply the limit by `(V_icmplim_jitter / 2)` to keep the average number of packets… | ||||||||||
#define V_icmplim_inc VNET(icmplim_inc) | ||||||||||
SYSCTL_INT(_net_inet_icmp, ICMPCTL_ICMPLIM, icmplim_inc, CTLFLAG_VNET | CTLFLAG_RW, | ||||||||||
Not Done Inline ActionsI think ICMPCTL_ICMPLIM needs to be OID_AUTO, because we already use ICMPCTL_ICMPLIM for icmplim itself. Without that change the kernel reports "sysctl: OID number(3) is already in use for 'icmplim_inc'" kp: I think ICMPCTL_ICMPLIM needs to be OID_AUTO, because we already use ICMPCTL_ICMPLIM for… | ||||||||||
&VNET_NAME(icmplim_inc), 0, | ||||||||||
"Increment value for randomizing the number of ICMP responses per second"); | ||||||||||
Done Inline ActionsMaybe "number of responses per second" rather than "bandwidth limit" to be consistent with icmplim, above? philip: Maybe "number of responses per second" rather than "bandwidth limit" to be consistent with… | ||||||||||
VNET_DEFINE_STATIC(int, icmplim_output) = 1; | VNET_DEFINE_STATIC(int, icmplim_output) = 1; | |||||||||
Not Done Inline Actions
This seems unusual in the way sysctls are typically documented, maybe "Random icmplim jitter adjustment limit" or something like that? emaste: This seems unusual in the way sysctls are typically documented, maybe "Random icmplim jitter… | ||||||||||
#define V_icmplim_output VNET(icmplim_output) | #define V_icmplim_output VNET(icmplim_output) | |||||||||
SYSCTL_INT(_net_inet_icmp, OID_AUTO, icmplim_output, CTLFLAG_VNET | CTLFLAG_RW, | SYSCTL_INT(_net_inet_icmp, OID_AUTO, icmplim_output, CTLFLAG_VNET | CTLFLAG_RW, | |||||||||
&VNET_NAME(icmplim_output), 0, | &VNET_NAME(icmplim_output), 0, | |||||||||
"Enable logging of ICMP response rate limiting"); | "Enable logging of ICMP response rate limiting"); | |||||||||
#ifdef INET | #ifdef INET | |||||||||
VNET_PCPUSTAT_DEFINE(struct icmpstat, icmpstat); | VNET_PCPUSTAT_DEFINE(struct icmpstat, icmpstat); | |||||||||
VNET_PCPUSTAT_SYSINIT(icmpstat); | VNET_PCPUSTAT_SYSINIT(icmpstat); | |||||||||
▲ Show 20 Lines • Show All 988 Lines • ▼ Show 20 Lines | VNET_DEFINE_STATIC(struct icmp_rate, icmp_rates[BANDLIM_MAX]) = { | |||||||||
{ "icmp tstamp response" }, | { "icmp tstamp response" }, | |||||||||
{ "closed port RST response" }, | { "closed port RST response" }, | |||||||||
{ "open port RST response" }, | { "open port RST response" }, | |||||||||
{ "icmp6 unreach response" }, | { "icmp6 unreach response" }, | |||||||||
{ "sctp ootb response" } | { "sctp ootb response" } | |||||||||
}; | }; | |||||||||
#define V_icmp_rates VNET(icmp_rates) | #define V_icmp_rates VNET(icmp_rates) | |||||||||
/* | ||||||||||
* Adjust limit +/- to jitter the measurement for security. | ||||||||||
* CVE-2020-25705 | ||||||||||
*/ | ||||||||||
Done Inline ActionsWe should remember to adjust this comment when the paper is published. :) philip: We should remember to adjust this comment when the paper is published. :) | ||||||||||
static void | static void | |||||||||
icmp_bandlimit_init(void) | icmp_bandlimit_init(void) | |||||||||
{ | { | |||||||||
for (int i = 0; i < BANDLIM_MAX; i++) { | for (int i = 0; i < BANDLIM_MAX; i++) { | |||||||||
V_icmp_rates[i].cr.cr_rate = counter_u64_alloc(M_WAITOK); | V_icmp_rates[i].cr.cr_rate = counter_u64_alloc(M_WAITOK); | |||||||||
V_icmp_rates[i].cr.cr_ticks = ticks; | V_icmp_rates[i].cr.cr_ticks = ticks; | |||||||||
} | } | |||||||||
Show All 10 Lines | ||||||||||
} | } | |||||||||
VNET_SYSUNINIT(icmp_bandlimit, SI_SUB_PROTO_DOMAIN, SI_ORDER_THIRD, | VNET_SYSUNINIT(icmp_bandlimit, SI_SUB_PROTO_DOMAIN, SI_ORDER_THIRD, | |||||||||
icmp_bandlimit_uninit, NULL); | icmp_bandlimit_uninit, NULL); | |||||||||
int | int | |||||||||
badport_bandlim(int which) | badport_bandlim(int which) | |||||||||
{ | { | |||||||||
int64_t pps; | int64_t pps; | |||||||||
int32_t inc; | ||||||||||
if (V_icmplim == 0 || which == BANDLIM_UNLIMITED) | if (V_icmplim == 0 || which == BANDLIM_UNLIMITED) | |||||||||
Done Inline ActionsSpurious whitespace here. philip: Spurious whitespace here. | ||||||||||
return (0); | return (0); | |||||||||
KASSERT(which >= 0 && which < BANDLIM_MAX, | KASSERT(which >= 0 && which < BANDLIM_MAX, | |||||||||
("%s: which %d", __func__, which)); | ("%s: which %d", __func__, which)); | |||||||||
pps = counter_ratecheck(&V_icmp_rates[which].cr, V_icmplim); | /* | |||||||||
* Add random jitter to the administratively set limit. | ||||||||||
*/ | ||||||||||
inc = arc4random_uniform(V_icmplim_inc); | ||||||||||
pps = counter_ratecheck(&V_icmp_rates[which].cr, V_icmplim, inc); | ||||||||||
Done Inline ActionsHow frequently is badport_bandlim() called, and do the jitter numbers need to be unpredictable? If a non-cryptographic PRNG would suffice, prng32_bounded() might be a suitable replacement with less CPU use. cem: How frequently is `badport_bandlim()` called, and do the jitter numbers need to be… | ||||||||||
cyUnsubmitted Done Inline Actionscounter_ratecheck() in ys/kern/subr_counter.c takes only two arguments. cy: counter_ratecheck() in ys/kern/subr_counter.c takes only two arguments. | ||||||||||
if (pps == -1) | if (pps == -1) | |||||||||
return (-1); | return (-1); | |||||||||
if (pps > 0 && V_icmplim_output) | if (pps > 0 && V_icmplim_output) | |||||||||
Not Done Inline Actionscounter_ratecheck returns -1 for each over-limit packet in the interval, no? We'd want to pick a new value only once per I'd think? emaste: `counter_ratecheck` returns `-1` for each over-limit packet in the interval, no? We'd want to… | ||||||||||
log(LOG_NOTICE, "Limiting %s from %jd to %d packets/sec\n", | log(LOG_NOTICE, "Limiting %s from %jd to %d packets/sec\n", | |||||||||
V_icmp_rates[which].descr, (intmax_t )pps, V_icmplim); | V_icmp_rates[which].descr, (intmax_t )pps, V_icmplim); | |||||||||
Not Done Inline ActionsSomeone could change the icmplim sysctl after this calculation happens; maybe we should clamp at the counter_ratecheck call itself? emaste: Someone could change the icmplim sysctl after this calculation happens; maybe we should clamp… | ||||||||||
return (0); | return (0); | |||||||||
} | } | |||||||||
Not Done Inline Actionswhat about arc4random_uniform(V_icmplim_inc * 2 +1) - V_icmplim_inc instead avoiding the need for the % 2 special cases? (also adding appropriate limits) emaste: what about `arc4random_uniform(V_icmplim_inc * 2 +1) - V_icmplim_inc` instead avoiding the need… |
We probably want this to be 3 (so ICMPs are counted as 0, 1, or 2 units towards the limit). Making it 16 would dramatically cut the number of packets which get out before we hit the limit.