Skip to content

Commit

Permalink
conntrack: Fix ICMPv4 error data L4 length check.
Browse files Browse the repository at this point in the history 
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: a489b16 ("conntrack: New userspace connection tracker.")
CC: Daniele Di Proietto <diproiettod@ovn.org>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2019-August/361949.html
Reported-by: Vishal Deep Ajmera <vishal.deep.ajmera@ericsson.com>
Signed-off-by: Vishal Deep Ajmera <vishal.deep.ajmera@ericsson.com>
Co-authored-by: Vishal Deep Ajmera <vishal.deep.ajmera@ericsson.com>
Signed-off-by: Darrell Ball <dlu998@gmail.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
  • Loading branch information
@darball1 @vishaldeepajmera @blp
2 people authored and blp committed Sep 23, 2019
1 parent dea2eb8 commit 14c43d9
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 17 deletions.
41 changes: 24 additions & 17 deletions lib/conntrack.c
View file
Expand Up @@ -1715,9 +1715,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;
}

Expand All @@ -1730,9 +1731,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;
}

Expand All @@ -1746,7 +1748,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)
Expand Down Expand Up @@ -1778,9 +1780,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;
}

Expand Down Expand Up @@ -1831,8 +1833,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;
Expand Down Expand Up @@ -1919,7 +1922,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;
Expand All @@ -1944,26 +1947,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;
}
Expand Down Expand Up @@ -2042,7 +2048,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;
}
Expand Down
3 changes: 3 additions & 0 deletions lib/packets.h
View file
Expand Up @@ -744,6 +744,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;
Expand Down

0 comments on commit 14c43d9

Please sign in to comment.