| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20241017094315.6948-1-fw@strlen.de>
Date: Thu, 17 Oct 2024 11:43:05 +0200
From: Florian Westphal <fw@...len.de>
To: <netdev@...r.kernel.org>
Cc: Florian Westphal <fw@...len.de>,
Nathan Harold <nharold@...gle.com>,
Maciej Żenczykowski <maze@...gle.com>,
Steffen Klassert <steffen.klassert@...unet.com>,
Yan Yan <evitayan@...gle.com>
Subject: [PATCH ipsec] xfrm: migrate: work around 0 if_id on migrate
Looks like there are userspace applications which rely on the ability to
migrate xfrm-interface policies while not providing the interface id.
This worked because old code contains a match workaround:
"if_id == 0 || pol->if_id == if_id".
The packetpath lookup code uses the id_id as a key into rhashtable; after
switch of migrate lookup over to those functions policy lookup fails.
Add a workaround: if no policy to migrate is found and userspace provided
no if_id (normal for non-interface policies!) do a full search of all
policies and try to find one that matches the selector.
This is super-slow, so print a warning when we find a policy, so
hopefully such userspace requests are fixed up over time to not rely on
magic-0-match.
In case of setups where userspace frequently tries to migrate non-existent
policies with if_id 0 such migrate requests will take much longer to
complete.
Other options:
1. also add xfrm interface usage counter so we know in advance if the
slowpath could potentially find the 'right' policy or not.
2. Completely revert policy insertion patches and go back to linear
insertion complexity.
1) seems premature, I'd expect userspace to try migration of policies it
has inserted in the past, so either normal fastpath or slowpath
should find a match anyway.
2) seems like a worse option to me.
Cc: Nathan Harold <nharold@...gle.com>
Cc: Maciej Żenczykowski <maze@...gle.com>
Cc: Steffen Klassert <steffen.klassert@...unet.com>
Fixes: 563d5ca93e88 ("xfrm: switch migrate to xfrm_policy_lookup_bytype")
Reported-by: Yan Yan <evitayan@...gle.com>
Tested-by: Yan Yan <evitayan@...gle.com>
Signed-off-by: Florian Westphal <fw@...len.de>
---
net/xfrm/xfrm_policy.c | 101 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 101 insertions(+)
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index d555ea401234..29554173831a 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -4425,6 +4425,81 @@ EXPORT_SYMBOL_GPL(xfrm_audit_policy_delete);
#endif
#ifdef CONFIG_XFRM_MIGRATE
+static bool xfrm_migrate_selector_match(const struct xfrm_selector *sel_cmp,
+ const struct xfrm_selector *sel_tgt)
+{
+ if (sel_cmp->proto == IPSEC_ULPROTO_ANY) {
+ if (sel_tgt->family == sel_cmp->family &&
+ xfrm_addr_equal(&sel_tgt->daddr, &sel_cmp->daddr,
+ sel_cmp->family) &&
+ xfrm_addr_equal(&sel_tgt->saddr, &sel_cmp->saddr,
+ sel_cmp->family) &&
+ sel_tgt->prefixlen_d == sel_cmp->prefixlen_d &&
+ sel_tgt->prefixlen_s == sel_cmp->prefixlen_s) {
+ return true;
+ }
+ } else {
+ if (memcmp(sel_tgt, sel_cmp, sizeof(*sel_tgt)) == 0)
+ return true;
+ }
+ return false;
+}
+
+/* Ugly workaround for userspace that wants to migrate policies for
+ * xfrm interfaces but does not provide the interface if_id.
+ *
+ * Old code used to search the lists and handled if_id == 0 as 'does match'.
+ * New xfrm_migrate code uses the packet-path lookup which uses the if_id
+ * as part of hash key and won't find correct policies.
+ *
+ * Walk entire policy list to see if there is a matching selector without
+ * checking if_id.
+ */
+static u32 xfrm_migrate_policy_find_slow(const struct xfrm_selector *sel,
+ u8 dir, u8 type, struct net *net)
+{
+ const struct xfrm_policy *policy, *cand = NULL;
+ const struct hlist_head *chain;
+ u32 if_id = 0;
+
+ chain = policy_hash_direct(net, &sel->daddr, &sel->saddr, sel->family, dir);
+ hlist_for_each_entry(policy, chain, bydst) {
+ if (policy->type != type)
+ continue;
+
+ if (xfrm_migrate_selector_match(sel, &policy->selector)) {
+ if_id = policy->if_id;
+ cand = policy;
+ break;
+ }
+ }
+
+ spin_lock_bh(&net->xfrm.xfrm_policy_lock);
+
+ list_for_each_entry(policy, &net->xfrm.policy_all, walk.all) {
+ if (xfrm_policy_is_dead_or_sk(policy))
+ continue;
+
+ if (policy->type != type)
+ continue;
+
+ /* candidate has better priority */
+ if (cand && policy->priority >= cand->priority)
+ continue;
+
+ if (dir != xfrm_policy_id2dir(policy->index))
+ continue;
+
+ if (xfrm_migrate_selector_match(sel, &policy->selector)) {
+ if_id = policy->if_id;
+ cand = policy;
+ }
+ }
+ spin_unlock_bh(&net->xfrm.xfrm_policy_lock);
+
+ return if_id;
+}
+
static struct xfrm_policy *xfrm_migrate_policy_find(const struct xfrm_selector *sel,
u8 dir, u8 type, struct net *net, u32 if_id)
{
@@ -4579,6 +4654,19 @@ static int xfrm_migrate_check(const struct xfrm_migrate *m, int num_migrate,
return 0;
}
+static void xfrm_migrate_warn_workaround(void)
+{
+ char name[sizeof(current->comm)];
+ static bool warned;
+
+ if (warned)
+ return;
+
+ warned = true;
+ pr_warn_once("warning: `%s' is migrating xfrm interface policies with if_id 0, this is slow.\n",
+ get_task_comm(name, current));
+}
+
int xfrm_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
struct xfrm_migrate *m, int num_migrate,
struct xfrm_kmaddress *k, struct net *net,
@@ -4606,11 +4694,24 @@ int xfrm_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
/* Stage 1 - find policy */
pol = xfrm_migrate_policy_find(sel, dir, type, net, if_id);
if (IS_ERR_OR_NULL(pol)) {
+ if (if_id == 0) {
+ if_id = xfrm_migrate_policy_find_slow(sel, dir, type, net);
+
+ if (if_id) {
+ pol = xfrm_migrate_policy_find(sel, dir, type, net, if_id);
+ if (!IS_ERR_OR_NULL(pol)) {
+ xfrm_migrate_warn_workaround();
+ goto found;
+ }
+ }
+ }
+
NL_SET_ERR_MSG(extack, "Target policy not found");
err = IS_ERR(pol) ? PTR_ERR(pol) : -ENOENT;
goto out;
}
+found:
/* Stage 2 - find and update state(s) */
for (i = 0, mp = m; i < num_migrate; i++, mp++) {
if ((x = xfrm_migrate_state_find(mp, net, if_id))) {
--
2.45.2
Powered by blists - more mailing lists