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. > Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
