From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steffen Klassert Subject: Re: [PATCH ipsec] xfrm: do unconditional template resolution before pcpu cache check Date: Fri, 3 Nov 2017 10:27:40 +0100 Message-ID: <20171103092740.GQ11292@secunet.com> References: <20171102154601.25590-1-fw@strlen.de> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Cc: Florian Westphal , , Stephen Smalley To: Paul Moore Return-path: Received: from a.mx.secunet.com ([62.96.220.36]:35736 "EHLO a.mx.secunet.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754185AbdKCJ1n (ORCPT ); Fri, 3 Nov 2017 05:27:43 -0400 Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: 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!