[ovs-dev] [PATCH branch-2.6] conntrack: Fix ICMPv4 error data L4 length check.
Darrell Ball
dlu998 at gmail.com
Sat Sep 28 18:45:59 UTC 2019
Thanks for doing this Vishal !
Except for minor patch formatting issues (inline), this is fine and also
tests fine.
On Fri, Sep 27, 2019 at 10:54 PM Vishal Deep Ajmera <
vishal.deep.ajmera at ericsson.com> wrote:
> From: Darrell Ball <dlu998 at gmail.com>
>
> 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 <diproiettod at ovn.org>
> Reported-at:
> https://mail.openvswitch.org/pipermail/ovs-dev/2019-August/361949.html
> Reported-by: Vishal Deep Ajmera <vishal.deep.ajmera at ericsson.com>
> Signed-off-by: Vishal Deep Ajmera <vishal.deep.ajmera at ericsson.com>
> Co-authored-by: Vishal Deep Ajmera <vishal.deep.ajmera at ericsson.com>
> Signed-off-by: Darrell Ball <dlu998 at gmail.com>
>
> Signed-off-by: Ben Pfaff <blp at ovn.org>
>
remove extra signoff when submitting the patch
> (cherry picked from commit 6c2a93064afe8d812e4506880d1fd8f96108f92a)
>
> Conflicts:
> lib/conntrack.c
>
move part starting from 'cherry' below the '---', although this info is not
really needed
> ---
> lib/conntrack.c | 35 ++++++++++++++++++++---------------
> lib/packets.h | 3 +++
> 2 files changed, 23 insertions(+), 15 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 8abaf7e..d59083e 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -664,11 +664,12 @@ 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)
> {
> const struct tcp_header *tcp = data;
>
> - if (OVS_UNLIKELY(size < TCP_HEADER_LEN)) {
> + if (OVS_UNLIKELY(size < (chk_len ? *chk_len : TCP_HEADER_LEN))) {
> return false;
> }
>
> @@ -680,11 +681,12 @@ 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)
> {
> const struct udp_header *udp = data;
>
> - if (OVS_UNLIKELY(size < UDP_HEADER_LEN)) {
> + if (OVS_UNLIKELY(size < (chk_len ? *chk_len : UDP_HEADER_LEN))) {
> return false;
> }
>
> @@ -696,7 +698,8 @@ 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);
> + size_t size, bool *related, const void *l3,
> + size_t *chk_len);
>
> static uint8_t
> reverse_icmp_type(uint8_t type)
> @@ -728,11 +731,11 @@ 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)
> {
> const struct icmp_header *icmp = data;
>
> - if (OVS_UNLIKELY(size < ICMP_HEADER_LEN)) {
> + if (OVS_UNLIKELY(size < (chk_len ? *chk_len : ICMP_HEADER_LEN))) {
> return false;
> }
>
> @@ -783,8 +786,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);
> + ok = extract_l4(key, l4, tail - l4, NULL, l3, &check_len);
> if (ok) {
> conn_key_reverse(key);
> *related = true;
> @@ -872,7 +876,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);
> + ok = extract_l4(key, l4, tail - l4, NULL, l3, NULL);
> if (ok) {
> conn_key_reverse(key);
> *related = true;
> @@ -897,21 +901,22 @@ 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)
> + const void *l3, size_t *chk_len)
> {
> if (key->nw_proto == IPPROTO_TCP) {
> return (!related || check_l4_tcp(key, data, size, l3))
> - && extract_l4_tcp(key, data, size);
> + && extract_l4_tcp(key, data, size, chk_len);
> } else if (key->nw_proto == IPPROTO_UDP) {
> return (!related || check_l4_udp(key, data, size, l3))
> - && extract_l4_udp(key, data, size);
> + && 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))
> - && 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))
> @@ -982,7 +987,7 @@ conn_key_extract(struct conntrack *ct, struct
> dp_packet *pkt, ovs_be16 dl_type,
>
> if (ok) {
> if (extract_l4(&ctx->key, l4, dp_packet_l4_size(pkt),
> - &ctx->related, l3)) {
> + &ctx->related, l3, NULL)) {
> ctx->hash = conn_key_hash(&ctx->key, ct->hash_basis);
> return true;
> }
> diff --git a/lib/packets.h b/lib/packets.h
> index 0705576..adfa86c 100644
> --- a/lib/packets.h
> +++ b/lib/packets.h
> @@ -656,6 +656,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;
> --
> 1.9.1
>
>
More information about the dev
mailing list