On 10/20/26 5:09 PM, Aaron Conole wrote: > Ilya Maximets <[email protected]> writes: > >> On 10/20/26 2:58 PM, Aaron Conole wrote: >>> Ilya Maximets <[email protected]> writes: >>> >>>> On 10/13/26 11:27 PM, Aaron Conole wrote: >>>>> The FTP ALG handling for IPv4 and IPv6 is guarded from overwriting the >>>>> buffer space by checking the headroom + tail-room against the rewrite >>>>> delta. However, when a packet needs to be expanded, only the >>>>> tail-room is actually available. The guard itself allows >>>>> modifications to pass that may otherwise be flagged. >>>>> >>>>> Fix this by only checking that the tail-room has sufficient space to >>>>> handle the packet growth. This is a bit different as we no longer >>>>> check the delta, but just the actual expansion room available. This >>>>> check is a bit more conservative, so there could be an edge-case >>>>> packet that used to pass through the checks but no longer >>>>> would. >>>>> >>>>> The FTP ALG unit tests are updated to correct a comment about the >>>>> guard design, and test for the new condition. >>>>> >>>>> Fixes: bd5e81a0e596 ("Userspace Datapath: Add ALG infra and FTP.") >>>>> Reported-by: VinÃcius Rodrigues <[email protected]> >>>>> Assisted-by: Claude Sonnet 4.5, Claude Opus >>>>> Signed-off-by: Aaron Conole <[email protected]> >>>>> --- >>>>> v1->v2: Fix splats related to the test suite >>>>> v2->v3: Fix the test suite after review from Ilya >>>>> NOTE: We need to add Vinicius to the AUTHORS file on commit, >>>>> because there is an email domain that CloudFlare complains >>>>> about. >>>>> >>>>> lib/conntrack.c | 20 ++++---- >>>>> tests/library.at | 4 ++ >>>>> tests/test-conntrack.c | 113 +++++++++++++++++++++++++++++++++++++++-- >>>>> 3 files changed, 124 insertions(+), 13 deletions(-) >>>>> >>>>> diff --git a/lib/conntrack.c b/lib/conntrack.c >>>>> index f84cdd216a..0809e73f24 100644 >>>>> --- a/lib/conntrack.c >>>>> +++ b/lib/conntrack.c >>>>> @@ -3294,12 +3294,12 @@ repl_ftp_v4_addr(struct dp_packet *pkt, ovs_be32 >>>>> v4_addr_rep, >>>>> >>>>> /* Do conservative check for pathological MTU usage. */ >>>>> uint32_t orig_used_size = dp_packet_size(pkt); >>>>> - if (orig_used_size + MAX_FTP_V4_NAT_DELTA > >>>>> - dp_packet_get_allocated(pkt)) { >>>>> - >>>>> + if (MAX_FTP_V4_NAT_DELTA > dp_packet_tailroom(pkt)) { >>>>> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); >>>>> - VLOG_WARN_RL(&rl, "Unsupported effective MTU %u used with FTP >>>>> V4", >>>>> - dp_packet_get_allocated(pkt)); >>>>> + VLOG_WARN_RL(&rl, >>>>> + "Oversized packet detected with FTPv4 (%"PRIuSIZE >>>>> + " vs. %"PRIu32")", >>>>> + dp_packet_tailroom(pkt), MAX_FTP_V4_NAT_DELTA); >>>>> return 0; >>>>> } >>>>> >>>>> @@ -3674,12 +3674,12 @@ repl_ftp_v6_addr(struct dp_packet *pkt, union >>>>> ct_addr v6_addr_rep, >>>>> >>>>> /* Do conservative check for pathological MTU usage. */ >>>>> uint32_t orig_used_size = dp_packet_size(pkt); >>>>> - if (orig_used_size + MAX_FTP_V6_NAT_DELTA > >>>>> - dp_packet_get_allocated(pkt)) { >>>>> - >>>>> + if (MAX_FTP_V6_NAT_DELTA > dp_packet_tailroom(pkt)) { >>>>> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); >>>>> - VLOG_WARN_RL(&rl, "Unsupported effective MTU %u used with FTP >>>>> V6", >>>>> - dp_packet_get_allocated(pkt)); >>>>> + VLOG_WARN_RL(&rl, >>>>> + "Oversized packet detected with FTPv6 (%"PRIuSIZE >>>>> + " vs. %"PRIu32")", >>>>> + dp_packet_tailroom(pkt), MAX_FTP_V6_NAT_DELTA); >>>>> return 0; >>>>> } >>>>> >>>>> diff --git a/tests/library.at b/tests/library.at >>>>> index 80ebe6ed8a..227d1fb433 100644 >>>>> --- a/tests/library.at >>>>> +++ b/tests/library.at >>>>> @@ -296,3 +296,7 @@ AT_CLEANUP >>>>> AT_SETUP([Conntrack Library - FTP ALG parsing]) >>>>> AT_CHECK([ovstest test-conntrack ftp-alg-large-payload]) >>>>> AT_CLEANUP >>>>> + >>>>> +AT_SETUP([Conntrack Library - FTP ALG guard]) >>>>> +AT_CHECK([ovstest test-conntrack ftp-alg-no-tailroom], [0], [], [ignore]) >>>> >>>> Should we check the stderr to make sure we actually hit the path we need? >>>> It should be possible to remove time from the log formatter. >>> >>> I can change it to do that. For now, I was thinking it is okay to just >>> rely on asserts - that way if the output changes at any point we don't >>> need to do any kind of extra changes, but given it is part of the test >>> harness it probably makes more sense to be extra cautious here. >>> >>>>> +AT_CLEANUP >>>>> diff --git a/tests/test-conntrack.c b/tests/test-conntrack.c >>>>> index 2babe989c4..e9b690d4b9 100644 >>>>> --- a/tests/test-conntrack.c >>>>> +++ b/tests/test-conntrack.c >>>>> @@ -75,9 +75,8 @@ build_eth_ip_packet(struct dp_packet *pkt, struct >>>>> eth_addr eth_src, >>>>> } >>>>> >>>>> if (pkt == NULL) { >>>>> - /* 64-byte extra headroom keeps dp_packet_get_allocated() large >>>>> enough >>>>> - * that the FTP V4 MTU guard (orig_used_size + 8 <= allocated) >>>>> passes >>>>> - * even when the packet is near its maximum size. */ >>>>> + /* Allocate a packet with enough room for payload, and reserve >>>>> + * 64-bytes for extra headroom. */ >>>>> pkt = dp_packet_new_with_headroom(ETH_HEADER_LEN + IP_HEADER_LEN >>>>> + proto_len + payload_alloc, >>>>> 64); >>>>> } >>>>> @@ -576,7 +575,112 @@ test_ftp_alg_large_payload(struct ovs_cmdl_context >>>>> *ctx OVS_UNUSED) >>>>> conntrack_destroy(ct); >>>>> } >>>>> >>>>> +/* Test FTP ALG tail-room guard. >>>>> + * >>>>> + * This test verifies that the FTP ALG properly rejects packets that >>>>> don't >>>>> + * have sufficient tail-room for NAT address replacement. It creates a >>>>> packet >>>>> + * with minimal tail-room (less than MAX_FTP_V4_NAT_DELTA) and confirms >>>>> the >>>>> + * guard triggers, leaving the packet unmodified. */ >>>>> +static void >>>>> +test_ftp_alg_no_tailroom(struct ovs_cmdl_context *ctx OVS_UNUSED) >>>>> +{ >>>>> + struct eth_addr eth_src = ETH_ADDR_C(00, 01, 02, 03, 04, 05); >>>>> + struct eth_addr eth_dst = ETH_ADDR_C(00, 06, 07, 08, 09, 0a); >>>>> + ovs_be32 ip_src = inet_addr("10.0.0.100"); >>>>> + ovs_be32 ip_dst = inet_addr("192.168.100.200"); >>>>> + uint16_t sport = 54321; >>>>> + uint16_t dport = 21; >>>>> + >>>>> + struct nat_action_info_t nat_info; >>>>> + memset(&nat_info, 0, sizeof nat_info); >>>>> + nat_info.nat_action = NAT_ACTION_SRC; >>>>> + nat_info.min_addr.ipv4 = ip_dst; >>>>> + nat_info.max_addr.ipv4 = ip_dst; >>>>> + >>>>> + ct = conntrack_init(); >>>>> + conntrack_set_tcp_seq_chk(ct, false); >>>>> + >>>>> + long long now = time_msec(); >>>>> + >>>>> + struct dp_packet *syn = build_eth_ip_packet(NULL, eth_src, eth_dst, >>>>> + ip_src, ip_dst, >>>>> + IPPROTO_TCP, 0); >>>>> + build_tcp_packet(syn, sport, dport, TCP_SYN, NULL, 0); >>>>> + >>>>> + struct dp_packet_batch syn_batch; >>>>> + dp_packet_batch_init_packet(&syn_batch, syn); >>>>> + conntrack_execute(ct, &syn_batch, htons(ETH_TYPE_IP), false, true, 0, >>>>> + NULL, NULL, "ftp", &nat_info, now, 0); >>>>> + dp_packet_delete_batch(&syn_batch, true); >>>>> + >>>>> + /* Create a packet with NO extra headroom - allocate exact size >>>>> needed. >>>>> + * This ensures tail-room will be insufficient for FTP NAT >>>>> expansion. */ >>>>> + char ftp_cmd[] = "PORT 10,0,0,100,212,53\r\n"; >>>>> + size_t ftp_len = strlen(ftp_cmd); >>>>> + >>>>> + /* Allocate packet with ZERO extra space beyond what's needed */ >>>> >>>> These comments say 'NO extra headroom' and 'ZERO extra space', but the code >>>> below reserves extra space in the headroom, which looks inconsistent. And >>>> we >>>> need extra bytes in order to hit the conditions for this issue >>>> specifically. >>> >>> These are leftovers from what I asked Claude to help with - and I didn't >>> clean them up. Actually given it wrote a test that I blindly trusted, I >>> went the other way, and didn't use any AI to do a 'final pass' before >>> submitting. Probably should have done it, it would have caught... >>> >>>>> + size_t exact_size = (ETH_HEADER_LEN + IP_HEADER_LEN + TCP_HEADER_LEN >>>>> + >>>>> + ftp_len); >>>>> + enum { EXTRA = 8 }; /* */ >>>> >>>> Empty comment? >>> >>> Yeah, here's the story - I was trying to change the packet buffer to >>> have the extra head and tailroom to match against (which is what the '2' >>> was supposed to be). But then I did a dp_packet_reserve with both. >>> >>>>> + struct dp_packet *pkt = dp_packet_new(exact_size + 2 + EXTRA); >>>>> + dp_packet_reserve(pkt, 2 + EXTRA); >>>> >>>> Would be nice to have a comment on what 2 is. >>> >>> Yeah, I'll clean it up to only reserve EXTRA and the 2 will be the tail >>> room (and I'll use a constant for that, too). >> >> FWIW, this '2' came from the eth_compose(): >> >> /* The magic 2 here ensures that the L3 header (when it is added later) >> * will be 32-bit aligned. */ >> >> We need to make sure that the headers are still properly aligned within >> the buffer. > > :-/ > > Okay, my tired brain has been doing too much. Still, I want the test > cases to be: > > 1. Headroom, no tailroom > 2. No headroom, no tailroom > 3. no headroom, insufficient tailroom > 3. some headromm, insufficient tailroom > > I want to make sure at least some of the math is getting exercised, and > I am going to submit v3 with those cases; making sure that we don't have > magic constants.
OK. v4, I assume. :) _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
