Re: [PATCH v2] netfilter: conntrack: clamp timeouts to INT_MAX

[Florian Westphal <fw@strlen.de>]

Jay Elliott <jelliott@arista.com> wrote:
> On Sat, Nov 11, 2017 at 10:27 AM, Florian Westphal <fw@strlen.de> wrote:
> > It also looks wrong.
> > let ct->timeout be 1000.
> > let nfct_time_stamp be 0x80000000
> >
> > Then ct->timout is capped to 0x7fffffff.
> > Next check considers the timeout to be expired, as 0x7fff... - 0x800 < 0.
> 
> Thanks for pointing that out; it does look like something that could
> cause troubles.
> 
> Is it alright if I submit a fix to this as a separate patch?  I
> *think* I have a solution (pending some testing), but I also think
> it's outside of the scope of this commit since it's a pre-existing
> problem so I'd like to fix it separately.

Sorry, I am not following.  This problem is added with this patch.

> > So I guess best bet is to actually do a 64bit multiplication, as you
> > did, then truncate.
> >
> > Please use u64 for this (the u_intXX_t types are prehistoric leftovers).
> 
> So to clarify, are changing the u_int64_t variables to u64 and fixing
> the case where nfct_time_stamp >= 0x8000... the only changes that need
> to be made based on the v2 patch I sent out?

Yes, I think so, only changes in nfnetlink.c are needed, i.e. (totally untested):

-       u_int32_t timeout = ntohl(nla_get_be32(cda[CTA_TIMEOUT]));
+       u64 timeout = ntohl(nla_get_be32(cda[CTA_TIMEOUT])) * HZ;
+
+	if (timeout > INT_MAX)
		timeout = INT_MAX;

-	ct->timeout = nfct_time_stamp + timeout * HZ;
+	ct->timeout = nfct_time_stamp + (u32)timeout;

        if (test_bit(IPS_DYING_BIT, &ct->status))
                return -ETIME;
@@ -1762,6 +1765,8 @@ 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_nla;

        ct = nf_conntrack_alloc(net, zone, otuple, rtuple, GFP_ATOMIC);
        if (IS_ERR(ct))
@@ -1770,7 +1775,11 @@ 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_nla = ntohl(nla_get_be32(cda[CTA_TIMEOUT])) * HZ;
+       if (timeout_nla > INT_MAX)
+               timeout_nla = INT_MAX;
+       ct->timeout = nfct_time_stamp + timeout_nla;