netdev - Re: [RFC PATCH] xfrm: fix regression introduced by xdst pcpu cache

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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1509545152.24459.4.camel@tycho.nsa.gov>
Date:   Wed, 01 Nov 2017 10:05:52 -0400
From:   Stephen Smalley <sds@...ho.nsa.gov>
To:     Florian Westphal <fw@...len.de>, Paul Moore <paul@...l-moore.com>
Cc:     herbert@...dor.apana.org.au, netdev@...r.kernel.org,
        linux-security-module@...r.kernel.org, selinux@...ho.nsa.gov,
        davem@...emloft.net
Subject: Re: [RFC PATCH] xfrm: fix regression introduced by xdst pcpu cache

On Wed, 2017-11-01 at 00:08 +0100, Florian Westphal wrote:
> Paul Moore <paul@...l-moore.com> wrote:
> > On Mon, Oct 30, 2017 at 10:58 AM, Stephen Smalley <sds@...ho.nsa.go
> > v> wrote:
> > > matching before (as in this patch) or after calling
> > > xfrm_bundle_ok()?
> > 
> > I would probably make the LSM call the last check, as you've done;
> > but
> > I have to say that is just so it is consistent with the "LSM last"
> > philosophy and not because of any performance related argument.
> > 
> > > ... Also,
> > > do we need to test xfrm->sel.family before calling
> > > xfrm_selector_match
> > > (as in this patch) or not - xfrm_state_look_at() does so when the
> > > state is XFRM_STATE_VALID but not when it is _ERROR or _EXPIRED?
> > 
> > Speaking purely from a SELinux perspective, I'm not sure it
> > matters:
> > as long as the labels match we are happy.  However, from a general
> > IPsec perspective it does seem like a reasonable thing.
> > 
> > Granted I'm probably missing something, but it seems a little odd
> > that
> > the code isn't already checking that the selectors match (... what
> > am
> > I missing?).  It does check the policies, maybe that is enough in
> > the
> > normal IPsec case?
> 
> The assumption was that identical policies would yield the same SAs,
> but thats not correct.
> 
> > > diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> > > index 2746b62..171818b 100644
> > > --- a/net/xfrm/xfrm_policy.c
> > > +++ b/net/xfrm/xfrm_policy.c
> > > @@ -1820,6 +1820,11 @@ xfrm_resolve_and_create_bundle(struct
> > > xfrm_policy **pols, int num_pols,
> > >             !xfrm_pol_dead(xdst) &&
> > >             memcmp(xdst->pols, pols,
> > >                    sizeof(struct xfrm_policy *) * num_pols) == 0
> > > &&
> > > +           (!xdst->u.dst.xfrm->sel.family ||
> > > +            xfrm_selector_match(&xdst->u.dst.xfrm->sel, fl,
> > > +                                xdst->u.dst.xfrm->sel.family))
> > > &&
> > > +           security_xfrm_state_pol_flow_match(xdst->u.dst.xfrm,
> > > +                                              xdst->pols[0], fl)
> > > &&
> 
> ... so this needs to walk the bundle and validate each selector.
> 
> Alternatively we could always do template resolution and then check
> that all states found match those of the old pcpu xdst:

With your patch below, the selinux-testsuite passes, and I couldn't
trigger any failures even running the inet_socket tests repeatedly.

Tested-by: Stephen Smalley <sds@...ho.nsa.gov>

> 
> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -1786,19 +1786,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 *
> @@ -1812,26 +1816,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)) {