RE: [PATCH 2/6] ip_frag: discard datagrams with overlapping fragments

[Konstantin Ananyev <konstantin.ananyev@huawei.com>]

> Existing code does not handle overlapping fragments.
> 
> RFC 8200 (IPv6) requires that on overlap all reassembly is abandoned
> andall received fragments are dropped. RFC 791 (IPv4) originally called
> fortrimming and rewriting, but Linux discards for IPv4 as well, since
> overlap has no legitimate use and is a known attack vector.

Spaces are missing in a few places in the sentence above:
'andall' . "fortrimming'.

> 
> Depends on the duplicate-tolerance change so that an exact duplicate is
> dropped on its own rather than discarding the whole datagram.
> 
> Fixes: cc8f4d020c0b ("examples/ip_reassembly: initial import")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/ip_frag/ip_frag_internal.c | 34 ++++++++++++++++++++++++++--------
>  1 file changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/ip_frag/ip_frag_internal.c b/lib/ip_frag/ip_frag_internal.c
> index 9a03ef995a..2505314a29 100644
> --- a/lib/ip_frag/ip_frag_internal.c
> +++ b/lib/ip_frag/ip_frag_internal.c
> @@ -92,16 +92,34 @@ ip_frag_process(struct ip_frag_pkt *fp, struct
> rte_ip_frag_death_row *dr,
>  	uint32_t i, idx;
> 
>  	/*
> -	 * Discard an exact duplicate fragment. If a previously stored fragment
> -	 * already covers the same offset and length, this fragment carries no
> -	 * new data. Reassembly is tolerant of duplicates (RFC 791), so drop
> -	 * only this mbuf and keep the reassembly entry intact rather than
> -	 * treating it as an error. Fragments overlapping an existing one with
> -	 * different bounds are not handled here.
> +	 * Scan the fragments already collected for this datagram before
> +	 * storing the new one. The stored set is kept free of duplicates and
> +	 * overlaps, so a single pass is sufficient.
>  	 */
>  	for (i = 0; i != fp->last_idx; i++) {
> -		if (fp->frags[i].mb != NULL && fp->frags[i].ofs == ofs &&
> -				fp->frags[i].len == len) {
> +		if (fp->frags[i].mb == NULL)
> +			continue;
> +
> +		/*
> +		 * Exact duplicate: carries no new data. Reassembly tolerates
> +		 * duplicates (RFC 791), so drop only this mbuf and keep the
> +		 * entry.
> +		 */
> +		if (fp->frags[i].ofs == ofs && fp->frags[i].len == len) {
> +			IP_FRAG_MBUF2DR(dr, mb);
> +			return NULL;
> +		}
> +
> +		/*
> +		 * Overlap with an existing fragment. Per RFC 8200 section 4.5
> +		 * (and RFC 5722) the datagram must be discarded; the same is
> +		 * applied to IPv4. Free all collected fragments, drop this one,
> +		 * and invalidate the entry.
> +		 */
> +		if (ofs < fp->frags[i].ofs + fp->frags[i].len &&
> +				fp->frags[i].ofs < ofs + len) {
> +			ip_frag_free(fp, dr);
> +			ip_frag_key_invalidate(&fp->key);
>  			IP_FRAG_MBUF2DR(dr, mb);
>  			return NULL;

Usually that function does some logging IP_FRAG_LOG(...) in case
invalid fragments were detected (see function body below these changes).
Probably worth to keep same here:
Might be a new helper function that will report an error and free table entry,
that can be used across all places in that function.
Apart from that:
Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com> 

>  		}
> --
> 2.53.0