On Wed, 2017-11-01 at 00:08 +0100, Florian Westphal wrote:
> Paul Moore <p...@paul-moore.com> wrote:
> > On Mon, Oct 30, 2017 at 10:58 AM, Stephen Smalley <s...@tycho.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 <s...@tycho.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)) {
