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).
>> + memset(dp_packet_put_uninit(pkt, exact_size), 0, exact_size);
>> +
>> + struct eth_header *eth = dp_packet_data(pkt);
>> + eth->eth_dst = eth_dst;
>> + eth->eth_src = eth_src;
>> + eth->eth_type = htons(ETH_TYPE_IP);
>> +
>> + dp_packet_set_l3(pkt, (char *) dp_packet_data(pkt) + ETH_HEADER_LEN);
>> +
>> + struct ip_header *iph = dp_packet_l3(pkt);
>> + iph->ip_ihl_ver = IP_IHL_VER(5, 4);
>> + iph->ip_tot_len = htons(IP_HEADER_LEN + TCP_HEADER_LEN + ftp_len);
>> + iph->ip_ttl = 64;
>> + iph->ip_proto = IPPROTO_TCP;
>> + put_16aligned_be32(&iph->ip_src, ip_src);
>> + put_16aligned_be32(&iph->ip_dst, ip_dst);
>> + iph->ip_csum = csum(iph, IP_HEADER_LEN);
>> +
>> + dp_packet_set_l4(pkt, (char *) iph + IP_HEADER_LEN);
>> +
>> + struct tcp_header *tcph = dp_packet_l4(pkt);
>> + tcph->tcp_src = htons(sport);
>> + tcph->tcp_dst = htons(dport);
>> + put_16aligned_be32(&tcph->tcp_seq, htonl(1000));
>> + put_16aligned_be32(&tcph->tcp_ack, htonl(1000));
>> + tcph->tcp_ctl = TCP_CTL(TCP_PSH | TCP_ACK, TCP_HEADER_LEN / 4);
>> + tcph->tcp_winsz = htons(65535);
>> +
>> + /* Copy FTP payload. */
>> + memcpy((char *) tcph + TCP_HEADER_LEN, ftp_cmd, ftp_len);
>> + tcph->tcp_csum =
>> csum_finish(csum_continue(packet_csum_pseudoheader(iph),
>> + tcph, TCP_HEADER_LEN + ftp_len));
>> +
>> + /* The rewrite needs seven extra bytes; the buffer has free space only
>> in
>> + * the headroom. */
>
> It was seven in the diff I proposed in for v2, but in this version it's five,
> since you changed the IP from .1 to .100. Or am I missing something?
Yeah, the comment should be adjusted anyway, so I'll do that.
>> + ovs_assert(dp_packet_size(pkt) == exact_size);
>> + ovs_assert(dp_packet_tailroom(pkt) == 0);
>> + ovs_assert(dp_packet_headroom(pkt) == 2 + EXTRA);
>> + ovs_assert(dp_packet_size(pkt) + EXTRA <= dp_packet_get_allocated(pkt));
>> +
>> + /* Process through conntrack - guard should reject modification. */
>> + struct dp_packet_batch batch;
>> + dp_packet_batch_init_packet(&batch, pkt);
>> + conntrack_execute(ct, &batch, htons(ETH_TYPE_IP), false, true, 0,
>> + NULL, NULL, "ftp", &nat_info, now, 0);
>> +
>> + /* Verify payload was NOT modified (guard prevented it). */
>> + tcph = dp_packet_l4(pkt);
>> + const char *payload_start = (const char *) tcph + TCP_HEADER_LEN;
>> + ovs_assert(!strncmp(payload_start, ftp_cmd, ftp_len));
>> +
>> + dp_packet_delete_batch(&batch, true);
>> + conntrack_destroy(ct);
>> +}
>> +
>>
>> +
>
> Please, don't add an extra line after the line feed.
ACK
>> static const struct ovs_cmdl_command commands[] = {
>> /* Connection tracker tests. */
>> /* Starts 'n_threads' threads. Each thread will send 'n_pkts' packets to
>> @@ -601,6 +705,9 @@ static const struct ovs_cmdl_command commands[] = {
>> * is rewritten to the SNAT target rather than causing a crash. */
>> {"ftp-alg-large-payload", "", 0, 0,
>> test_ftp_alg_large_payload, OVS_RO},
>> + /* Verifies FTP ALG rejects rewrites without sufficient tail-room. */
>> + {"ftp-alg-no-tailroom", "", 0, 0,
>> + test_ftp_alg_no_tailroom, OVS_RO},
>>
>> {NULL, NULL, 0, 0, NULL, OVS_RO},
>> };
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
