Ilya Maximets <[email protected]> writes:
> On 8/31/26 8:12 PM, Aaron Conole wrote:
>> The FTP ALG handling for IPv4 and IPv6 is guarded from overwriting the
>> buffer space by checking the headroom + tailroom against the rewrite
>> delta. However, when a packet needs to be expanded, only the tailroom
>> is actually available. The guard itself allows modifications to pass
>> that may otherwise be flagged.
>>
>> Fix this by only checking that the tailroom 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
>> Signed-off-by: Aaron Conole <[email protected]>
>> ---
>> v1->v2: Fix the test suite related errors
>>
>> AUTHORS.rst | 1 +
>> lib/conntrack.c | 20 +++----
>> tests/library.at | 4 ++
>> tests/test-conntrack.c | 122 ++++++++++++++++++++++++++++++++++++++++-
>> 4 files changed, 134 insertions(+), 13 deletions(-)
>
> Hi, Aaron. Thanks for the patch and sorry for delay.
> The code change seems correct to me, but I have a few comments for the
> test below.
>
>>
>> diff --git a/AUTHORS.rst b/AUTHORS.rst
>> index 0f7445c80f..92500507d4 100644
>> --- a/AUTHORS.rst
>> +++ b/AUTHORS.rst
>> @@ -797,6 +797,7 @@ Tulio Ribeiro
>> [email protected]
>> Tytus Kurek [email protected]
>> Valentin Bud [email protected]
>> Vasiliy Tolstov [email protected]
>> +VinÃcius Rodrigues [email protected]
>> Vinllen Chen [email protected]
>> Vipul Ashri [email protected]
>> Vishal Swarankar [email protected]
>> 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])
>> +AT_CLEANUP
>> diff --git a/tests/test-conntrack.c b/tests/test-conntrack.c
>> index 2babe989c4..74cf1a4426 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,119 @@ test_ftp_alg_large_payload(struct ovs_cmdl_context
>> *ctx OVS_UNUSED)
>> conntrack_destroy(ct);
>> }
>>
>> +/* Test FTP ALG tailroom guard.
>> + *
>> + * This test verifies that the FTP ALG properly rejects packets that don't
>> + * have sufficient tailroom for NAT address replacement. It creates a packet
>
> nit: Double spaces between sentences. Here and in other comments below.
>
>> + * with minimal tailroom (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("10.0.0.1");
>> + uint16_t sport = 54321;
>> + uint16_t dport = 21;
>> +
>> + /* SNAT configuration */
>
> nit: Period at the exnd of the comment. Here and in other comments below.
ACK
>> + 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;
>
> So, we're performing a source address translation into a shorter address.
> Sounds like the opposite of what we're fixing in this patch.
>
>> +
>> + ct = conntrack_init();
>> + conntrack_set_tcp_seq_chk(ct, false);
>> +
>> + long long now = time_msec();
>> +
>> + /* Create conntrack entry with SYN */
>> + 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 tailroom 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 */
>> + size_t exact_size = (ETH_HEADER_LEN + IP_HEADER_LEN + TCP_HEADER_LEN +
>> + ftp_len);
>> + struct dp_packet *pkt = dp_packet_new(exact_size);
>> +
>> + /* Manually build the packet without extra headroom */
>> + eth_compose(pkt, eth_src, eth_dst, ETH_TYPE_IP,
>> + IP_HEADER_LEN + TCP_HEADER_LEN + ftp_len);
>
> This call reallocates the buffer and also reserves 6 bytes of headroom.
>
> The end size is 78 and the allocated space is 84. Which doesn't allow for
> the 8 bytes of MAX_FTP_V4_NAT_DELTA failing the check even before this patch.
> So, the test passes even without a fix, which is not supposed to happen.
>
> We need to be more accurate if we want the exact geometry of the packet
> and we should assert that we got the exact geometry we want to make sure
> the test is testing what we expect it it to.
Hrrm, yes that was inadvertent. To be honest, I asked Claude to write
the test case, and only spot checked it - blindly trusting it. In
future, I will not be making the same mistake.
> Maybe something like this (opus made the change, but seems correct):
>
> diff --git a/tests/test-conntrack.c b/tests/test-conntrack.c
> index 74cf1a442..8869933b4 100644
> --- a/tests/test-conntrack.c
> +++ b/tests/test-conntrack.c
> @@ -575,23 +575,18 @@ test_ftp_alg_large_payload(struct ovs_cmdl_context *ctx
> OVS_UNUSED)
> conntrack_destroy(ct);
> }
>
> -/* Test FTP ALG tailroom guard.
> - *
> - * This test verifies that the FTP ALG properly rejects packets that don't
> - * have sufficient tailroom for NAT address replacement. It creates a packet
> - * with minimal tailroom (less than MAX_FTP_V4_NAT_DELTA) and confirms the
> - * guard triggers, leaving the packet unmodified. */
> +/* Tests an FTP PORT rewrite on a packet whose free space is all headroom and
> + * whose tailroom is zero, so the address expansion has nowhere to go. */
> 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("10.0.0.1");
> + ovs_be32 ip_src = inet_addr("10.0.0.1");
> + ovs_be32 ip_dst = inet_addr("192.168.100.200");
> uint16_t sport = 54321;
> uint16_t dport = 21;
>
> - /* SNAT configuration */
> struct nat_action_info_t nat_info;
> memset(&nat_info, 0, sizeof nat_info);
> nat_info.nat_action = NAT_ACTION_SRC;
> @@ -603,7 +598,6 @@ test_ftp_alg_no_tailroom(struct ovs_cmdl_context *ctx
> OVS_UNUSED)
>
> long long now = time_msec();
>
> - /* Create conntrack entry with SYN */
> struct dp_packet *syn = build_eth_ip_packet(NULL, eth_src, eth_dst,
> ip_src, ip_dst,
> IPPROTO_TCP, 0);
> @@ -615,30 +609,34 @@ test_ftp_alg_no_tailroom(struct ovs_cmdl_context *ctx
> OVS_UNUSED)
> 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 tailroom will be insufficient for FTP NAT expansion. */
> - char ftp_cmd[] = "PORT 10,0,0,100,212,53\r\n";
> + const char ftp_cmd[] = "PORT 10,0,0,1,212,53\r\n";
> size_t ftp_len = strlen(ftp_cmd);
> -
> - /* Allocate packet with ZERO extra space beyond what's needed */
> - size_t exact_size = (ETH_HEADER_LEN + IP_HEADER_LEN + TCP_HEADER_LEN +
> - ftp_len);
> - struct dp_packet *pkt = dp_packet_new(exact_size);
> -
> - /* Manually build the packet without extra headroom */
> - eth_compose(pkt, eth_src, eth_dst, ETH_TYPE_IP,
> - IP_HEADER_LEN + TCP_HEADER_LEN + ftp_len);
> + size_t pkt_len = ETH_HEADER_LEN + IP_HEADER_LEN + TCP_HEADER_LEN +
> ftp_len;
> +
> + /* Build the packet by hand and never resize it afterwards, so the free
> + * space stays entirely in the headroom and the tailroom stays zero. The
> + * leading 2 bytes 32-bit align the L3 header, as eth_compose() does. */
> + enum { EXTRA = 8 };
> + struct dp_packet *pkt = dp_packet_new(2 + pkt_len + EXTRA);
> + dp_packet_reserve(pkt, 2 + EXTRA);
> + memset(dp_packet_put_uninit(pkt, pkt_len), 0, pkt_len);
> +
> + 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;
> - packet_set_ipv4_addr(pkt, &iph->ip_src, ip_src);
> - packet_set_ipv4_addr(pkt, &iph->ip_dst, ip_dst);
> + 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);
> @@ -646,48 +644,31 @@ test_ftp_alg_no_tailroom(struct ovs_cmdl_context *ctx
> OVS_UNUSED)
> 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);
> - tcph->tcp_urg = 0;
> -
> - /* 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));
>
> - /* Update checksums */
> - iph->ip_csum = 0;
> - iph->ip_csum = csum(iph, IP_HEADER_LEN);
> - tcph->tcp_csum = 0;
> - uint32_t tcp_csum = packet_csum_pseudoheader(iph);
> - tcph->tcp_csum = csum_finish(
> - csum_continue(tcp_csum, tcph, TCP_HEADER_LEN + ftp_len));
> -
> - /* Verify we have insufficient tailroom */
> - size_t tailroom = dp_packet_tailroom(pkt);
> - ovs_assert(tailroom < 8); /* Less than MAX_FTP_V4_NAT_DELTA */
> + /* The rewrite needs seven extra bytes; the buffer has free space only in
> + * the headroom. */
> + ovs_assert(dp_packet_size(pkt) == pkt_len);
> + 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));
>
> - /* Save original payload for comparison */
> - char original_payload[64];
> - const char *payload_start = (const char *) tcph + TCP_HEADER_LEN;
> - memcpy(original_payload, payload_start, ftp_len);
> -
> - /* 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);
> - payload_start = (const char *) tcph + TCP_HEADER_LEN;
> - ovs_assert(!memcmp(payload_start, original_payload, ftp_len));
> -
> - /* The original address should still be present, not the SNAT address */
> - ovs_assert(!strncmp(payload_start, "PORT 10,0,0,100,", 16));
> + 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);
> }
>
>
> -
> static const struct ovs_cmdl_command commands[] = {
> /* Connection tracker tests. */
> /* Starts 'n_threads' threads. Each thread will send 'n_pkts' packets to
> @@ -712,9 +693,7 @@ 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 that the FTP ALG tailroom guard properly rejects packets
> - * that don't have sufficient space for NAT address expansion. Creates
> - * a packet with zero tailroom and confirms it's not modified. */
> + /* Verifies FTP ALG rejects rewrites without sufficient tailroom. */
> {"ftp-alg-no-tailroom", "", 0, 0,
> test_ftp_alg_no_tailroom, OVS_RO},
>
> ---
>
> WDYT?
I'll double check and fold it in.
> Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
