linux-kernel - Re: [PATCH net] net: ipv4: do not handle duplicate fragments as overlapping

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <cb96bca9-1dda-b243-b581-91f1b51f1517@gmail.com>
Date:   Wed, 12 Dec 2018 22:20:42 -0800
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Michal Kubecek <mkubecek@...e.cz>,
        "David S. Miller" <davem@...emloft.net>
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        Peter Oskolkov <posk@...gle.com>,
        Eric Dumazet <eric.dumazet@...il.com>,
        Gustavo Figueira <gfigueira@...e.com>
Subject: Re: [PATCH net] net: ipv4: do not handle duplicate fragments as
 overlapping



On 12/12/2018 06:28 PM, Michal Kubecek wrote:
> Since commit 7969e5c40dfd ("ip: discard IPv4 datagrams with overlapping
> segments.") IPv4 reassembly code drops the whole queue whenever an
> overlapping fragment is received. However, the test is written in a way
> which detects duplicate fragments as overlapping so that in environments
> with many duplicate packets, fragmented packets may be undeliverable.
> 
> Add an extra test and for (potentially) duplicate fragment, only drop the
> new fragment rather than the whole queue. Only starting offset and length
> are checked, not the contents of the fragments as that would be too
> expensive.  Check for duplicity with last (tail) fragment first as in real
> life scenarios this should be the most frequent case and we would have to
> iterate through the whole "run" otherwise.
> 
> Fixes: 7969e5c40dfd ("ip: discard IPv4 datagrams with overlapping segments.")
> Signed-off-by: Michal Kubecek <mkubecek@...e.cz>
> ---
>  net/ipv4/ip_fragment.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
> index aa0b22697998..f09e3683b209 100644
> --- a/net/ipv4/ip_fragment.c
> +++ b/net/ipv4/ip_fragment.c
> @@ -436,6 +436,10 @@ static int ip_frag_queue(struct ipq *qp, struct sk_buff *skb)
>  			ip4_frag_append_to_last_run(&qp->q, skb);
>  		else
>  			ip4_frag_create_run(&qp->q, skb);
> +	} else if (offset == prev_tail->ip_defrag_offset &&
> +		   skb->len == prev_tail->len) {
> +		/* potential duplicate of last fragment */
> +		goto err;

What value is in @err variable at this point ?

Are you sure callers expect to receive -EINVAL ?



>  	} else {
>  		/* Binary search. Note that skb can become the first fragment,
>  		 * but not the last (covered above).
> @@ -449,8 +453,16 @@ static int ip_frag_queue(struct ipq *qp, struct sk_buff *skb)
>  			else if (offset >= skb1->ip_defrag_offset +
>  						FRAG_CB(skb1)->frag_run_len)
>  				rbn = &parent->rb_right;
> -			else /* Found an overlap with skb1. */
> +			else {
> +				/* check for potential duplicate */
> +				while (skb1 && skb1->ip_defrag_offset < offset)
> +					skb1 = FRAG_CB(skb1)->next_frag;
> +				if (skb1 && offset == skb1->ip_defrag_offset &&
> +				    skb->len == skb1->len)
> +					goto err;

Maybe we should not care, if the node in the rbtree contains the range of this
incoming fragment, do not worry about finding if it is overlap or not ?

I am nervous about adding back a linear scan.

> +				/* Found an overlap */
>  				goto overlap;
> +			}
>  		} while (*rbn);
>  		/* Here we have parent properly set, and rbn pointing to
>  		 * one of its NULL left/right children. Insert skb.
> 

Thanks !