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

[Steffen Klassert]

On Thu, Nov 02, 2017 at 06:57:29PM -0400, Paul Moore wrote:
> On Thu, Nov 2, 2017 at 11:46 AM, Florian Westphal  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 
> > Fixes: ec30d78c14a813db39a647b6a348b428 ("xfrm: add xdst pcpu cache")
> > Reported-by: Stephen Smalley 
> > Tested-by: Stephen Smalley 
> > Signed-off-by: Florian Westphal 
> > ---
> >  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 

Patch applied, thanks everyone!

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

[Paul Moore]

On Thu, Nov 2, 2017 at 11:46 AM, Florian Westphal  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 
> Fixes: ec30d78c14a813db39a647b6a348b428 ("xfrm: add xdst pcpu cache")
> Reported-by: Stephen Smalley 
> Tested-by: Stephen Smalley 
> Signed-off-by: Florian Westphal 
> ---
>  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 

> 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 = >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(>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

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

[Florian Westphal]

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 
Fixes: ec30d78c14a813db39a647b6a348b428 ("xfrm: add xdst pcpu cache")
Reported-by: Stephen Smalley 
Tested-by: Stephen Smalley 
Signed-off-by: Florian Westphal 
---
 net/xfrm/xfrm_policy.c | 42 --
 1 file changed, 24 insertions(+), 18 deletions(-)

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 = >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(>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