Hi Alejandro, Alejandro Pacheco via dev <[email protected]> writes:
> Hello, > > This series fixes header length checks in the userspace datapath. > Version 1 went to the security team, who asked for it to be posted here > for review as one series. How did you find these issues? Were they found by code review, static analysis, hitting the issues in production? > Patches 1 to 10 are minimal fixes, one per bug, each with a test: > > 1 the ND option walk in packet_set_nd() and odp_set_nd() > 2 the SCTP chunk header length check in conntrack > 3 conntrack ICMP handlers picked by protocol number alone > 4 the TCP and SCTP checksum update on a truncated quoted header > 5-7 the GTP-U, ERSPAN and GRE tunnel pops > 8 NSH set and pop > 9 layer offsets and checksum offloads left by output truncation > 10 set_arp() on a truncated ARP header > > With AddressSanitizer, the test of each of patches 1 to 10 fails on the > current code and passes with its patch. > > Patch 11 is hardening on top: it adds dp_packet_l3_hdr() and > dp_packet_l4_hdr(), which return a header only when the packet holds > it, and makes the L3 and L4 field setters use them. It is not needed > for backports. > > The related testsuite sections pass with the whole series under > AddressSanitizer, clang-analyze reports nothing new for any patch of > it, and the tree builds with sparse and -Werror. > > Each patch mail carries its commit exactly as git format-patch wrote > it, as a plain-text attachment, with only the author line in its body, > because my mail setup rewraps message bodies. git am and patchwork > take each one as a single commit with the right author. This paragraph is unneeded. > Please apply the patches as they are (git am), so that they keep their > author; change anything you like in review. This instruction isn't needed - maintainers do our best to NEVER lose attribution. > v4: Please do not use the public CI infrastructure as your testing ground. All of the failures could have been replicated with your own GitHub actions runs, and that prevents those of us who use review assistance scripts from having to filter through all the broken series. Consider that the CI is a shared resource, and use it wisely. > - Patch 11: initialize the IPv6 address in the new tests with braces, > as test_ipv6_static_masks() does, for the sparse error the robot > reported on v3 (include/sparse has no IN6ADDR_LOOPBACK_INIT). > - Patches 1 to 10 are unchanged. > > v3: > - Patch 1: assert the L4 header in the new check_set_nd() test, for > the clang-analyze warning the robot reported on v2. > - Patches 2 to 11 are unchanged, so the GTP-U extension header patch > posted on top of patch 5 still applies. > > v2: > - Posted to ovs-dev as one series, as the security team asked. > - ND: fixed the loops in place, no new helper. > - set_arp(): new minimal fix (patch 10); the accessor patch is now > hardening only and comes last. > - Tests: raw hex packets with a comment per header, > OVS_WAIT_UNTIL_EQUAL, time/warp under AT_CHECK, add_of_ports. > - Shorter comments and commit messages. > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
