[PATCH]: SAD sometimes has double SAs.

[PATCH]: SAD sometimes has double SAs.

[Joy Latten]

Last Friday I proposed creating larval SAs to act as
placeholders to prevent a second acquire resulting in 
double SAs being created. 
I tried this and so far I have not seen any double SAs
being created. I also plan to run some stress tests over 
the weekend.

Please let me know what improvements I can make to this patch or
if there is a better way to do this.

> A while back I reported that I sometimes saw double and triple 
> SAs being created. The patch to check for protocol when deleting
> larval SA removed one obstacle in that I no longer see triple SAs. 
> Now, once in a while double SAs. I think I have figured out the
> second obstacle.
> 
> The initiator installs his SAs into the kernel before the responder.
> As soon as they are installed, the blocked packet (which started
> the ACQUIRE) is sent. By this time the responder has installed his
> inbound SA(s) and so the newly arrived ipsec packet can be processed.
> In the case of tcp connections and a ping, a response may be 
> warranted, and thus an outgoing packet results. 
> 
> >From what I can tell of the log file below, sometimes, this
> might happen before the responder has completed installing 
> the outbound SAs. In the log file, the outbound AH has been added, 
> but not the outbound ESP, which is the one the outgoing packet 
> looks for first. Thus resulting in a second acquire. 
> 
> I think this becomes more problematic when using both AH AND ESP, 
> rather than just using ESP with authentication. With the latter, 
> only one SA needed thus reducing the latency in installing the 
> SAs before incoming packet arrives.     
> 
> So far, the only solution I can think of besides mandating all 
> userspace key daemons do SA maintenance is to perhaps add larval 
> SAs for both directions when adding the SPI. Currently, responder 
> does GETSPI for one way and initiator for opposite. When GETSPI is
> called, larval SA is created containing the SPI, but it is only 
> for one direction. Perhaps we can add a larval SA (no SPI) for 
> opposite direction to act as a placeholder indicating ACQUIRE 
> occurring, since SAs are created for both directions during an ACQUIRE.
> The initiator may have larval SA from GETSPI and larval SA from the
> ACQUIRE depending that GETSPI is in opposite direction of ACQUIRE.
> Calling __find_acq_core() should ensure we don't create duplicate 
> larval SAs. Also, should IKE negotiations return error, larval SAs
> should expire. They also should be removed when we do the
> xfrm_state_add() and xfrm_state_update() to add the new SAs.
>  

Joy


This patch is against linux-2.6.21-rc4-git5

Signed-off-by: Joy Latten<latten@austin.ibm.com>


diff -urpN linux-2.6.20.orig/net/xfrm/xfrm_state.c linux-2.6.20/net/xfrm/xfrm_state.c
--- linux-2.6.20.orig/net/xfrm/xfrm_state.c	2007-03-20 22:39:15.000000000 -0500
+++ linux-2.6.20/net/xfrm/xfrm_state.c	2007-03-23 16:38:37.000000000 -0500
@@ -692,12 +692,15 @@ void xfrm_state_insert(struct xfrm_state
 }
 EXPORT_SYMBOL(xfrm_state_insert);
 
+static struct xfrm_state *create_larval_sa(unsigned short family, u8 mode, u32 reqid, u8 proto, xfrm_address_t *daddr, xfrm_address_t *saddr);
+
 /* xfrm_state_lock is held */
 static struct xfrm_state *__find_acq_core(unsigned short family, u8 mode, u32 reqid, u8 proto, xfrm_address_t *daddr, xfrm_address_t *saddr, int create)
 {
 	unsigned int h = xfrm_dst_hash(daddr, saddr, reqid, family);
 	struct hlist_node *entry;
-	struct xfrm_state *x;
+	struct xfrm_state *x, *x1;
+	int track_opposite = 0;
 
 	hlist_for_each_entry(x, entry, xfrm_state_bydst+h, bydst) {
 		if (x->props.reqid  != reqid ||
@@ -710,11 +713,20 @@ static struct xfrm_state *__find_acq_cor
 
 		switch (family) {
 		case AF_INET:
+			if (x->id.daddr.a4 == saddr->a4 &&
+			    x->props.saddr.a4 == daddr->a4)
+				track_opposite = 1;
 			if (x->id.daddr.a4    != daddr->a4 ||
 			    x->props.saddr.a4 != saddr->a4)
 				continue;
 			break;
 		case AF_INET6:
+			if (ipv6_addr_equal((struct in6_addr *)x->id.daddr.a6,
+					     (struct in6_addr *)saddr) ||
+			    ipv6_addr_equal((struct in6_addr *)
+					     x->props.saddr.a6,
+					     (struct in6_addr *)daddr))
+					track_opposite = 1;
 			if (!ipv6_addr_equal((struct in6_addr *)x->id.daddr.a6,
 					     (struct in6_addr *)daddr) ||
 			    !ipv6_addr_equal((struct in6_addr *)
@@ -731,6 +743,27 @@ static struct xfrm_state *__find_acq_cor
 	if (!create)
 		return NULL;
 
+	x = create_larval_sa(family, mode, reqid, proto, daddr, saddr);
+	
+	/* create a larval SA for opposite direction to act as a 
+	   placeholder duing the ACQUIRE. This prevents any additional
+	   packets from creating additional ACQUIRES for same traffic
+	   stream.
+ 	*/
+	
+	if (!track_opposite) 
+		x1 = create_larval_sa(family, mode, reqid, proto, saddr, daddr);
+	
+	wake_up(&km_waitq);
+	return x;
+}
+
+/* xfrm_state_lock is held */
+static struct xfrm_state *create_larval_sa(unsigned short family, u8 mode, u32 reqid, u8 proto, xfrm_address_t *daddr, xfrm_address_t *saddr)
+{
+	struct xfrm_state *x;
+	unsigned int h = xfrm_dst_hash(daddr, saddr, reqid, family);
+	
 	x = xfrm_state_alloc();
 	if (likely(x)) {
 		switch (family) {
@@ -769,15 +802,14 @@ static struct xfrm_state *__find_acq_cor
 		hlist_add_head(&x->bydst, xfrm_state_bydst+h);
 		h = xfrm_src_hash(daddr, saddr, family);
 		hlist_add_head(&x->bysrc, xfrm_state_bysrc+h);
-		wake_up(&km_waitq);
 
 		xfrm_state_num++;
 
 		xfrm_hash_grow_check(x->bydst.next != NULL);
-	}
 
-	return x;
-}
+	};
+		return x;
+};
 
 static struct xfrm_state *__xfrm_find_acq_byseq(u32 seq);
 

Re: [PATCH]: SAD sometimes has double SAs.

[David Miller]

From: Joy Latten <latten@austin.ibm.com>
Date: Fri, 23 Mar 2007 16:58:20 -0600

> Last Friday I proposed creating larval SAs to act as
> placeholders to prevent a second acquire resulting in 
> double SAs being created. 
> I tried this and so far I have not seen any double SAs
> being created. I also plan to run some stress tests over 
> the weekend.
> 
> Please let me know what improvements I can make to this patch or
> if there is a better way to do this.

I'll take a look at your patch after I deal with some ipv6
locking bugs, thanks Joy.

Re: [PATCH]: SAD sometimes has double SAs.

[Eric Paris]

On Fri, 2007-03-23 at 16:58 -0600, Joy Latten wrote:

> @@ -710,11 +713,20 @@ static struct xfrm_state *__find_acq_cor
>  
>  		switch (family) {
>  		case AF_INET:
> +			if (x->id.daddr.a4 == saddr->a4 &&
> +			    x->props.saddr.a4 == daddr->a4)
> +				track_opposite = 1;
>  			if (x->id.daddr.a4    != daddr->a4 ||
>  			    x->props.saddr.a4 != saddr->a4)
>  				continue;
>  			break;
>  		case AF_INET6:
> +			if (ipv6_addr_equal((struct in6_addr *)x->id.daddr.a6,
> +					     (struct in6_addr *)saddr) ||
> +			    ipv6_addr_equal((struct in6_addr *)
> +					     x->props.saddr.a6,
> +					     (struct in6_addr *)daddr))
> +					track_opposite = 1;
>  			if (!ipv6_addr_equal((struct in6_addr *)x->id.daddr.a6,
>  					     (struct in6_addr *)daddr) ||
>  			    !ipv6_addr_equal((struct in6_addr *)

I'm not at all able to speak on the correctness or validity of the
solution, but shouldn't the ipv6 case be a && not an || like the ipv4
case?  Isn't this going to match all sorts of things?  Did you test this
patch on ipv6 and see it to solve your problem?

I'm also not enjoying the formatting in the ipv6 part where the first
time you have the cast on the same time as the object but not the second
part where x->props.saddr.a6 is on its own little line.

-Eric

Re: [PATCH]: SAD sometimes has double SAs.

[David Miller]

From: Eric Paris <eparis@redhat.com>
Date: Mon, 26 Mar 2007 17:34:59 -0400

> I'm not at all able to speak on the correctness or validity of the
> solution,

Neither am I yet :)

> but shouldn't the ipv6 case be a && not an || like the ipv4
> case?  Isn't this going to match all sorts of things?  Did you test this
> patch on ipv6 and see it to solve your problem?
> 
> I'm also not enjoying the formatting in the ipv6 part where the first
> time you have the cast on the same time as the object but not the second
> part where x->props.saddr.a6 is on its own little line.

Also, I want to understand what is going to tear down these
"other direction" fake entries later on?  I think I can review
this patch better if I understand that.

As it stands, this looks to me like a workaround for an improperly
implemented IPSEC daemon.  Joy states it as saying that the current
code requires the keying daemon to manage it's SAs, and I wonder
whether any other implementation is even valid.

There is a limit to the amount to which we can workaround racoon's
design issues. :-)

Re: [PATCH]: SAD sometimes has double SAs.

[Joy Latten]

On Mon, 2007-03-26 at 17:34 -0400, Eric Paris wrote:
> On Fri, 2007-03-23 at 16:58 -0600, Joy Latten wrote:
> 
> > @@ -710,11 +713,20 @@ static struct xfrm_state *__find_acq_cor
> >  
> >  		switch (family) {
> >  		case AF_INET:
> > +			if (x->id.daddr.a4 == saddr->a4 &&
> > +			    x->props.saddr.a4 == daddr->a4)
> > +				track_opposite = 1;
> >  			if (x->id.daddr.a4    != daddr->a4 ||
> >  			    x->props.saddr.a4 != saddr->a4)
> >  				continue;
> >  			break;
> >  		case AF_INET6:
> > +			if (ipv6_addr_equal((struct in6_addr *)x->id.daddr.a6,
> > +					     (struct in6_addr *)saddr) ||
> > +			    ipv6_addr_equal((struct in6_addr *)
> > +					     x->props.saddr.a6,
> > +					     (struct in6_addr *)daddr))
> > +					track_opposite = 1;
> >  			if (!ipv6_addr_equal((struct in6_addr *)x->id.daddr.a6,
> >  					     (struct in6_addr *)daddr) ||
> >  			    !ipv6_addr_equal((struct in6_addr *)
> 
> I'm not at all able to speak on the correctness or validity of the
> solution, but shouldn't the ipv6 case be a && not an || like the ipv4
> case?  Isn't this going to match all sorts of things?  Did you test this
> patch on ipv6 and see it to solve your problem?
> 
Will fix this and resend. Sorry, forgot about ipv6. My mistake! :-( 

> I'm also not enjoying the formatting in the ipv6 part where the first
> time you have the cast on the same time as the object but not the second
> part where x->props.saddr.a6 is on its own little line.
> 
ok. 

Joy

Re: [PATCH]: SAD sometimes has double SAs.

[Joy Latten]

On Mon, 2007-03-26 at 14:48 -0700, David Miller wrote:
> From: Eric Paris <eparis@redhat.com>
> Date: Mon, 26 Mar 2007 17:34:59 -0400
> 
> > I'm not at all able to speak on the correctness or validity of the
> > solution,
> 
> Neither am I yet :)
> 
> > but shouldn't the ipv6 case be a && not an || like the ipv4
> > case?  Isn't this going to match all sorts of things?  Did you test this
> > patch on ipv6 and see it to solve your problem?
> > 
> > I'm also not enjoying the formatting in the ipv6 part where the first
> > time you have the cast on the same time as the object but not the second
> > part where x->props.saddr.a6 is on its own little line.
> 
> Also, I want to understand what is going to tear down these
> "other direction" fake entries later on?  I think I can review
> this patch better if I understand that.
> 
I am going to refer to the other-direction-placeholder as the "fake"
entry. And the larval SA that gets created for the new
spi as result of a GETSPI message as the "real" entry. 

The fake entry gets created when the real entry does and does not have
an spi. It shares some of the same properties of a "real" larval SA
(they are created using same code) and it's state
is marked as XFRM_STATE_ACQ. The real entry has a timeout. So, should
IKE negotiation fail, take too long, etc... it will eventually timeout
and be deleted. So does the fake entry. It will timeout and should be
eventually deleted. (I will test this part tomorrow for assurance.)
 
When the IKE negotiations are successful, xfrm_state_add() and the
xfrm_state_update() look for larval SAs in that they look for an SA with
same src, dst, etc... and with state==XFRM_STATE_ACQUIRE. Any that are
found are deleted and new SA added. This removes the real larval SA, and
should also remove the fake entry too.     

Of course, this is all based on my assumption that IKE will install
two SAs, one for incoming and one for outgoing. 

Hopefully this answers how fake entries will be removed. 
Admittedly, I may miss something or didn't understand something
correctly as I learn the code, so please let me know. 
There may even be a better solution that I don't readily see. 

> As it stands, this looks to me like a workaround for an improperly
> implemented IPSEC daemon.  Joy states it as saying that the current
> code requires the keying daemon to manage it's SAs, and I wonder
> whether any other implementation is even valid.
> 
My big mouth. :-) But yes, I do think more SA management in 
userspace would be ideal. 

This fix will hopefully ensure kernel doesn't send any
extra acquires regardless. 

Joy

Re: [PATCH]: SAD sometimes has double SAs.

[Joy Latten]

Last night I browsed the ipsec-tools code and saw places in
racoon where improvement would actually fix this
problem. I am working on a patch and will pursue this 
on the ipsec-tools mailing list.

I apologize for any inconvenience. 
Eric, sorry as I know you already patched lspp kernel
for testing. I strongly think this should be fixed
in userspace. 

The permission check before flushing does still
need to be added to kernel. 

Regards,
Joy
  
On Mon, 2007-03-26 at 19:04 -0600, Joy Latten wrote:
> On Mon, 2007-03-26 at 14:48 -0700, David Miller wrote:
> > From: Eric Paris <eparis@redhat.com>
> > Date: Mon, 26 Mar 2007 17:34:59 -0400
> > 
> > > I'm not at all able to speak on the correctness or validity of the
> > > solution,
> > 
> > Neither am I yet :)
> > 
> > > but shouldn't the ipv6 case be a && not an || like the ipv4
> > > case?  Isn't this going to match all sorts of things?  Did you test this
> > > patch on ipv6 and see it to solve your problem?
> > > 
> > > I'm also not enjoying the formatting in the ipv6 part where the first
> > > time you have the cast on the same time as the object but not the second
> > > part where x->props.saddr.a6 is on its own little line.
> > 
> > Also, I want to understand what is going to tear down these
> > "other direction" fake entries later on?  I think I can review
> > this patch better if I understand that.
> > 
> I am going to refer to the other-direction-placeholder as the "fake"
> entry. And the larval SA that gets created for the new
> spi as result of a GETSPI message as the "real" entry. 
> 
> The fake entry gets created when the real entry does and does not have
> an spi. It shares some of the same properties of a "real" larval SA
> (they are created using same code) and it's state
> is marked as XFRM_STATE_ACQ. The real entry has a timeout. So, should
> IKE negotiation fail, take too long, etc... it will eventually timeout
> and be deleted. So does the fake entry. It will timeout and should be
> eventually deleted. (I will test this part tomorrow for assurance.)
>  
> When the IKE negotiations are successful, xfrm_state_add() and the
> xfrm_state_update() look for larval SAs in that they look for an SA with
> same src, dst, etc... and with state==XFRM_STATE_ACQUIRE. Any that are
> found are deleted and new SA added. This removes the real larval SA, and
> should also remove the fake entry too.     
> 
> Of course, this is all based on my assumption that IKE will install
> two SAs, one for incoming and one for outgoing. 
> 
> Hopefully this answers how fake entries will be removed. 
> Admittedly, I may miss something or didn't understand something
> correctly as I learn the code, so please let me know. 
> There may even be a better solution that I don't readily see. 
> 
> > As it stands, this looks to me like a workaround for an improperly
> > implemented IPSEC daemon.  Joy states it as saying that the current
> > code requires the keying daemon to manage it's SAs, and I wonder
> > whether any other implementation is even valid.
> > 
> My big mouth. :-) But yes, I do think more SA management in 
> userspace would be ideal. 
> 
> This fix will hopefully ensure kernel doesn't send any
> extra acquires regardless. 
> 
> Joy
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

LSPP kernels (was Re: [PATCH]: SAD sometimes has double SAs).

[James Morris]

On Wed, 28 Mar 2007, Joy Latten wrote:

> Eric, sorry as I know you already patched lspp kernel
> for testing.

I think it'd be better to have the lspp kernel join the upstream workflow 
process, rather than being a shortcut into RHEL.

Please consider creating an lspp git tree (based off Linus' tree), then 
once patches there are tested and ready to submit upstream, post them here 
or selinux-list, where they can be reviewed and applied to either my or 
DaveM's git tree.

>From there, they'll be picked up in -mm for even wider testing then be 
merged into mainline as appropriate.  Then, they can be incorporated into 
distro devel kernels when they update their kernels, or backported to 
stable distro kernels as already reviewed & tested upstream patches.

If there are any objections, please respond.


- James
-- 
James Morris
<jmorris@namei.org>

Re: LSPP kernels (was Re: [PATCH]: SAD sometimes has double SAs).

[Paul Moore]

On Wednesday, March 28 2007 12:20:24 pm James Morris wrote:
> On Wed, 28 Mar 2007, Joy Latten wrote:
> > Eric, sorry as I know you already patched lspp kernel
> > for testing.
>
> I think it'd be better to have the lspp kernel join the upstream workflow
> process, rather than being a shortcut into RHEL.
>
> Please consider creating an lspp git tree (based off Linus' tree), then
> once patches there are tested and ready to submit upstream, post them here
> or selinux-list, where they can be reviewed and applied to either my or
> DaveM's git tree.
>
> From there, they'll be picked up in -mm for even wider testing then be
> merged into mainline as appropriate.  Then, they can be incorporated into
> distro devel kernels when they update their kernels, or backported to
> stable distro kernels as already reviewed & tested upstream patches.
>
> If there are any objections, please respond.

I think the original intent of the LSPP kernel "series" was to test patches 
before they were submitted to a wider audience (not too different from what 
you are describing).  Eric Paris became the LSPP/MLS group's Andrew Morton if 
you will :)

However, for whatever reason, things appear to have stumbled a bit in recent 
months and I think making an effort to move to a more standard approach based 
on current kernel development would be a step in the right direction.  This 
would probably make backports a bit more difficult but Eric's a smart guy and 
I'm sure he wouldn't mind :)

Does anyone have access to a public site we could use to host a git tree?  If 
no one has anything available (or is willing to maintain the tree) I might be 
able to do something.

-- 
paul moore
linux security @ hp

Re: LSPP kernels (was Re: [PATCH]: SAD sometimes has double SAs).

[Eric Paris]

On Wed, 2007-03-28 at 12:20 -0400, James Morris wrote:
> On Wed, 28 Mar 2007, Joy Latten wrote:
> 
> > Eric, sorry as I know you already patched lspp kernel
> > for testing.
> 
> I think it'd be better to have the lspp kernel join the upstream workflow 
> process, rather than being a shortcut into RHEL.
> 
> Please consider creating an lspp git tree (based off Linus' tree), then 
> once patches there are tested and ready to submit upstream, post them here 
> or selinux-list, where they can be reviewed and applied to either my or 
> DaveM's git tree.
> 
> From there, they'll be picked up in -mm for even wider testing then be 
> merged into mainline as appropriate.  Then, they can be incorporated into 
> distro devel kernels when they update their kernels, or backported to 
> stable distro kernels as already reviewed & tested upstream patches.
> 
> If there are any objections, please respond.

It is definitely NOT a shortcut into RHEL.  Nor is this government cert
effort (LSPP) being driven primary on RHEL code.  Not a single patch
will go into RHEL until it is upstream or in a tree to go upstream.
That is a given.  All development is being done upstream and then being
ported back to RHEL.  The LSPP kernel she mentioned is at this time
merely a testing ground for patches which may not quite be upstream
ready or are upstream but aren't in RHEL proper yet.  As it stands now
the LSPP kernel is carrying 22 patches on top of RHEL 5 GA (which is
2.6.18 based)  of those let me give you a breakdown.

12 are network related.
10 of those are in Linus's kernel
1 is not yet in miller's tree but i would expect it soon
1 is going to likely be dropped according to this thread

10 remaining patches are audit patches.

There is already a viro/audit-current.git tree on kernel.org where these
should be appearing.  I could make this a little easier for the audit
tree maintainer and make my own tree which he could pull from and then
push to Linus but a tree which should hold all of these does exist.  All
of them have been sent to the linux-audit mailing list and have been
commented on there.

I don't want to give the impression that upstream is not coming first.
All the work is being done upstream either on netdev or linux-audit and
then I pull it back into this LSPP kernel she talked about so that
people interested primarily in the testing necessary to meet that
particular government standard have a neat tidy little prebuild rpm to
work with.  Eventually all of these will show up in RHEL, but not until
all of the patches i'm dealing with are upstream.

-Eric

Re: LSPP kernels (was Re: [PATCH]: SAD sometimes has double SAs).

[James Morris]

On Wed, 28 Mar 2007, Eric Paris wrote:

> It is definitely NOT a shortcut into RHEL.

Ok, that was a poor choice of words on my part.

> I don't want to give the impression that upstream is not coming first.
> All the work is being done upstream either on netdev or linux-audit and
> then I pull it back into this LSPP kernel she talked about so that
> people interested primarily in the testing necessary to meet that
> particular government standard have a neat tidy little prebuild rpm to
> work with.  Eventually all of these will show up in RHEL, but not until
> all of the patches i'm dealing with are upstream.

It seems my understanding wasn't clear on the overall workflow.  If the 
consensus is to stay with this scheme, then please disregard my previous 
post.


-- 
James Morris
<jmorris@namei.org>

Re: LSPP kernels (was Re: [PATCH]: SAD sometimes has double SAs).

[David Miller]

From: Paul Moore <paul.moore@hp.com>
Date: Wed, 28 Mar 2007 12:36:57 -0400

> Does anyone have access to a public site we could use to host a git
> tree?  If no one has anything available (or is willing to maintain
> the tree) I might be able to do something.

It's not difficult to get an account on master.kernel.org
and also there has been some success with infradead.org as
well.

James or someone else could help you get going, and he's
gone through this process already :)

Re: [PATCH]: SAD sometimes has double SAs.

[David Miller]

From: Joy Latten <latten@austin.ibm.com>
Date: Wed, 28 Mar 2007 09:15:15 -0600

> Last night I browsed the ipsec-tools code and saw places in
> racoon where improvement would actually fix this
> problem. I am working on a patch and will pursue this 
> on the ipsec-tools mailing list.
> 
> I apologize for any inconvenience. 

No problem, thanks for the update.

> The permission check before flushing does still
> need to be added to kernel. 

Yep, I'll take care of integrating that patch.

Thanks!