Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] The commit message is imprecise, and for a Cc: stable fix it
leaves out the impact.
--
ipvs: reject FTP control ports as data ports
With this patch, ip_vs_ftp_out() will not create a passive-mode data
connection if the port advertised by the server is zero or is one of the
configured FTP control ports. It also changes ip_vs_ftp_in() so that
active-mode handling is refused when the client port is zero or when
vport - 1 is a configured control port.
> ip_vs_ftp_out() creates a wildcard data connection from the
> server-advertised passive port. If that port is one of the configured FTP
> control ports, ip_vs_conn_new() binds the FTP helper to the new connection
> again. A subsequent wildcard lookup can then extend a controlled-connection
> chain.
>
> Reject zero and configured control ports before creating passive
> connections. For active mode, reject a zero client port and a data port
> derived from a configured control port.
[Severity: Low]
This is tagged for stable. Could the commit message say what goes wrong
once the controlled-connection chain gets extended?
The earlier patch in this series, "ipvs: avoid stack overflow from
recursive connection expiration", points to the actual consequence. That
consequence is chains of controlled connections pinned through n_control,
and stack exhaustion from recursive ip_vs_conn_expire(). This message does
not mention that, or any dependency on that patch.
The message also doesn't say where the trigger comes from. In passive mode
the port comes from the real server's 227/229 reply. The active case needs
an admin configuration where vport - 1 is also a configured FTP port.
Is the active-mode sentence accurate? Every active-mode data port is
derived from a configured control port. The data port is vport - 1, and
vport is always a configured FTP port, because tcp_app_conn_bind() only
binds the helper when:
if (inc->port == cp->vport) {
Read literally, the sentence says all active transfers are rejected. The
code in ip_vs_ftp_in() only rejects them when the derived port is itself
a configured control port:
if (!port || is_control_port(ntohs(cp->vport) - 1))
return 0;
Could this be reworded along the lines of "reject a derived data port
(vport - 1) that is itself a configured control port"?
--
Sashiko AI review ยท
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1790146910.git.zihanx%40nebusec.ai
