Message-ID: <CAHC9VhR9UMzh4euR87OaqcjfqodvEAr-KBLjGYa4C+Z7zVTZ9A@mail.gmail.com>
Date:   Thu, 2 Nov 2017 18:37:27 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     Stephen Smalley <sds@...ho.nsa.gov>
Cc:     Florian Westphal <fw@...len.de>, 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 Thu, Nov 2, 2017 at 8:58 AM, Stephen Smalley <sds@...ho.nsa.gov> wrote:
> On Wed, 2017-11-01 at 17:39 -0400, Paul Moore wrote:
>> On Tue, Oct 31, 2017 at 7:08 PM, Florian Westphal <fw@...len.de>
>> wrote:
>> > Paul Moore <paul@...l-moore.com> wrote:
>> > > On Mon, Oct 30, 2017 at 10:58 AM, Stephen Smalley <sds@...ho.nsa.
>> > > gov> 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.
>>
>> Well, to be fair, I think the assumption is valid for normal,
>> unlabeled IPsec.  The problem comes when SELinux starts labeling SAs
>> and now you have multiple SAs for a given policy, each differing only
>> in the SELinux/LSM label.
>
> No, it is invalid for normal, unlabeled IPSEC too, in the case where
> one has defined xfrm state selectors.  That's what my other testsuite
> patch (which is presently only on the xfrmselectortest branch) is
> exercising - matching of xfrm state selectors.  But in any event,
> Florian's patch fixes both, so I'm fine with it.  I don't know though
> how it compares performance-wise with walking the bundle and just
> calling security_xfrm_state_pol_flow_match() and xfrm_selector_match()
> on each one.
>
>> Considering that adding the SELinux/LSM label effectively adds an
>> additional selector, I'm wondering if we should simply add the
>> SELinux/LSM label matching to xfrm_selector_match()?  Looking quickly
>> at the code it seems as though we always follow xfrm_selector_match()
>> with a LSM check anyway, the one exception being in
>> __xfrm_policy_check() ... which *might* be a valid exception, as we
>> don't do our access checks for inbound traffic at that point in the
>> stack.
>
> Possibly, but that should probably be a separate patch. We should just
> fix this regression for 4.14, either via Florian's patch or by
> augmenting my patch to perform the matching calls on all of the xfrms.

I agree that v4.14 should get the smallest patch possible that fixes
the problem.  I was just looking at the patches presented so far and
thinking out loud.

-- 
paul moore
www.paul-moore.com

Powered by blists - more mailing lists