Re: [PATCH nf v2] ipvs: avoid wrapping persistent service timeouts

Re: [PATCH nf v2] ipvs: avoid wrapping persistent service timeouts

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

[PATCH nf v2] ipvs: avoid wrapping persistent service timeouts

[Yingjie Wang]

ip_vs_add_service() and ip_vs_edit_service() convert an unchecked u32
timeout from seconds to jiffies with u->timeout * HZ. Both operands are
unsigned int, so values above UINT_MAX / HZ wrap.

With HZ=1000, 4294968 seconds becomes 704 jiffies and is reported back
as 0 seconds. Persistence templates copy svc->timeout, so their timer is
armed for 0.7 seconds and a client can be moved to another real server
long before the configured timeout.

ipvsadm limits service persistence and protocol timeouts to 31 days.
Enforce the same limit in the kernel and reject larger values with
-EINVAL. Convert accepted values through a common helper that clamps
them to MAX_JIFFY_OFFSET / HZ on 32-bit systems. Use the helper for
timeouts received from the synchronization protocol as well, since those
values do not pass through the service configuration checks.

Change the service timeout and protocol timeout tables to unsigned long
because they store jiffies. With HZ=1000, 31 days is 2678400000 jiffies,
which does not fit in the signed int used by the protocol tables. This
also allows the global protocol timeout interface to represent the full
31-day range on 64-bit systems.

The overflow was reproduced in an x86-64 QEMU guest with HZ=1000. A
persistent one-packet UDP service was set to 4294968 seconds, and two
datagrams sent 1.5 seconds apart were directed to different real servers
after the template expired.

On the x86-64 test kernel with HZ=1000, 2678401-second service timeouts
were accepted and 4294968 seconds read back as 0, while a 2678400-second
protocol timeout was rejected. With this change, 2678401 and 4294968
seconds are rejected, a failed edit leaves the previous service timeout
unchanged, and 2678400-second service and protocol timeouts read back
unchanged.

On an i386 test kernel with HZ=1000, the accepted 2678400-second values
were clamped to 1073741 seconds by the 32-bit jiffy limit and read back
as such through both interfaces.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Suggested-by: Julian Anastasov <ja@ssi.bg>
Link: https://lore.kernel.org/r/4f420fda8690ac470be44b3da284b65501c77bb5.1793430892.git.x-professor@outlook.com
Link: https://lore.kernel.org/r/5fa37fba-530c-2c5b-038e-9cb95e34023d@ssi.bg
Signed-off-by: Yingjie Wang <x-professor@outlook.com>
---

Notes:
    Changes since v1:
    - Follow Julian's suggestion to use the 31-day ipvsadm limit, reject
      larger configured values, and clamp accepted values only for the
      32-bit jiffy limit.
    - Move the conversion helper to include/net/ip_vs.h and use it for
      service, protocol and synchronized connection timeouts.
    - Store service and protocol timeout ticks in unsigned long values so
      the full range is representable.
    - Add runtime coverage for the 31-day boundary, failed service updates
      and global TCP/UDP timeout readback on x86-64 and i386. The i386 test
      also verifies the intended MAX_JIFFY_OFFSET clamp.
    
    v1: https://lore.kernel.org/r/4f420fda8690ac470be44b3da284b65501c77bb5.1793430892.git.x-professor@outlook.com

 include/net/ip_vs.h                   | 20 ++++++++++++---
 net/netfilter/ipvs/ip_vs_app.c        |  3 +--
 net/netfilter/ipvs/ip_vs_ctl.c        | 35 ++++++++++++++++-----------
 net/netfilter/ipvs/ip_vs_proto.c      |  4 +--
 net/netfilter/ipvs/ip_vs_proto_sctp.c |  6 ++---
 net/netfilter/ipvs/ip_vs_proto_tcp.c  |  6 ++---
 net/netfilter/ipvs/ip_vs_proto_udp.c  |  6 ++---
 net/netfilter/ipvs/ip_vs_sync.c       |  6 ++---
 8 files changed, 52 insertions(+), 34 deletions(-)

diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
index 9a83bff32569..4eaa06b5f991 100644
--- a/include/net/ip_vs.h
+++ b/include/net/ip_vs.h
@@ -18,6 +18,7 @@
 #include <linux/workqueue.h>
 
 #include <linux/compiler.h>
+#include <linux/jiffies.h>
 #include <linux/timer.h>
 #include <linux/bug.h>
 
@@ -55,6 +56,18 @@
 #define IP_VS_CONN_MAX	(1 << 24)
 #endif
 
+/* All timeouts up to 31 days */
+#define IP_VS_TIMEOUT_MAX_SECS	(86400U * 31)
+
+static inline unsigned long ip_vs_timeout_ticks(unsigned int secs)
+{
+	secs = min_t(unsigned int, secs, IP_VS_TIMEOUT_MAX_SECS);
+#if BITS_PER_LONG <= 32
+	secs = min_t(unsigned int, secs, MAX_JIFFY_OFFSET / HZ);
+#endif
+	return (unsigned long)secs * HZ;
+}
+
 /* svc_table limits */
 #define IP_VS_SVC_TAB_MIN_BITS	4
 #define IP_VS_SVC_TAB_MAX_BITS	20
@@ -776,7 +789,7 @@ struct ip_vs_protocol {
 struct ip_vs_proto_data {
 	struct ip_vs_proto_data	*next;
 	struct ip_vs_protocol	*pp;
-	int			*timeout_table;	/* protocol timeout table */
+	unsigned long		*timeout_table;	/* protocol timeout table */
 	atomic_t		appcnt;		/* counter of proto app incs. */
 	struct tcp_states_t	*tcp_state_table;
 };
@@ -938,7 +951,7 @@ struct ip_vs_service {
 	atomic_t		refcnt;   /* reference counter */
 	__be16			port;	  /* port number for the service */
 	unsigned int		flags;	  /* service status flags */
-	unsigned int		timeout;  /* persistent timeout in ticks */
+	unsigned long		timeout;  /* persistent timeout in ticks */
 	__be32			netmask;  /* grouping granularity, mask/plen */
 	struct netns_ipvs	*ipvs;
 
@@ -1844,7 +1857,8 @@ struct ip_vs_pe *__ip_vs_pe_getbyname(const char *pe_name);
 int ip_vs_protocol_init(void);
 void ip_vs_protocol_cleanup(void);
 void ip_vs_protocol_timeout_change(struct netns_ipvs *ipvs, int flags);
-int *ip_vs_create_timeout_table(int *table, int size);
+unsigned long *ip_vs_create_timeout_table(const unsigned long *table,
+					  size_t size);
 void ip_vs_tcpudp_debug_packet(int af, struct ip_vs_protocol *pp,
 			       const struct sk_buff *skb, int offset,
 			       const char *msg);
diff --git a/net/netfilter/ipvs/ip_vs_app.c b/net/netfilter/ipvs/ip_vs_app.c
index 11cbdbaf561d..89be90a2c383 100644
--- a/net/netfilter/ipvs/ip_vs_app.c
+++ b/net/netfilter/ipvs/ip_vs_app.c
@@ -93,8 +93,7 @@ ip_vs_app_inc_new(struct netns_ipvs *ipvs, struct ip_vs_app *app, __u16 proto,
 
 	if (app->timeouts) {
 		inc->timeout_table =
-			ip_vs_create_timeout_table(app->timeouts,
-						   app->timeouts_size);
+			kmemdup(app->timeouts, app->timeouts_size, GFP_KERNEL);
 		if (!inc->timeout_table) {
 			ret = -ENOMEM;
 			goto out;
diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c
index ce9fe20d6106..5f19fd9545a6 100644
--- a/net/netfilter/ipvs/ip_vs_ctl.c
+++ b/net/netfilter/ipvs/ip_vs_ctl.c
@@ -1766,6 +1766,9 @@ ip_vs_add_service(struct netns_ipvs *ipvs, struct ip_vs_service_user_kern *u,
 	int ret = 0;
 	bool grow;
 
+	if (u->timeout > IP_VS_TIMEOUT_MAX_SECS)
+		return -EINVAL;
+
 	/* increase the module use count */
 	if (!ip_vs_use_count_inc())
 		return -ENOPROTOOPT;
@@ -1864,7 +1867,7 @@ ip_vs_add_service(struct netns_ipvs *ipvs, struct ip_vs_service_user_kern *u,
 	svc->port = u->port;
 	svc->fwmark = u->fwmark;
 	svc->flags = u->flags & ~IP_VS_SVC_F_HASHED;
-	svc->timeout = u->timeout * HZ;
+	svc->timeout = ip_vs_timeout_ticks(u->timeout);
 	svc->netmask = u->netmask;
 	svc->ipvs = ipvs;
 
@@ -1971,6 +1974,9 @@ ip_vs_edit_service(struct ip_vs_service *svc, struct ip_vs_service_user_kern *u)
 	struct netns_ipvs *ipvs = svc->ipvs;
 	int af_id = ip_vs_af_index(svc->af);
 
+	if (u->timeout > IP_VS_TIMEOUT_MAX_SECS)
+		return -EINVAL;
+
 	/*
 	 * Lookup the scheduler, by 'u->sched_name'
 	 */
@@ -2031,7 +2037,7 @@ ip_vs_edit_service(struct ip_vs_service *svc, struct ip_vs_service_user_kern *u)
 	 * Set the flags and timeout value
 	 */
 	svc->flags = u->flags | IP_VS_SVC_F_HASHED;
-	svc->timeout = u->timeout * HZ;
+	svc->timeout = ip_vs_timeout_ticks(u->timeout);
 	svc->netmask = u->netmask;
 
 	old_pe = rcu_dereference_protected(svc->pe, 1);
@@ -3064,9 +3070,8 @@ static int ip_vs_info_seq_show(struct seq_file *seq, void *v)
 		}
 
 		if (svc->flags & IP_VS_SVC_F_PERSISTENT)
-			seq_printf(seq, "persistent %d %08X\n",
-				svc->timeout,
-				ntohl(svc->netmask));
+			seq_printf(seq, "persistent %lu %08X\n",
+				   svc->timeout, ntohl(svc->netmask));
 		else
 			seq_putc(seq, '\n');
 
@@ -3365,36 +3370,38 @@ static int ip_vs_set_timeout(struct netns_ipvs *ipvs, struct ip_vs_timeout_user
 		  u->udp_timeout);
 
 #ifdef CONFIG_IP_VS_PROTO_TCP
-	if (u->tcp_timeout < 0 || u->tcp_timeout > (INT_MAX / HZ) ||
-	    u->tcp_fin_timeout < 0 || u->tcp_fin_timeout > (INT_MAX / HZ)) {
+	if (u->tcp_timeout < 0 ||
+	    u->tcp_timeout > IP_VS_TIMEOUT_MAX_SECS ||
+	    u->tcp_fin_timeout < 0 ||
+	    u->tcp_fin_timeout > IP_VS_TIMEOUT_MAX_SECS) {
 		return -EINVAL;
 	}
 #endif
 
 #ifdef CONFIG_IP_VS_PROTO_UDP
-	if (u->udp_timeout < 0 || u->udp_timeout > (INT_MAX / HZ))
+	if (u->udp_timeout < 0 || u->udp_timeout > IP_VS_TIMEOUT_MAX_SECS)
 		return -EINVAL;
 #endif
 
 #ifdef CONFIG_IP_VS_PROTO_TCP
 	if (u->tcp_timeout) {
 		pd = ip_vs_proto_data_get(ipvs, IPPROTO_TCP);
-		pd->timeout_table[IP_VS_TCP_S_ESTABLISHED]
-			= u->tcp_timeout * HZ;
+		pd->timeout_table[IP_VS_TCP_S_ESTABLISHED] =
+			ip_vs_timeout_ticks(u->tcp_timeout);
 	}
 
 	if (u->tcp_fin_timeout) {
 		pd = ip_vs_proto_data_get(ipvs, IPPROTO_TCP);
-		pd->timeout_table[IP_VS_TCP_S_FIN_WAIT]
-			= u->tcp_fin_timeout * HZ;
+		pd->timeout_table[IP_VS_TCP_S_FIN_WAIT] =
+			ip_vs_timeout_ticks(u->tcp_fin_timeout);
 	}
 #endif
 
 #ifdef CONFIG_IP_VS_PROTO_UDP
 	if (u->udp_timeout) {
 		pd = ip_vs_proto_data_get(ipvs, IPPROTO_UDP);
-		pd->timeout_table[IP_VS_UDP_S_NORMAL]
-			= u->udp_timeout * HZ;
+		pd->timeout_table[IP_VS_UDP_S_NORMAL] =
+			ip_vs_timeout_ticks(u->udp_timeout);
 	}
 #endif
 	return 0;
diff --git a/net/netfilter/ipvs/ip_vs_proto.c b/net/netfilter/ipvs/ip_vs_proto.c
index 0046aed1fb38..3796cd93c028 100644
--- a/net/netfilter/ipvs/ip_vs_proto.c
+++ b/net/netfilter/ipvs/ip_vs_proto.c
@@ -185,8 +185,8 @@ void ip_vs_protocol_timeout_change(struct netns_ipvs *ipvs, int flags)
 }
 
 
-int *
-ip_vs_create_timeout_table(int *table, int size)
+unsigned long *
+ip_vs_create_timeout_table(const unsigned long *table, size_t size)
 {
 	return kmemdup(table, size, GFP_KERNEL);
 }
diff --git a/net/netfilter/ipvs/ip_vs_proto_sctp.c b/net/netfilter/ipvs/ip_vs_proto_sctp.c
index fb8af6b15a39..7e447b99d94c 100644
--- a/net/netfilter/ipvs/ip_vs_proto_sctp.c
+++ b/net/netfilter/ipvs/ip_vs_proto_sctp.c
@@ -327,7 +327,7 @@ static const __u8 sctp_states
 #define IP_VS_SCTP_MAX_RTO	((60 + 1) * HZ)
 
 /* Timeout table[state] */
-static const int sctp_timeouts[IP_VS_SCTP_S_LAST + 1] = {
+static const unsigned long sctp_timeouts[IP_VS_SCTP_S_LAST + 1] = {
 	[IP_VS_SCTP_S_NONE]			= 2 * HZ,
 	[IP_VS_SCTP_S_INIT1]			= (0 + 3 + 1) * HZ,
 	[IP_VS_SCTP_S_INIT]			= IP_VS_SCTP_MAX_RTO,
@@ -552,8 +552,8 @@ static int sctp_app_conn_bind(struct ip_vs_conn *cp)
 static int __ip_vs_sctp_init(struct netns_ipvs *ipvs, struct ip_vs_proto_data *pd)
 {
 	ip_vs_init_hash_table(ipvs->sctp_apps, SCTP_APP_TAB_SIZE);
-	pd->timeout_table = ip_vs_create_timeout_table((int *)sctp_timeouts,
-							sizeof(sctp_timeouts));
+	pd->timeout_table =
+		ip_vs_create_timeout_table(sctp_timeouts, sizeof(sctp_timeouts));
 	if (!pd->timeout_table)
 		return -ENOMEM;
 	return 0;
diff --git a/net/netfilter/ipvs/ip_vs_proto_tcp.c b/net/netfilter/ipvs/ip_vs_proto_tcp.c
index fec0e8b47b71..923405b65a33 100644
--- a/net/netfilter/ipvs/ip_vs_proto_tcp.c
+++ b/net/netfilter/ipvs/ip_vs_proto_tcp.c
@@ -326,7 +326,7 @@ static const int tcp_state_off[IP_VS_DIR_LAST] = {
 /*
  *	Timeout table[state]
  */
-static const int tcp_timeouts[IP_VS_TCP_S_LAST+1] = {
+static const unsigned long tcp_timeouts[IP_VS_TCP_S_LAST + 1] = {
 	[IP_VS_TCP_S_NONE]		=	2*HZ,
 	[IP_VS_TCP_S_ESTABLISHED]	=	15*60*HZ,
 	[IP_VS_TCP_S_SYN_SENT]		=	2*60*HZ,
@@ -665,8 +665,8 @@ void ip_vs_tcp_conn_listen(struct ip_vs_conn *cp)
 static int __ip_vs_tcp_init(struct netns_ipvs *ipvs, struct ip_vs_proto_data *pd)
 {
 	ip_vs_init_hash_table(ipvs->tcp_apps, TCP_APP_TAB_SIZE);
-	pd->timeout_table = ip_vs_create_timeout_table((int *)tcp_timeouts,
-							sizeof(tcp_timeouts));
+	pd->timeout_table =
+		ip_vs_create_timeout_table(tcp_timeouts, sizeof(tcp_timeouts));
 	if (!pd->timeout_table)
 		return -ENOMEM;
 	pd->tcp_state_table = tcp_states;
diff --git a/net/netfilter/ipvs/ip_vs_proto_udp.c b/net/netfilter/ipvs/ip_vs_proto_udp.c
index f32785682402..b4de47f8273e 100644
--- a/net/netfilter/ipvs/ip_vs_proto_udp.c
+++ b/net/netfilter/ipvs/ip_vs_proto_udp.c
@@ -396,7 +396,7 @@ static int udp_app_conn_bind(struct ip_vs_conn *cp)
 }
 
 
-static const int udp_timeouts[IP_VS_UDP_S_LAST+1] = {
+static const unsigned long udp_timeouts[IP_VS_UDP_S_LAST + 1] = {
 	[IP_VS_UDP_S_NORMAL]		=	5*60*HZ,
 	[IP_VS_UDP_S_LAST]		=	2*HZ,
 };
@@ -432,8 +432,8 @@ udp_state_transition(struct ip_vs_conn *cp, int direction,
 static int __udp_init(struct netns_ipvs *ipvs, struct ip_vs_proto_data *pd)
 {
 	ip_vs_init_hash_table(ipvs->udp_apps, UDP_APP_TAB_SIZE);
-	pd->timeout_table = ip_vs_create_timeout_table((int *)udp_timeouts,
-							sizeof(udp_timeouts));
+	pd->timeout_table =
+		ip_vs_create_timeout_table(udp_timeouts, sizeof(udp_timeouts));
 	if (!pd->timeout_table)
 		return -ENOMEM;
 	return 0;
diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
index 157f5c8361ab..175b842cdfec 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -847,7 +847,7 @@ static void ip_vs_proc_conn(struct netns_ipvs *ipvs, struct ip_vs_conn_param *pa
 			    unsigned int flags, unsigned int state,
 			    unsigned int protocol, unsigned int type,
 			    const union nf_inet_addr *daddr, __be16 dport,
-			    unsigned long timeout, __u32 fwmark,
+			    unsigned int timeout, __u32 fwmark,
 			    struct ip_vs_sync_conn_options *opt)
 {
 	struct ip_vs_dest *dest;
@@ -939,9 +939,7 @@ static void ip_vs_proc_conn(struct netns_ipvs *ipvs, struct ip_vs_conn_param *pa
 	 *  - No problem.
 	 */
 	if (timeout) {
-		if (timeout > MAX_SCHEDULE_TIMEOUT / HZ)
-			timeout = MAX_SCHEDULE_TIMEOUT / HZ;
-		cp->timeout = timeout*HZ;
+		cp->timeout = ip_vs_timeout_ticks(timeout);
 	} else {
 		struct ip_vs_proto_data *pd;
 

base-commit: 734bef548a96e8774d6eef5dfa2026c6edc4e80b
-- 
2.43.0