Message ID | 1510786873-60834-1-git-send-email-jelliott@arista.com |
---|---|
State | Accepted |
Delegated to: | Pablo Neira |
Headers | show |
Series | [v4] netfilter: conntrack: clamp timeouts to INT_MAX | expand |
On Wed, Nov 15, 2017 at 03:01:13PM -0800, Jay Elliott wrote: > When the conntracking code multiplies a timeout by HZ, it can overflow > from positive to negative; this causes it to instantly expire. To > protect against this the multiplication is done in 64-bit so we can > prevent it from exceeding INT_MAX. This is something you must be observing via conntrack utility or conntrackd? > Signed-off-by: Jay Elliott <jelliott@arista.com> > --- > net/netfilter/nf_conntrack_netlink.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c > index de4053d..41db7e3 100644 > --- a/net/netfilter/nf_conntrack_netlink.c > +++ b/net/netfilter/nf_conntrack_netlink.c > @@ -1560,9 +1560,11 @@ static int ctnetlink_change_helper(struct nf_conn *ct, > static int ctnetlink_change_timeout(struct nf_conn *ct, > const struct nlattr * const cda[]) > { > - u_int32_t timeout = ntohl(nla_get_be32(cda[CTA_TIMEOUT])); > + u64 timeout = (u64)ntohl(nla_get_be32(cda[CTA_TIMEOUT])) * HZ; > > - ct->timeout = nfct_time_stamp + timeout * HZ; > + if (timeout > INT_MAX) > + timeout = INT_MAX; > + ct->timeout = nfct_time_stamp + (u32)timeout; > > if (test_bit(IPS_DYING_BIT, &ct->status)) > return -ETIME; > @@ -1762,6 +1764,7 @@ static int change_seq_adj(struct nf_ct_seqadj *seq, > int err = -EINVAL; > struct nf_conntrack_helper *helper; > struct nf_conn_tstamp *tstamp; > + u64 timeout; > > ct = nf_conntrack_alloc(net, zone, otuple, rtuple, GFP_ATOMIC); > if (IS_ERR(ct)) > @@ -1770,7 +1773,10 @@ static int change_seq_adj(struct nf_ct_seqadj *seq, > if (!cda[CTA_TIMEOUT]) > goto err1; > > - ct->timeout = nfct_time_stamp + ntohl(nla_get_be32(cda[CTA_TIMEOUT])) * HZ; > + timeout = (u64)ntohl(nla_get_be32(cda[CTA_TIMEOUT])) * HZ; > + if (timeout > INT_MAX) > + timeout = INT_MAX; > + ct->timeout = (u32)timeout + nfct_time_stamp; > > rcu_read_lock(); > if (cda[CTA_HELP]) { > -- > 1.8.1.4 > -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Wed, Nov 15, 2017 at 03:01:13PM -0800, Jay Elliott wrote: > > When the conntracking code multiplies a timeout by HZ, it can overflow > > from positive to negative; this causes it to instantly expire. To > > protect against this the multiplication is done in 64-bit so we can > > prevent it from exceeding INT_MAX. > > This is something you must be observing via conntrack utility or > conntrackd? nf_ct_is_expired() will consider a ct as expired if the delta between current time and the (future) timeout is > 2**31. This triggers when ctnetlink configures a timeout larger than 2**31. The patch looks correct to me. Acked-by: Florian Westphal <fw@strlen.de> -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 28, 2017 at 01:12:44AM +0100, Florian Westphal wrote: > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > On Wed, Nov 15, 2017 at 03:01:13PM -0800, Jay Elliott wrote: > > > When the conntracking code multiplies a timeout by HZ, it can overflow > > > from positive to negative; this causes it to instantly expire. To > > > protect against this the multiplication is done in 64-bit so we can > > > prevent it from exceeding INT_MAX. > > > > This is something you must be observing via conntrack utility or > > conntrackd? > > nf_ct_is_expired() will consider a ct as expired if the delta > between current time and the (future) timeout is > 2**31. > > This triggers when ctnetlink configures a timeout larger than 2**31. > > The patch looks correct to me. > > Acked-by: Florian Westphal <fw@strlen.de> I'll pass this to net instead, this is likely breaking conntrackd. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c index de4053d..41db7e3 100644 --- a/net/netfilter/nf_conntrack_netlink.c +++ b/net/netfilter/nf_conntrack_netlink.c @@ -1560,9 +1560,11 @@ static int ctnetlink_change_helper(struct nf_conn *ct, static int ctnetlink_change_timeout(struct nf_conn *ct, const struct nlattr * const cda[]) { - u_int32_t timeout = ntohl(nla_get_be32(cda[CTA_TIMEOUT])); + u64 timeout = (u64)ntohl(nla_get_be32(cda[CTA_TIMEOUT])) * HZ; - ct->timeout = nfct_time_stamp + timeout * HZ; + if (timeout > INT_MAX) + timeout = INT_MAX; + ct->timeout = nfct_time_stamp + (u32)timeout; if (test_bit(IPS_DYING_BIT, &ct->status)) return -ETIME; @@ -1762,6 +1764,7 @@ static int change_seq_adj(struct nf_ct_seqadj *seq, int err = -EINVAL; struct nf_conntrack_helper *helper; struct nf_conn_tstamp *tstamp; + u64 timeout; ct = nf_conntrack_alloc(net, zone, otuple, rtuple, GFP_ATOMIC); if (IS_ERR(ct)) @@ -1770,7 +1773,10 @@ static int change_seq_adj(struct nf_ct_seqadj *seq, if (!cda[CTA_TIMEOUT]) goto err1; - ct->timeout = nfct_time_stamp + ntohl(nla_get_be32(cda[CTA_TIMEOUT])) * HZ; + timeout = (u64)ntohl(nla_get_be32(cda[CTA_TIMEOUT])) * HZ; + if (timeout > INT_MAX) + timeout = INT_MAX; + ct->timeout = (u32)timeout + nfct_time_stamp; rcu_read_lock(); if (cda[CTA_HELP]) {
When the conntracking code multiplies a timeout by HZ, it can overflow from positive to negative; this causes it to instantly expire. To protect against this the multiplication is done in 64-bit so we can prevent it from exceeding INT_MAX. Signed-off-by: Jay Elliott <jelliott@arista.com> --- net/netfilter/nf_conntrack_netlink.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)