[ovs-dev] [patch v1] conntrack: Fix ICMPV4 error data L4 length check.

[Darrell Ball]

On Tue, Aug 27, 2019 at 2:02 AM Vishal Deep Ajmera <
vishal.deep.ajmera at ericsson.com> wrote:

> Hi Darrell,
>
> Thanks for the patch. When I applied the patch to latest master,
> I see that we take care of length check (< 8) only for ICMPv6 and
> not for ICMPv4.


That is interesting
i just tried applying on top of tree and I see that the git applies some
changes (2 lines)
in extract_l4_icmp6() rather the intended extract_l4_icmp() as in the patch
I sent out.
My guess is that the surrounding lines are identical in the 2 functions and
I had other
patches in the same branch shifting the patch downward, hence git applied
the changes
to extract_l4_icmp6() rather than extract_l4_icmp()

I'll make the changes on a clean branch and resend.

JTBC, the 8 byte ICMP error data L4 length restriction is only for V4.
ICMP6 does not have this restriction; see
https://tools.ietf.org/html/rfc4443



> We need to do it for ICMPv4 as well.
>
>
The intention is only for ICMPv4; see above.


> Also, we are already using 'related' to skip or not to skip length check.
>

We cannot use 'related' for this as ICMPv6 is different than ICMPv4


>  * 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.
>
> However we continue to validate length in extract_l4_tcp (<8 or <20).
> I understand that check for minimum 8 bytes header is needed to make
> sure we can extract tcp port numbers.
>

No, it is a sanity check like other sanity checks.
The length check varies depending on the context.


> Can we instead try to converge all checks at one place and still take care
> of nested header? In my opinion it will simplify the code.
>

See the information above


>
> Warm Regards,
> Vishal Ajmera
>