netdev - Re: [PATCH ipsec] xfrm: do unconditional template resolution before pcpu cache check

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHC9VhSikfCPf6_c-ts6FLsw56k_tpBGGMDD4909zP=V_Va+tQ@mail.gmail.com>
Date:   Thu, 2 Nov 2017 18:57:29 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     Florian Westphal <fw@...len.de>
Cc:     netdev@...r.kernel.org, Stephen Smalley <sds@...ho.nsa.gov>,
        steffen.klassert@...unet.com
Subject: Re: [PATCH ipsec] xfrm: do unconditional template resolution before
 pcpu cache check

On Thu, Nov 2, 2017 at 11:46 AM, Florian Westphal <fw@...len.de> wrote:
> Stephen Smalley says:
>  Since 4.14-rc1, the selinux-testsuite has been encountering sporadic
>  failures during testing of labeled IPSEC. git bisect pointed to
>  commit ec30d ("xfrm: add xdst pcpu cache").
>  The xdst pcpu cache is only checking that the policies are the same,
>  but does not validate that the policy, state, and flow match with respect
>  to security context labeling.
>  As a result, the wrong SA could be used and the receiver could end up
>  performing permission checking and providing SO_PEERSEC or SCM_SECURITY
>  values for the wrong security context.
>
> This fix makes it so that we always do the template resolution, and
> then checks that the found states match those in the pcpu bundle.
>
> This has the disadvantage of doing a bit more work (lookup in state hash
> table) if we can reuse the xdst entry (we only avoid xdst alloc/free)
> but we don't add a lot of extra work in case we can't reuse.
>
> xfrm_pol_dead() check is removed, reasoning is that
> xfrm_tmpl_resolve does all needed checks.
>
> Cc: Paul Moore <paul@...l-moore.com>
> Fixes: ec30d78c14a813db39a647b6a348b428 ("xfrm: add xdst pcpu cache")
> Reported-by: Stephen Smalley <sds@...ho.nsa.gov>
> Tested-by: Stephen Smalley <sds@...ho.nsa.gov>
> Signed-off-by: Florian Westphal <fw@...len.de>
> ---
>  net/xfrm/xfrm_policy.c | 42 ++++++++++++++++++++++++------------------
>  1 file changed, 24 insertions(+), 18 deletions(-)

This looks reasonable and seems like probably the simplest approach to
me.  I'm building a test kernel with it now, but considering the time
of day here, I probably will not be able to test it until tomorrow
morning; however it is important to note that Stephen did test this
already so please don't wait on my test results - we are likely to be
running the same tests anyway.

Acked-by: Paul Moore <paul@...l-moore.com>

> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index 8cafb3c0a4ac..a2e531bf4f97 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -1787,19 +1787,23 @@ void xfrm_policy_cache_flush(void)
>         put_online_cpus();
>  }
>
> -static bool xfrm_pol_dead(struct xfrm_dst *xdst)
> +static bool xfrm_xdst_can_reuse(struct xfrm_dst *xdst,
> +                               struct xfrm_state * const xfrm[],
> +                               int num)
>  {
> -       unsigned int num_pols = xdst->num_pols;
> -       unsigned int pol_dead = 0, i;
> +       const struct dst_entry *dst = &xdst->u.dst;
> +       int i;
>
> -       for (i = 0; i < num_pols; i++)
> -               pol_dead |= xdst->pols[i]->walk.dead;
> +       if (xdst->num_xfrms != num)
> +               return false;
>
> -       /* Mark DST_OBSOLETE_DEAD to fail the next xfrm_dst_check() */
> -       if (pol_dead)
> -               xdst->u.dst.obsolete = DST_OBSOLETE_DEAD;
> +       for (i = 0; i < num; i++) {
> +               if (!dst || dst->xfrm != xfrm[i])
> +                       return false;
> +               dst = dst->child;
> +       }
>
> -       return pol_dead;
> +       return xfrm_bundle_ok(xdst);
>  }
>
>  static struct xfrm_dst *
> @@ -1813,26 +1817,28 @@ xfrm_resolve_and_create_bundle(struct xfrm_policy **pols, int num_pols,
>         struct dst_entry *dst;
>         int err;
>
> +       /* Try to instantiate a bundle */
> +       err = xfrm_tmpl_resolve(pols, num_pols, fl, xfrm, family);
> +       if (err <= 0) {
> +               if (err != 0 && err != -EAGAIN)
> +                       XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTPOLERROR);
> +               return ERR_PTR(err);
> +       }
> +
>         xdst = this_cpu_read(xfrm_last_dst);
>         if (xdst &&
>             xdst->u.dst.dev == dst_orig->dev &&
>             xdst->num_pols == num_pols &&
> -           !xfrm_pol_dead(xdst) &&
>             memcmp(xdst->pols, pols,
>                    sizeof(struct xfrm_policy *) * num_pols) == 0 &&
> -           xfrm_bundle_ok(xdst)) {
> +           xfrm_xdst_can_reuse(xdst, xfrm, err)) {
>                 dst_hold(&xdst->u.dst);
> +               while (err > 0)
> +                       xfrm_state_put(xfrm[--err]);
>                 return xdst;
>         }
>
>         old = xdst;
> -       /* Try to instantiate a bundle */
> -       err = xfrm_tmpl_resolve(pols, num_pols, fl, xfrm, family);
> -       if (err <= 0) {
> -               if (err != 0 && err != -EAGAIN)
> -                       XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTPOLERROR);
> -               return ERR_PTR(err);
> -       }
>
>         dst = xfrm_bundle_create(pols[0], xfrm, err, fl, dst_orig);
>         if (IS_ERR(dst)) {
> --
> 2.13.6
>



-- 
paul moore
www.paul-moore.com