On Thu, Nov 2, 2017 at 11:46 AM, Florian Westphal <f...@strlen.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 <p...@paul-moore.com> > Fixes: ec30d78c14a813db39a647b6a348b428 ("xfrm: add xdst pcpu cache") > Reported-by: Stephen Smalley <s...@tycho.nsa.gov> > Tested-by: Stephen Smalley <s...@tycho.nsa.gov> > Signed-off-by: Florian Westphal <f...@strlen.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 <p...@paul-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