> 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: [email protected]
>
> Signed-off-by: Stephen Hemminger <[email protected]>
> ---
> 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 <[email protected]>
> }
> --
> 2.53.0
