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