| 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: <CANP3RGeeR9vso0MyjRhFuTmx5K7ttt0bisHucce0ONeJotXOZw@mail.gmail.com>
Date: Thu, 17 Oct 2024 12:39:57 +0200
From: Maciej Żenczykowski <maze@...gle.com>
To: Florian Westphal <fw@...len.de>
Cc: netdev@...r.kernel.org, Nathan Harold <nharold@...gle.com>,
Steffen Klassert <steffen.klassert@...unet.com>, Yan Yan <evitayan@...gle.com>
Subject: Re: [PATCH ipsec] xfrm: migrate: work around 0 if_id on migrate
On Thu, Oct 17, 2024 at 12:33 PM Florian Westphal <fw@...len.de> wrote:
>
> 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
>
Q: Considering the performance impact... would it make sense to hide
this behind a sysctl or a kconfig?
Yan Yan: Also, while I know we found this issue in Android... do we
actually need the fix? Wasn't the adjustment to netd sufficient?
Android <16 doesn't support >6.6 anyway, and Android 16 should already
have the netd fix...
Powered by blists - more mailing lists