FAILED: patch "[PATCH] ipvs: fix racy memcpy in proc_do_sync_threshold" failed to apply to 4.19-stable tree

PATCH In review [email protected] 2 participants 5 messages Aug 21, 2023 – Aug 26, 2023

Subsystems (4)

Networking Stable branch netfilter IPVS

lore

Share & tools

lore

[email protected]

author maintainer

2023-08-21 13:33 UTC #0 lore raw

The patch below does not apply to the 4.19-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

To reproduce the conflict and resubmit, you may use the following commands:

git fetch https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/ linux-4.19.y
git checkout FETCH_HEAD
git cherry-pick -x 5310760af1d4fbea1452bfc77db5f9a680f7ae47
# <resolve conflicts, build, test, etc.>
git commit -s
git send-email --to '<[email protected]>' --in-reply-to '2023082114-remix-cable-0852@gregkh' --subject-prefix 'PATCH 4.19.y' HEAD^..

Possible dependencies:

5310760af1d4 ("ipvs: fix racy memcpy in proc_do_sync_threshold") 1b90af292e71 ("ipvs: Improve robustness to the ipvs sysctl")

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From 5310760af1d4fbea1452bfc77db5f9a680f7ae47 Mon Sep 17 00:00:00 2001
From: Sishuai Gong <[email protected]>
Date: Thu, 10 Aug 2023 15:12:42 -0400
Subject: [PATCH] ipvs: fix racy memcpy in proc_do_sync_threshold

When two threads run proc_do_sync_threshold() in parallel, data races could happen between the two memcpy():

Thread-1 Thread-2

memcpy(val, valp, sizeof(val));
				memcpy(valp, val, sizeof(val));

This race might mess up the (struct ctl_table *) table->data, so we add a mutex lock to serialize them.

Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")

Link https://lore.kernel.org/netdev/[email protected]/ open here

Signed-off-by Sishuai Gong <[email protected]>

Acked-by Simon Horman <[email protected]>

Acked-by Julian Anastasov <[email protected]>

Signed-off-by Florian Westphal <[email protected]>

net/netfilter/ipvs/ip_vs_ctl.c +4 -0

index 62606fb44d02..4bb0d90eca1c 100644
@@ -1876,6 +1876,7 @@ static int
 proc_do_sync_threshold(struct ctl_table *table, int write,
 		       void *buffer, size_t *lenp, loff_t *ppos)
 {
+	struct netns_ipvs *ipvs = table->extra2;
 	int *valp = table->data;
 	int val[2];
 	int rc;

@@ -1885,6 +1886,7 @@ proc_do_sync_threshold(struct ctl_table *table, int write,
 		.mode = table->mode,
 	};
 
+	mutex_lock(&ipvs->sync_mutex);
 	memcpy(val, valp, sizeof(val));
 	rc = proc_dointvec(&tmp, write, buffer, lenp, ppos);
 	if (write) {

@@ -1894,6 +1896,7 @@ proc_do_sync_threshold(struct ctl_table *table, int write,
 		else
 			memcpy(valp, val, sizeof(val));
 	}
+	mutex_unlock(&ipvs->sync_mutex);
 	return rc;
 }

@@ -4321,6 +4324,7 @@ static int __net_init ip_vs_control_net_init_sysctl(struct netns_ipvs *ipvs)
 	ipvs->sysctl_sync_threshold[0] = DEFAULT_SYNC_THRESHOLD;
 	ipvs->sysctl_sync_threshold[1] = DEFAULT_SYNC_PERIOD;
 	tbl[idx].data = &ipvs->sysctl_sync_threshold;
+	tbl[idx].extra2 = ipvs;
 	tbl[idx++].maxlen = sizeof(ipvs->sysctl_sync_threshold);
 	ipvs->sysctl_sync_refresh_period = DEFAULT_SYNC_REFRESH_PERIOD;
 	tbl[idx++].data = &ipvs->sysctl_sync_refresh_period;

Julian Anastasov

maintainer

2023-08-22 15:02 UTC #1 lore raw

Hello,

Quoted 15 lines · On Mon, 21 Aug 2023, [email protected] wrote:

The patch below does not apply to the 4.19-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.
To reproduce the conflict and resubmit, you may use the following commands:
git fetch https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/ linux-4.19.y
git checkout FETCH_HEAD
git cherry-pick -x 5310760af1d4fbea1452bfc77db5f9a680f7ae47
# <resolve conflicts, build, test, etc.>
git commit -s
git send-email --to '<[email protected]>' --in-reply-to '2023082114-remix-cable-0852@gregkh' --subject-prefix 'PATCH 4.19.y' HEAD^..

Quoted 4 lines

Possible dependencies:
5310760af1d4 ("ipvs: fix racy memcpy in proc_do_sync_threshold") 1b90af292e71 ("ipvs: Improve robustness to the ipvs sysctl")

It can happen only if we backport the other mentioned
commit 1b90af292e71 which needs changing SYSCTL_ZERO/SYSCTL_ONE to zero/one and then 5310760af1d4 will apply as-is to both 4.14 and 4.19. Should I send backport for 1b90af292e71, it is even more useful as a fix than 5310760af1d4?

Quoted 57 lines

thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
Quoted 1 line
From 5310760af1d4fbea1452bfc77db5f9a680f7ae47 Mon Sep 17 00:00:00 2001
From: Sishuai Gong <[email protected]>
Date: Thu, 10 Aug 2023 15:12:42 -0400
Subject: [PATCH] ipvs: fix racy memcpy in proc_do_sync_threshold
When two threads run proc_do_sync_threshold() in parallel, data races could happen between the two memcpy():
Thread-1 Thread-2
memcpy(val, valp, sizeof(val));
				memcpy(valp, val, sizeof(val));
This race might mess up the (struct ctl_table *) table->data, so we add a mutex lock to serialize them.
Fixes 1da177e4c3f4 ("Linux-2.6.12-rc2")
Link https://lore.kernel.org/netdev/[email protected]/ open here
Signed-off-by Sishuai Gong <[email protected]>
Acked-by Simon Horman <[email protected]>
Acked-by Julian Anastasov <[email protected]>
Signed-off-by Florian Westphal <[email protected]>
net/netfilter/ipvs/ip_vs_ctl.c +3 -0
index 62606fb44d02..4bb0d90eca1c 100644
@@ -1876,6 +1876,7 @@ static int
 proc_do_sync_threshold(struct ctl_table *table, int write,
 		       void *buffer, size_t *lenp, loff_t *ppos)
 {
+	struct netns_ipvs *ipvs = table->extra2;
 	int *valp = table->data;
 	int val[2];
 	int rc;
@@ -1885,6 +1886,7 @@ proc_do_sync_threshold(struct ctl_table *table, int write,
 		.mode = table->mode,
 	};
 
+	mutex_lock(&ipvs->sync_mutex);
 	memcpy(val, valp, sizeof(val));
 	rc = proc_dointvec(&tmp, write, buffer, lenp, ppos);
 	if (write) {
@@ -1894,6 +1896,7 @@ proc_do_sync_threshold(struct ctl_table *table, int write,
 		else
 			memcpy(valp, val, sizeof(val));
 	}
+	mutex_unlock(&ipvs->sync_mutex);
 	return rc;
 }
 

net/netfilter/ipvs/ip_vs_ctl.c static int __net_init ip_vs_control_net_init_sysctl(struct netns_ipvs *ipvs) · line 4330

Quoted 7 lines

net/netfilter/ipvs/ip_vs_ctl.c +1 -0

@@ -4321,6 +4324,7 @@ static int __net_init ip_vs_control_net_init_sysctl(struct netns_ipvs *ipvs)
 	ipvs->sysctl_sync_threshold[0] = DEFAULT_SYNC_THRESHOLD;
 	ipvs->sysctl_sync_threshold[1] = DEFAULT_SYNC_PERIOD;
 	tbl[idx].data = &ipvs->sysctl_sync_threshold;
+	tbl[idx].extra2 = ipvs;
 	tbl[idx++].maxlen = sizeof(ipvs->sysctl_sync_threshold);
 	ipvs->sysctl_sync_refresh_period = DEFAULT_SYNC_REFRESH_PERIOD;
 	tbl[idx++].data = &ipvs->sysctl_sync_refresh_period;

Regards

--
Julian Anastasov <[email protected]>

Julian Anastasov

maintainer

2023-08-24 11:53 UTC #2 lore raw

cover ipvs: backport 1b90af292e71 and 5310760af1d4

In reply to [email protected]

Hello,

This patchset contains backport for
commit 1b90af292e71b20d03b837d39406acfbdc5d4b2a. It applies to linux-4.14.y and linux-4.19.y and differs from original commit for the zero/one values used for extra1/extra2.

When applied, the concerned commit
5310760af1d4fbea1452bfc77db5f9a680f7ae47 can be cherry-picked and it will apply cleanly on top of 1b90af292e71.

Junwei Hu (1):
ipvs: Improve robustness to the ipvs sysctl

 net/netfilter/ipvs/ip_vs_ctl.c | 70 +++++++++++++++++-----------------
 1 file changed, 36 insertions(+), 34 deletions(-)

Signature · 1 line

2.41.0

Julian Anastasov

maintainer

2023-08-24 11:53 UTC #3 lore raw

PATCH 1/1 ipvs: Improve robustness to the ipvs sysctl

From: Junwei Hu <[email protected]>

The ipvs module parse the user buffer and save it to sysctl, then check if the value is valid. invalid value occurs over a period of time.
Here, I add a variable, struct ctl_table tmp, used to read the value from the user buffer, and save only when it is valid. I delete proc_do_sync_mode and use extra1/2 in table for the proc_dointvec_minmax call.

Fixes f73181c8288f ("ipvs: add support for sync threads")

Signed-off-by Junwei Hu <[email protected]>

Acked-by Julian Anastasov <[email protected]>

Signed-off-by Pablo Neira Ayuso <[email protected]>

(cherry picked from commit 1b90af292e71b20d03b837d39406acfbdc5d4b2a)

[Julian: Backport by changing SYSCTL_ZERO/SYSCTL_ONE to zero/one]

Signed-off-by Julian Anastasov <[email protected]>

---
 net/netfilter/ipvs/ip_vs_ctl.c | 70 +++++++++++++++++-----------------
 1 file changed, 36 insertions(+), 34 deletions(-)

net/netfilter/ipvs/ip_vs_ctl.c +36 ~~-34~~

index ecc16d8c1cc3..4e78c2a6a3ca 100644
@@ -1648,6 +1648,7 @@ static int ip_vs_zero_all(struct netns_ipvs *ipvs)
 #ifdef CONFIG_SYSCTL
 
 static int zero;
+static int one = 1;
 static int three = 3;
 
 static int

@@ -1659,12 +1660,18 @@ proc_do_defense_mode(struct ctl_table *table, int write,
 	int val = *valp;
 	int rc;
 
-	rc = proc_dointvec(table, write, buffer, lenp, ppos);
+	struct ctl_table tmp = {
+		.data = &val,
+		.maxlen = sizeof(int),
+		.mode = table->mode,
+	};
+
+	rc = proc_dointvec(&tmp, write, buffer, lenp, ppos);
 	if (write && (*valp != val)) {
-		if ((*valp < 0) || (*valp > 3)) {
-			/* Restore the correct value */
-			*valp = val;
+		if (val < 0 || val > 3) {
+			rc = -EINVAL;
 		} else {
+			*valp = val;
 			update_defense_level(ipvs);
 		}
 	}

@@ -1678,33 +1685,20 @@ proc_do_sync_threshold(struct ctl_table *table, int write,
 	int *valp = table->data;
 	int val[2];
 	int rc;
+	struct ctl_table tmp = {
+		.data = &val,
+		.maxlen = table->maxlen,
+		.mode = table->mode,
+	};
 
-	/* backup the value first */
 	memcpy(val, valp, sizeof(val));
-
-	rc = proc_dointvec(table, write, buffer, lenp, ppos);
-	if (write && (valp[0] < 0 || valp[1] < 0 ||
-	    (valp[0] >= valp[1] && valp[1]))) {
-		/* Restore the correct value */
-		memcpy(valp, val, sizeof(val));
-	}
-	return rc;
-}
-
-static int
-proc_do_sync_mode(struct ctl_table *table, int write,
-		     void __user *buffer, size_t *lenp, loff_t *ppos)
-{
-	int *valp = table->data;
-	int val = *valp;
-	int rc;
-
-	rc = proc_dointvec(table, write, buffer, lenp, ppos);
-	if (write && (*valp != val)) {
-		if ((*valp < 0) || (*valp > 1)) {
-			/* Restore the correct value */
-			*valp = val;
-		}
+	rc = proc_dointvec(&tmp, write, buffer, lenp, ppos);
+	if (write) {
+		if (val[0] < 0 || val[1] < 0 ||
+		    (val[0] >= val[1] && val[1]))
+			rc = -EINVAL;
+		else
+			memcpy(valp, val, sizeof(val));
 	}
 	return rc;
 }

@@ -1717,12 +1711,18 @@ proc_do_sync_ports(struct ctl_table *table, int write,
 	int val = *valp;
 	int rc;
 
-	rc = proc_dointvec(table, write, buffer, lenp, ppos);
+	struct ctl_table tmp = {
+		.data = &val,
+		.maxlen = sizeof(int),
+		.mode = table->mode,
+	};
+
+	rc = proc_dointvec(&tmp, write, buffer, lenp, ppos);
 	if (write && (*valp != val)) {
-		if (*valp < 1 || !is_power_of_2(*valp)) {
-			/* Restore the correct value */
+		if (val < 1 || !is_power_of_2(val))
+			rc = -EINVAL;
+		else
 			*valp = val;
-		}
 	}
 	return rc;
 }

@@ -1782,7 +1782,9 @@ static struct ctl_table vs_vars[] = {
 		.procname	= "sync_version",
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
-		.proc_handler	= proc_do_sync_mode,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= &zero,
+		.extra2		= &one,
 	},
 	{
 		.procname	= "sync_ports",

Signature · 1 line

2.41.0

Greg KH

author maintainer

2023-08-26 16:36 UTC #4 lore raw

Re: [PATCH -stable,4.14.y,4.19.y 0/1] ipvs: backport 1b90af292e71 and 5310760af1d4

In reply to Julian Anastasov

Quoted 11 lines · Julian Anastasov

Hello,
This patchset contains backport for
commit 1b90af292e71b20d03b837d39406acfbdc5d4b2a. It applies to linux-4.14.y and linux-4.19.y and differs from original commit for the zero/one values used for extra1/extra2.
When applied, the concerned commit
5310760af1d4fbea1452bfc77db5f9a680f7ae47 can be cherry-picked and it will apply cleanly on top of 1b90af292e71.

Quoted 10 lines

Junwei Hu (1):
ipvs: Improve robustness to the ipvs sysctl
 net/netfilter/ipvs/ip_vs_ctl.c | 70 +++++++++++++++++-----------------
 1 file changed, 36 insertions(+), 34 deletions(-)
Signature · 1 line
2.41.0

Now queued up, thanks.

greg k-h