FAILED: patch "[PATCH] ipvs: fix racy memcpy in proc_do_sync_threshold" failed to apply to 4.19-stable tree
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 stable@vger.kernel.org.
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 'stable@vger.kernel.org' --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 sishuai.system@gmail.com 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/B6988E90-0A1E-4B85-BF26-2DAF6D482433@gmail.co... Signed-off-by: Sishuai Gong sishuai.system@gmail.com Acked-by: Simon Horman horms@kernel.org Acked-by: Julian Anastasov ja@ssi.bg Signed-off-by: Florian Westphal fw@strlen.de
diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c index 62606fb44d02..4bb0d90eca1c 100644 --- a/net/netfilter/ipvs/ip_vs_ctl.c +++ b/net/netfilter/ipvs/ip_vs_ctl.c @@ -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;
Hello,
On Mon, 21 Aug 2023, gregkh@linuxfoundation.org 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 stable@vger.kernel.org.
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 'stable@vger.kernel.org' --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")
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?
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
From 5310760af1d4fbea1452bfc77db5f9a680f7ae47 Mon Sep 17 00:00:00 2001
From: Sishuai Gong sishuai.system@gmail.com 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/B6988E90-0A1E-4B85-BF26-2DAF6D482433@gmail.co... Signed-off-by: Sishuai Gong sishuai.system@gmail.com Acked-by: Simon Horman horms@kernel.org Acked-by: Julian Anastasov ja@ssi.bg Signed-off-by: Florian Westphal fw@strlen.de
diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c index 62606fb44d02..4bb0d90eca1c 100644 --- a/net/netfilter/ipvs/ip_vs_ctl.c +++ b/net/netfilter/ipvs/ip_vs_ctl.c @@ -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;
Regards
-- Julian Anastasov ja@ssi.bg
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(-)
From: Junwei Hu hujunwei4@huawei.com
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 hujunwei4@huawei.com Acked-by: Julian Anastasov ja@ssi.bg Signed-off-by: Pablo Neira Ayuso pablo@netfilter.org (cherry picked from commit 1b90af292e71b20d03b837d39406acfbdc5d4b2a) [Julian: Backport by changing SYSCTL_ZERO/SYSCTL_ONE to zero/one] Signed-off-by: Julian Anastasov ja@ssi.bg --- net/netfilter/ipvs/ip_vs_ctl.c | 70 +++++++++++++++++----------------- 1 file changed, 36 insertions(+), 34 deletions(-)
diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c index ecc16d8c1cc3..4e78c2a6a3ca 100644 --- a/net/netfilter/ipvs/ip_vs_ctl.c +++ b/net/netfilter/ipvs/ip_vs_ctl.c @@ -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",
On Thu, Aug 24, 2023 at 02:53:53PM +0300, Julian Anastasov wrote:
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(-)
-- 2.41.0
Now queued up, thanks.
greg k-h
linux-stable-mirror@lists.linaro.org
-
Greg KH -
gregkh＠linuxfoundation.org
-
Julian Anastasov