From patchwork Tue Aug 27 23:59:02 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Darrell Ball X-Patchwork-Id: 1154120 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=openvswitch.org (client-ip=140.211.169.12; helo=mail.linuxfoundation.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="mijOmRpk"; dkim-atps=neutral Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 46J5Sp1Wkkz9sBp for ; Wed, 28 Aug 2019 09:59:21 +1000 (AEST) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 98D2D1E2D; Tue, 27 Aug 2019 23:59:18 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id E104C1E18 for ; Tue, 27 Aug 2019 23:59:16 +0000 (UTC) X-Greylist: whitelisted by SQLgrey-1.7.6 Received: from mail-pf1-f196.google.com (mail-pf1-f196.google.com [209.85.210.196]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 4E0C38A9 for ; Tue, 27 Aug 2019 23:59:16 +0000 (UTC) Received: by mail-pf1-f196.google.com with SMTP id g2so446509pfq.0 for ; Tue, 27 Aug 2019 16:59:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id; bh=1OHbpEo3RsMZpEaAhuEsXQ80otgYL/+/pbI7+iOvUXE=; b=mijOmRpku30HdFRt7/hfwNd0Lt/GjzASKmKjrmiiMqaEQfy1t9LZRYzBb3oDTUJdJ2 5GTdA81QBPE1f4xauqu61Gk0G7pdeQ5q6LnvJJZNsas7VfAXKVYCxlmAi8jz/xdAbHa7 BIGRSUwivjUTqzTDI4tcoMaGQb0kdJSDJImWHFSFLrOJ5QUvtuwDdkO9h/SavTtVRF8A UPBKCVJWyULURKHACBgSzG9rjmdcpByGYdAQM3KYW1Qj8u26b9LWwPOgN4FDlqMNNiGF 9R2tz7vJ0iZFMyu4ZP+DpuFUGdybGARFQBZhUCj2RMI5SyxZnzYG1TKtovOC57kwdL36 61Ow== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=1OHbpEo3RsMZpEaAhuEsXQ80otgYL/+/pbI7+iOvUXE=; b=Ku3RD8tIIJvETHA2qcjTVGRdvOBp20YPTRt+CVRF3ct7SJD8S7OLRbNGZSpj/trBVm VrO4pgsbS8YTfpaQnqf58j+XlX9snHuJN+qASDVCagfH+imjpU/AAE6iAAn8t7Kz/Nar oAp8pDilMvJDWARQRkJoUpZ9ruD4HafscOO119GCfKaNcWNUCUIeA/ZoxqAjh0xlE2dV mK9fNOqaMHvG3nyvhmP4yUkJ2ni/oImItoLnnUO7GbV8Q6YtaKdZFzaWpEUCeZv7+GTr fQ8Ezmz5EZn2oY/7CaPb7SenAHRFe/vHaaGDVcGnet5rxl9XyADYKljBJq6ZfKfwhxi8 7U0A== X-Gm-Message-State: APjAAAVnCwRWdgsNzCBoOr+f7GrCv7jrtxSdkPixU5tg3ePLHTGm+tcP aQtogeHs2LIx7anNoLB1tQc= X-Google-Smtp-Source: APXvYqwc+CVw54y+CR2QEOzA2zfjVPk1T0D28tMto+bXz871hk/X+dRwwlEFlYysPvCexRW+5wBSGA== X-Received: by 2002:a63:2006:: with SMTP id g6mr917728pgg.287.1566950355621; Tue, 27 Aug 2019 16:59:15 -0700 (PDT) Received: from ubuntu.localdomain ([66.170.99.2]) by smtp.gmail.com with ESMTPSA id j9sm451030pfi.128.2019.08.27.16.59.14 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Tue, 27 Aug 2019 16:59:14 -0700 (PDT) From: Darrell Ball To: dlu998@gmail.com, dev@openvswitch.org Date: Tue, 27 Aug 2019 16:59:02 -0700 Message-Id: <1566950342-51171-1-git-send-email-dlu998@gmail.com> X-Mailer: git-send-email 1.9.1 X-Spam-Status: No, score=-1.7 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FROM, RCVD_IN_DNSWL_NONE autolearn=no version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Subject: [ovs-dev] [patch v2] conntrack: Fix ICMPv4 error data L4 length check. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org The ICMPv4 error data L4 length check was found to be too strict for TCP, expecting a minimum of 20 rather than 8 bytes. This worked by hapenstance for other inner protocols. The approach is to explicitly handle the ICMPv4 error data L4 length check and to do this for all supported inner protocols in the same way. Making the code common between protocols also allows the existing ICMPv4 related UDP tests to cover TCP and ICMP inner protocol cases. Note that ICMPv6 does not have an 8 byte limit for error L4 data. Fixes: a489b16854b5 ("conntrack: New userspace connection tracker.") CC: Daniele Di Proietto Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2019-August/361949.html Reported-by: Vishal Deep Ajmera Signed-off-by: Vishal Deep Ajmera Co-authored-by: Vishal Deep Ajmera Signed-off-by: Darrell Ball --- v2: Rebase to fix git applying to wrong function (extract_l4_icmp6 vs extract_l4_icmp) with same match signature. Minor fix to related comment in extract_l4(). lib/conntrack.c | 41 ++++++++++++++++++++++++----------------- lib/packets.h | 3 +++ 2 files changed, 27 insertions(+), 17 deletions(-) diff --git a/lib/conntrack.c b/lib/conntrack.c index 5f60fea..e5266e5 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -1514,9 +1514,10 @@ check_l4_icmp6(const struct conn_key *key, const void *data, size_t size, } static inline bool -extract_l4_tcp(struct conn_key *key, const void *data, size_t size) +extract_l4_tcp(struct conn_key *key, const void *data, size_t size, + size_t *chk_len) { - if (OVS_UNLIKELY(size < TCP_HEADER_LEN)) { + if (OVS_UNLIKELY(size < (chk_len ? *chk_len : TCP_HEADER_LEN))) { return false; } @@ -1529,9 +1530,10 @@ extract_l4_tcp(struct conn_key *key, const void *data, size_t size) } static inline bool -extract_l4_udp(struct conn_key *key, const void *data, size_t size) +extract_l4_udp(struct conn_key *key, const void *data, size_t size, + size_t *chk_len) { - if (OVS_UNLIKELY(size < UDP_HEADER_LEN)) { + if (OVS_UNLIKELY(size < (chk_len ? *chk_len : UDP_HEADER_LEN))) { return false; } @@ -1545,7 +1547,7 @@ extract_l4_udp(struct conn_key *key, const void *data, size_t size) static inline bool extract_l4(struct conn_key *key, const void *data, size_t size, bool *related, const void *l3, - bool validate_checksum); + bool validate_checksum, size_t *chk_len); static uint8_t reverse_icmp_type(uint8_t type) @@ -1577,9 +1579,9 @@ reverse_icmp_type(uint8_t type) * possible */ static inline int extract_l4_icmp(struct conn_key *key, const void *data, size_t size, - bool *related) + bool *related, size_t *chk_len) { - if (OVS_UNLIKELY(size < ICMP_HEADER_LEN)) { + if (OVS_UNLIKELY(size < (chk_len ? *chk_len : ICMP_HEADER_LEN))) { return false; } @@ -1630,8 +1632,9 @@ extract_l4_icmp(struct conn_key *key, const void *data, size_t size, key->src = inner_key.src; key->dst = inner_key.dst; key->nw_proto = inner_key.nw_proto; + size_t check_len = ICMP_ERROR_DATA_L4_LEN; - ok = extract_l4(key, l4, tail - l4, NULL, l3, false); + ok = extract_l4(key, l4, tail - l4, NULL, l3, false, &check_len); if (ok) { conn_key_reverse(key); *related = true; @@ -1718,7 +1721,7 @@ extract_l4_icmp6(struct conn_key *key, const void *data, size_t size, key->dst = inner_key.dst; key->nw_proto = inner_key.nw_proto; - ok = extract_l4(key, l4, tail - l4, NULL, l3, false); + ok = extract_l4(key, l4, tail - l4, NULL, l3, false, NULL); if (ok) { conn_key_reverse(key); *related = true; @@ -1743,26 +1746,29 @@ extract_l4_icmp6(struct conn_key *key, const void *data, size_t size, * an ICMP or ICMP6 header. * * If 'related' is NULL, it means that we're already parsing a header nested - * in an ICMP error. In this case, we skip checksum and length validation. */ + * in an ICMP error. In this case, we skip the checksum and some length + * validations. */ static inline bool extract_l4(struct conn_key *key, const void *data, size_t size, bool *related, - const void *l3, bool validate_checksum) + const void *l3, bool validate_checksum, size_t *chk_len) { if (key->nw_proto == IPPROTO_TCP) { return (!related || check_l4_tcp(key, data, size, l3, - validate_checksum)) && extract_l4_tcp(key, data, size); + validate_checksum)) + && extract_l4_tcp(key, data, size, chk_len); } else if (key->nw_proto == IPPROTO_UDP) { return (!related || check_l4_udp(key, data, size, l3, - validate_checksum)) && extract_l4_udp(key, data, size); + validate_checksum)) + && extract_l4_udp(key, data, size, chk_len); } else if (key->dl_type == htons(ETH_TYPE_IP) && key->nw_proto == IPPROTO_ICMP) { return (!related || check_l4_icmp(data, size, validate_checksum)) - && extract_l4_icmp(key, data, size, related); + && extract_l4_icmp(key, data, size, related, chk_len); } else if (key->dl_type == htons(ETH_TYPE_IPV6) && key->nw_proto == IPPROTO_ICMPV6) { return (!related || check_l4_icmp6(key, data, size, l3, - validate_checksum)) && extract_l4_icmp6(key, data, size, - related); + validate_checksum)) + && extract_l4_icmp6(key, data, size, related); } else { return false; } @@ -1841,7 +1847,8 @@ conn_key_extract(struct conntrack *ct, struct dp_packet *pkt, ovs_be16 dl_type, bool hwol_good_l4_csum = dp_packet_l4_checksum_valid(pkt); /* Validate the checksum only when hwol is not supported. */ if (extract_l4(&ctx->key, l4, dp_packet_l4_size(pkt), - &ctx->icmp_related, l3, !hwol_good_l4_csum)) { + &ctx->icmp_related, l3, !hwol_good_l4_csum, + NULL)) { ctx->hash = conn_key_hash(&ctx->key, ct->hash_basis); return true; } diff --git a/lib/packets.h b/lib/packets.h index a4bee38..c440098 100644 --- a/lib/packets.h +++ b/lib/packets.h @@ -769,6 +769,9 @@ struct icmp_header { }; BUILD_ASSERT_DECL(ICMP_HEADER_LEN == sizeof(struct icmp_header)); +/* ICMPV4 */ +#define ICMP_ERROR_DATA_L4_LEN 8 + #define IGMP_HEADER_LEN 8 struct igmp_header { uint8_t igmp_type;