Changeset View
Standalone View
sys/netinet/ip_icmp.c
Context not available. | ||||||||||
&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_curr_inc) = 0; | ||||||||||
#define V_icmplim_curr_inc VNET(icmplim_curr_inc) | ||||||||||
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… | ||||||||||
VNET_DEFINE_STATIC(int, icmplim_inc) = 16; | ||||||||||
#define V_icmplim_inc VNET(icmplim_inc) | ||||||||||
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… | ||||||||||
SYSCTL_INT(_net_inet_icmp, OID_AUTO, icmplim_inc, CTLFLAG_VNET | CTLFLAG_RW, | ||||||||||
&VNET_NAME(icmplim_inc), 0, | ||||||||||
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… | ||||||||||
"Increment value for randomizing the number of ICMP responses per second"); | ||||||||||
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… | ||||||||||
VNET_DEFINE_STATIC(int, icmplim_output) = 1; | VNET_DEFINE_STATIC(int, icmplim_output) = 1; | |||||||||
#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, | |||||||||
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. :) | ||||||||||
Context not available. | ||||||||||
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); | pps = counter_ratecheck(&V_icmp_rates[which].cr, V_icmplim + V_icmplim_curr_inc); | |||||||||
if (pps == -1) | ||||||||||
return (-1); | if (pps == -1) { | |||||||||
Done Inline ActionsSpurious whitespace here. philip: Spurious whitespace here. | ||||||||||
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… | ||||||||||
/* | ||||||||||
* Adjust limit +/- to jitter the measurement for security. | ||||||||||
* CVE-2020-25705 | ||||||||||
*/ | ||||||||||
if (V_icmplim_inc > 0) { | ||||||||||
int32_t inc = arc4random_uniform(V_icmplim_inc); | ||||||||||
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… | ||||||||||
if (inc % 2) | ||||||||||
V_icmplim_curr_inc = -inc; | ||||||||||
else | ||||||||||
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… | ||||||||||
V_icmplim_curr_inc = 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… | ||||||||||
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 > 0 && V_icmplim_output) | if (pps > 0 && V_icmplim_output) | |||||||||
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 + V_icmplim_curr_inc); | |||||||||
return (0); | return (0); | |||||||||
} | } | |||||||||
Context not available. |
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.