On Tue, Aug 27, 2019 at 2:02 AM Vishal Deep Ajmera < [email protected]> 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 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
