On 14 April 2017 at 14:26, Jarno Rajahalme <jarno@xxxxxxx> wrote:
> Conntrack helpers do not check for a potentially clashing conntrack
> entry when creating a new expectation. Also, nf_conntrack_in() will
> check expectations (via init_conntrack()) only if a conntrack entry
> can not be found. The expectation for a packet which also matches an
> existing conntrack entry will not be removed by conntrack, and is
> currently handled inconsistently by OVS, as OVS expects the
> expectation to be removed when the connection tracking entry matching
> that expectation is confirmed.
>
> It should be noted that normally an IP stack would not allow reuse of
> a 5-tuple of an old (possibly lingering) connection for a new data
> connection, so this is somewhat unlikely corner case. However, it is
> possible that a misbehaving source could cause conntrack entries be
> created that could then interfere with new related connections.
>
> Fix this in the OVS module by deleting the clashing conntrack entry
> after an expectation has been matched. This causes the following
> nf_conntrack_in() call also find the expectation and remove it when
> creating the new conntrack entry, as well as the forthcoming reply
> direction packets to match the new related connection instead of the
> old clashing conntrack entry.
>
> Fixes: 7f8a436eaa2c ("openvswitch: Add conntrack action")
> Reported-by: Yang Song <yangsong@xxxxxxxxxx>
> Signed-off-by: Jarno Rajahalme <jarno@xxxxxxx>
> ---
Hi Jarno,
> v3: Removed unnecessary if statement.
> v2: Fixed commit title.
>
> net/openvswitch/conntrack.c | 31 ++++++++++++++++++++++++++++++-
> 1 file changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index 7b2c2fc..d796ae7 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -514,10 +514,39 @@ ovs_ct_expect_find(struct net *net, const struct nf_conntrack_zone *zone,
> u16 proto, const struct sk_buff *skb)
> {
> struct nf_conntrack_tuple tuple;
> + struct nf_conntrack_expect *exp;
>
> if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb), proto, net, &tuple))
> return NULL;
> - return __nf_ct_expect_find(net, zone, &tuple);
> +
> + exp = __nf_ct_expect_find(net, zone, &tuple);
> +
Extraneous whitespace^
> + if (exp) {
> + struct nf_conntrack_tuple_hash *h;
> +
> + /* Delete existing conntrack entry, if it clashes with the
> + * expectation. This can happen since conntrack ALGs do not
> + * check for clashes between (new) expectations and existing
> + * conntrack entries. nf_conntrack_in() will check the
> + * expectations only if a conntrack entry can not be found,
> + * which can lead to OVS finding the expectation (here) in the
> + * init direction, but which will not be removed by the
> + * nf_conntrack_in() call, if a matching conntrack entry is
> + * found instead. In this case all init direction packets
> + * would be reported as new related packets, while reply
> + * direction packets would be reported as un-related
> + * established packets. */
> +
Extraneous whitespace^
> + h = nf_conntrack_find_get(net, zone, &tuple);
> + if (h) {
> + struct nf_conn *ct = nf_ct_tuplehash_to_ctrack(h);
> +
> + nf_ct_delete(ct, 0, 0);
> + nf_conntrack_put(&ct->ct_general);
Do we need the extra nf_conntrack_put() here? If
nf_conntrack_find_get() returns an entry, we'll call nf_ct_delete()
which releases a reference on the CT entry.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
