netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* pull request (net): ipsec 2013-10-09
@ 2013-10-09 10:59 Steffen Klassert
  2013-10-09 10:59 ` [PATCH 1/7] xfrm: Fix replay size checking on async events Steffen Klassert
                   ` (7 more replies)
  0 siblings, 8 replies; 9+ messages in thread
From: Steffen Klassert @ 2013-10-09 10:59 UTC (permalink / raw)
  To: David Miller; +Cc: Herbert Xu, Steffen Klassert, netdev

1) We used the wrong netlink attribute to verify the
   lenght of the replay window on async events. Fix this by
   using the right netlink attribute.

2) Policy lookups can not match the output interface on forwarding.
   Add the needed informations to the flow informations.

3) We update the pmtu when we receive a ICMPV6_DEST_UNREACH message
   on IPsec with ipv6. This is wrong and leads to strange fragmented
   packets, only ICMPV6_PKT_TOOBIG messages should update the pmtu.
   Fix this by removing the ICMPV6_DEST_UNREACH check from the IPsec
   protocol error handlers.

4) The legacy IPsec anti replay mechanism supports anti replay
   windows up to 32 packets. If a user requests for a bigger
   anti replay window, we use 32 packets but pretend that we use
   the requested window size. Fix from Fan Du.

5) If asynchronous events are enabled and replay_maxdiff is set to
   zero, we generate an async event for every received packet instead
   of checking whether a timeout occurred. Fix from Thomas Egerer.

6) Policies need a refcount when the state resolution timer is armed.
   Otherwise the timer can fire after the policy is deleted.

7) We might dreference a NULL pointer if the hold_queue is empty,
   add a check to avoid this.

Please pull or let me know if there are problems.

Thanks!

The following changes since commit 73a695f8572e4c46a2aecdbb63f26f36a43e6873:

  cxgb4: remove workqueue when driver registration fails (2013-09-15 22:28:58 -0400)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec.git master

for you to fetch changes up to 2bb53e2557964c2c5368a0392cf3b3b63a288cd0:

  xfrm: check for a vaild skb in xfrm_policy_queue_process (2013-10-08 10:49:51 +0200)

----------------------------------------------------------------
Fan Du (1):
      xfrm: Guard IPsec anti replay window against replay bitmap

Steffen Klassert (5):
      xfrm: Fix replay size checking on async events
      xfrm: Decode sessions with output interface.
      ipsec: Don't update the pmtu on ICMPV6_DEST_UNREACH
      xfrm: Add refcount handling to queued policies
      xfrm: check for a vaild skb in xfrm_policy_queue_process

Thomas Egerer (1):
      xfrm: Fix aevent generation for each received packet

 net/ipv4/xfrm4_policy.c |    1 +
 net/ipv6/ah6.c          |    3 +--
 net/ipv6/esp6.c         |    3 +--
 net/ipv6/ipcomp6.c      |    3 +--
 net/ipv6/xfrm6_policy.c |    1 +
 net/key/af_key.c        |    3 ++-
 net/xfrm/xfrm_policy.c  |   28 ++++++++++++++++++------
 net/xfrm/xfrm_replay.c  |   54 ++++++++++++++++++++++++-----------------------
 net/xfrm/xfrm_user.c    |    5 +++--
 9 files changed, 59 insertions(+), 42 deletions(-)

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/7] xfrm: Fix replay size checking on async events
  2013-10-09 10:59 pull request (net): ipsec 2013-10-09 Steffen Klassert
@ 2013-10-09 10:59 ` Steffen Klassert
  2013-10-09 10:59 ` [PATCH 2/7] xfrm: Decode sessions with output interface Steffen Klassert
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Steffen Klassert @ 2013-10-09 10:59 UTC (permalink / raw)
  To: David Miller; +Cc: Herbert Xu, Steffen Klassert, netdev

We pass the wrong netlink attribute to xfrm_replay_verify_len().
It should be XFRMA_REPLAY_ESN_VAL and not XFRMA_REPLAY_VAL as
we currently doing. This causes memory corruptions if the
replay esn attribute has incorrect length. Fix this by passing
the right attribute to xfrm_replay_verify_len().

Reported-by: Michael Rossberg <michael.rossberg@tu-ilmenau.de>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/xfrm/xfrm_user.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 3f565e4..4b26cee 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1856,7 +1856,7 @@ static int xfrm_new_ae(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (x->km.state != XFRM_STATE_VALID)
 		goto out;
 
-	err = xfrm_replay_verify_len(x->replay_esn, rp);
+	err = xfrm_replay_verify_len(x->replay_esn, re);
 	if (err)
 		goto out;
 
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/7] xfrm: Decode sessions with output interface.
  2013-10-09 10:59 pull request (net): ipsec 2013-10-09 Steffen Klassert
  2013-10-09 10:59 ` [PATCH 1/7] xfrm: Fix replay size checking on async events Steffen Klassert
@ 2013-10-09 10:59 ` Steffen Klassert
  2013-10-09 10:59 ` [PATCH 3/7] ipsec: Don't update the pmtu on ICMPV6_DEST_UNREACH Steffen Klassert
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Steffen Klassert @ 2013-10-09 10:59 UTC (permalink / raw)
  To: David Miller; +Cc: Herbert Xu, Steffen Klassert, netdev

The output interface matching does not work on forward
policy lookups, the output interface of the flowi is
always 0. Fix this by setting the output interface when
we decode the session.

Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/ipv4/xfrm4_policy.c |    1 +
 net/ipv6/xfrm6_policy.c |    1 +
 2 files changed, 2 insertions(+)

diff --git a/net/ipv4/xfrm4_policy.c b/net/ipv4/xfrm4_policy.c
index 9a459be..ccde542 100644
--- a/net/ipv4/xfrm4_policy.c
+++ b/net/ipv4/xfrm4_policy.c
@@ -107,6 +107,7 @@ _decode_session4(struct sk_buff *skb, struct flowi *fl, int reverse)
 
 	memset(fl4, 0, sizeof(struct flowi4));
 	fl4->flowi4_mark = skb->mark;
+	fl4->flowi4_oif = skb_dst(skb)->dev->ifindex;
 
 	if (!ip_is_fragment(iph)) {
 		switch (iph->protocol) {
diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c
index 23ed03d..08ed277 100644
--- a/net/ipv6/xfrm6_policy.c
+++ b/net/ipv6/xfrm6_policy.c
@@ -138,6 +138,7 @@ _decode_session6(struct sk_buff *skb, struct flowi *fl, int reverse)
 
 	memset(fl6, 0, sizeof(struct flowi6));
 	fl6->flowi6_mark = skb->mark;
+	fl6->flowi6_oif = skb_dst(skb)->dev->ifindex;
 
 	fl6->daddr = reverse ? hdr->saddr : hdr->daddr;
 	fl6->saddr = reverse ? hdr->daddr : hdr->saddr;
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 3/7] ipsec: Don't update the pmtu on ICMPV6_DEST_UNREACH
  2013-10-09 10:59 pull request (net): ipsec 2013-10-09 Steffen Klassert
  2013-10-09 10:59 ` [PATCH 1/7] xfrm: Fix replay size checking on async events Steffen Klassert
  2013-10-09 10:59 ` [PATCH 2/7] xfrm: Decode sessions with output interface Steffen Klassert
@ 2013-10-09 10:59 ` Steffen Klassert
  2013-10-09 10:59 ` [PATCH 4/7] xfrm: Guard IPsec anti replay window against replay bitmap Steffen Klassert
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Steffen Klassert @ 2013-10-09 10:59 UTC (permalink / raw)
  To: David Miller; +Cc: Herbert Xu, Steffen Klassert, netdev

Currently we update the pmtu in the IPsec protocol error handlers
if icmpv6 message type is either ICMPV6_DEST_UNREACH or
ICMPV6_PKT_TOOBIG. Updating the pmtu on ICMPV6_DEST_UNREACH
is wrong in any case, it causes strangely fragmented packets.
Only ICMPV6_PKT_TOOBIG signalizes pmtu discovery, so remove the
ICMPV6_DEST_UNREACH check in the IPsec protocol error handlers.

Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/ipv6/ah6.c     |    3 +--
 net/ipv6/esp6.c    |    3 +--
 net/ipv6/ipcomp6.c |    3 +--
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/net/ipv6/ah6.c b/net/ipv6/ah6.c
index 73784c3..82e1da3 100644
--- a/net/ipv6/ah6.c
+++ b/net/ipv6/ah6.c
@@ -618,8 +618,7 @@ static void ah6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 	struct ip_auth_hdr *ah = (struct ip_auth_hdr*)(skb->data+offset);
 	struct xfrm_state *x;
 
-	if (type != ICMPV6_DEST_UNREACH &&
-	    type != ICMPV6_PKT_TOOBIG &&
+	if (type != ICMPV6_PKT_TOOBIG &&
 	    type != NDISC_REDIRECT)
 		return;
 
diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
index d3618a7..e67e63f 100644
--- a/net/ipv6/esp6.c
+++ b/net/ipv6/esp6.c
@@ -436,8 +436,7 @@ static void esp6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 	struct ip_esp_hdr *esph = (struct ip_esp_hdr *)(skb->data + offset);
 	struct xfrm_state *x;
 
-	if (type != ICMPV6_DEST_UNREACH &&
-	    type != ICMPV6_PKT_TOOBIG &&
+	if (type != ICMPV6_PKT_TOOBIG &&
 	    type != NDISC_REDIRECT)
 		return;
 
diff --git a/net/ipv6/ipcomp6.c b/net/ipv6/ipcomp6.c
index 5636a91..ce507d9 100644
--- a/net/ipv6/ipcomp6.c
+++ b/net/ipv6/ipcomp6.c
@@ -64,8 +64,7 @@ static void ipcomp6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 		(struct ip_comp_hdr *)(skb->data + offset);
 	struct xfrm_state *x;
 
-	if (type != ICMPV6_DEST_UNREACH &&
-	    type != ICMPV6_PKT_TOOBIG &&
+	if (type != ICMPV6_PKT_TOOBIG &&
 	    type != NDISC_REDIRECT)
 		return;
 
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 4/7] xfrm: Guard IPsec anti replay window against replay bitmap
  2013-10-09 10:59 pull request (net): ipsec 2013-10-09 Steffen Klassert
                   ` (2 preceding siblings ...)
  2013-10-09 10:59 ` [PATCH 3/7] ipsec: Don't update the pmtu on ICMPV6_DEST_UNREACH Steffen Klassert
@ 2013-10-09 10:59 ` Steffen Klassert
  2013-10-09 10:59 ` [PATCH 5/7] xfrm: Fix aevent generation for each received packet Steffen Klassert
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Steffen Klassert @ 2013-10-09 10:59 UTC (permalink / raw)
  To: David Miller; +Cc: Herbert Xu, Steffen Klassert, netdev

From: Fan Du <fan.du@windriver.com>

For legacy IPsec anti replay mechanism:

bitmap in struct xfrm_replay_state could only provide a 32 bits
window size limit in current design, thus user level parameter
sadb_sa_replay should honor this limit, otherwise misleading
outputs("replay=244") by setkey -D will be:

192.168.25.2 192.168.22.2
	esp mode=transport spi=147561170(0x08cb9ad2) reqid=0(0x00000000)
	E: aes-cbc  9a8d7468 7655cf0b 719d27be b0ddaac2
	A: hmac-sha1  2d2115c2 ebf7c126 1c54f186 3b139b58 264a7331
	seq=0x00000000 replay=244 flags=0x00000000 state=mature
	created: Sep 17 14:00:00 2013	current: Sep 17 14:00:22 2013
	diff: 22(s)	hard: 30(s)	soft: 26(s)
	last: Sep 17 14:00:00 2013	hard: 0(s)	soft: 0(s)
	current: 1408(bytes)	hard: 0(bytes)	soft: 0(bytes)
	allocated: 22	hard: 0	soft: 0
	sadb_seq=1 pid=4854 refcnt=0
192.168.22.2 192.168.25.2
	esp mode=transport spi=255302123(0x0f3799eb) reqid=0(0x00000000)
	E: aes-cbc  6485d990 f61a6bd5 e5660252 608ad282
	A: hmac-sha1  0cca811a eb4fa893 c47ae56c 98f6e413 87379a88
	seq=0x00000000 replay=244 flags=0x00000000 state=mature
	created: Sep 17 14:00:00 2013	current: Sep 17 14:00:22 2013
	diff: 22(s)	hard: 30(s)	soft: 26(s)
	last: Sep 17 14:00:00 2013	hard: 0(s)	soft: 0(s)
	current: 1408(bytes)	hard: 0(bytes)	soft: 0(bytes)
	allocated: 22	hard: 0	soft: 0
	sadb_seq=0 pid=4854 refcnt=0

And also, optimizing xfrm_replay_check window checking by setting the
desirable x->props.replay_window with only doing the comparison once
for all when xfrm_state is first born.

Signed-off-by: Fan Du <fan.du@windriver.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/key/af_key.c       |    3 ++-
 net/xfrm/xfrm_replay.c |    3 +--
 net/xfrm/xfrm_user.c   |    3 ++-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/key/af_key.c b/net/key/af_key.c
index 9d58537..911ef03 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -1098,7 +1098,8 @@ static struct xfrm_state * pfkey_msg2xfrm_state(struct net *net,
 
 	x->id.proto = proto;
 	x->id.spi = sa->sadb_sa_spi;
-	x->props.replay_window = sa->sadb_sa_replay;
+	x->props.replay_window = min_t(unsigned int, sa->sadb_sa_replay,
+					(sizeof(x->replay.bitmap) * 8));
 	if (sa->sadb_sa_flags & SADB_SAFLAGS_NOECN)
 		x->props.flags |= XFRM_STATE_NOECN;
 	if (sa->sadb_sa_flags & SADB_SAFLAGS_DECAP_DSCP)
diff --git a/net/xfrm/xfrm_replay.c b/net/xfrm/xfrm_replay.c
index 8dafe6d3..eeca388 100644
--- a/net/xfrm/xfrm_replay.c
+++ b/net/xfrm/xfrm_replay.c
@@ -129,8 +129,7 @@ static int xfrm_replay_check(struct xfrm_state *x,
 		return 0;
 
 	diff = x->replay.seq - seq;
-	if (diff >= min_t(unsigned int, x->props.replay_window,
-			  sizeof(x->replay.bitmap) * 8)) {
+	if (diff >= x->props.replay_window) {
 		x->stats.replay_window++;
 		goto err;
 	}
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 4b26cee..f964d4c 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -446,7 +446,8 @@ static void copy_from_user_state(struct xfrm_state *x, struct xfrm_usersa_info *
 	memcpy(&x->sel, &p->sel, sizeof(x->sel));
 	memcpy(&x->lft, &p->lft, sizeof(x->lft));
 	x->props.mode = p->mode;
-	x->props.replay_window = p->replay_window;
+	x->props.replay_window = min_t(unsigned int, p->replay_window,
+					sizeof(x->replay.bitmap) * 8);
 	x->props.reqid = p->reqid;
 	x->props.family = p->family;
 	memcpy(&x->props.saddr, &p->saddr, sizeof(x->props.saddr));
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 5/7] xfrm: Fix aevent generation for each received packet
  2013-10-09 10:59 pull request (net): ipsec 2013-10-09 Steffen Klassert
                   ` (3 preceding siblings ...)
  2013-10-09 10:59 ` [PATCH 4/7] xfrm: Guard IPsec anti replay window against replay bitmap Steffen Klassert
@ 2013-10-09 10:59 ` Steffen Klassert
  2013-10-09 10:59 ` [PATCH 6/7] xfrm: Add refcount handling to queued policies Steffen Klassert
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Steffen Klassert @ 2013-10-09 10:59 UTC (permalink / raw)
  To: David Miller; +Cc: Herbert Xu, Steffen Klassert, netdev

From: Thomas Egerer <thomas.egerer@secunet.com>

If asynchronous events are enabled for a particular netlink socket,
the notify function is called by the advance function. The notify
function creates and dispatches a km_event if a replay timeout occurred,
or at least replay_maxdiff packets have been received since the last
asynchronous event has been sent. The function is supposed to return if
neither of the two events were detected for a state, or replay_maxdiff
is equal to zero.
Replay_maxdiff is initialized in xfrm_state_construct to the value of
the xfrm.sysctl_aevent_rseqth (2 by default), and updated if for a state
if the netlink attribute XFRMA_REPLAY_THRESH is set.
If, however, replay_maxdiff is set to zero, then all of the three notify
implementations perform a break from the switch statement instead of
checking whether a timeout occurred, and -- if not -- return.  As a
result an asynchronous event is generated for every replay update of a
state that has a zero replay_maxdiff value.
This patch modifies the notify functions such that they immediately
return if replay_maxdiff has the value zero, unless a timeout occurred.

Signed-off-by: Thomas Egerer <thomas.egerer@secunet.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/xfrm/xfrm_replay.c |   51 +++++++++++++++++++++++++-----------------------
 1 file changed, 27 insertions(+), 24 deletions(-)

diff --git a/net/xfrm/xfrm_replay.c b/net/xfrm/xfrm_replay.c
index eeca388..dab57da 100644
--- a/net/xfrm/xfrm_replay.c
+++ b/net/xfrm/xfrm_replay.c
@@ -61,9 +61,9 @@ static void xfrm_replay_notify(struct xfrm_state *x, int event)
 
 	switch (event) {
 	case XFRM_REPLAY_UPDATE:
-		if (x->replay_maxdiff &&
-		    (x->replay.seq - x->preplay.seq < x->replay_maxdiff) &&
-		    (x->replay.oseq - x->preplay.oseq < x->replay_maxdiff)) {
+		if (!x->replay_maxdiff ||
+		    ((x->replay.seq - x->preplay.seq < x->replay_maxdiff) &&
+		    (x->replay.oseq - x->preplay.oseq < x->replay_maxdiff))) {
 			if (x->xflags & XFRM_TIME_DEFER)
 				event = XFRM_REPLAY_TIMEOUT;
 			else
@@ -301,9 +301,10 @@ static void xfrm_replay_notify_bmp(struct xfrm_state *x, int event)
 
 	switch (event) {
 	case XFRM_REPLAY_UPDATE:
-		if (x->replay_maxdiff &&
-		    (replay_esn->seq - preplay_esn->seq < x->replay_maxdiff) &&
-		    (replay_esn->oseq - preplay_esn->oseq < x->replay_maxdiff)) {
+		if (!x->replay_maxdiff ||
+		    ((replay_esn->seq - preplay_esn->seq < x->replay_maxdiff) &&
+		    (replay_esn->oseq - preplay_esn->oseq
+		     < x->replay_maxdiff))) {
 			if (x->xflags & XFRM_TIME_DEFER)
 				event = XFRM_REPLAY_TIMEOUT;
 			else
@@ -352,28 +353,30 @@ static void xfrm_replay_notify_esn(struct xfrm_state *x, int event)
 
 	switch (event) {
 	case XFRM_REPLAY_UPDATE:
-		if (!x->replay_maxdiff)
-			break;
-
-		if (replay_esn->seq_hi == preplay_esn->seq_hi)
-			seq_diff = replay_esn->seq - preplay_esn->seq;
-		else
-			seq_diff = ~preplay_esn->seq + replay_esn->seq + 1;
-
-		if (replay_esn->oseq_hi == preplay_esn->oseq_hi)
-			oseq_diff = replay_esn->oseq - preplay_esn->oseq;
-		else
-			oseq_diff = ~preplay_esn->oseq + replay_esn->oseq + 1;
-
-		if (seq_diff < x->replay_maxdiff &&
-		    oseq_diff < x->replay_maxdiff) {
+		if (x->replay_maxdiff) {
+			if (replay_esn->seq_hi == preplay_esn->seq_hi)
+				seq_diff = replay_esn->seq - preplay_esn->seq;
+			else
+				seq_diff = ~preplay_esn->seq + replay_esn->seq
+					   + 1;
 
-			if (x->xflags & XFRM_TIME_DEFER)
-				event = XFRM_REPLAY_TIMEOUT;
+			if (replay_esn->oseq_hi == preplay_esn->oseq_hi)
+				oseq_diff = replay_esn->oseq
+					    - preplay_esn->oseq;
 			else
-				return;
+				oseq_diff = ~preplay_esn->oseq
+					    + replay_esn->oseq + 1;
+
+			if (seq_diff >= x->replay_maxdiff ||
+			    oseq_diff >= x->replay_maxdiff)
+				break;
 		}
 
+		if (x->xflags & XFRM_TIME_DEFER)
+			event = XFRM_REPLAY_TIMEOUT;
+		else
+			return;
+
 		break;
 
 	case XFRM_REPLAY_TIMEOUT:
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 6/7] xfrm: Add refcount handling to queued policies
  2013-10-09 10:59 pull request (net): ipsec 2013-10-09 Steffen Klassert
                   ` (4 preceding siblings ...)
  2013-10-09 10:59 ` [PATCH 5/7] xfrm: Fix aevent generation for each received packet Steffen Klassert
@ 2013-10-09 10:59 ` Steffen Klassert
  2013-10-09 10:59 ` [PATCH 7/7] xfrm: check for a vaild skb in xfrm_policy_queue_process Steffen Klassert
  2013-10-09 17:44 ` pull request (net): ipsec 2013-10-09 David Miller
  7 siblings, 0 replies; 9+ messages in thread
From: Steffen Klassert @ 2013-10-09 10:59 UTC (permalink / raw)
  To: David Miller; +Cc: Herbert Xu, Steffen Klassert, netdev

We need to ensure that policies can't go away as long as the hold timer
is armed, so take a refcont when we arm the timer and drop one if we
delete it.

Bug was introduced with git commit a0073fe18 ("xfrm: Add a state
resolution packet queue")

Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/xfrm/xfrm_policy.c |   24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index ed38d5d..5f9be97 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -334,7 +334,8 @@ static void xfrm_policy_kill(struct xfrm_policy *policy)
 
 	atomic_inc(&policy->genid);
 
-	del_timer(&policy->polq.hold_timer);
+	if (del_timer(&policy->polq.hold_timer))
+		xfrm_pol_put(policy);
 	xfrm_queue_purge(&policy->polq.hold_queue);
 
 	if (del_timer(&policy->timer))
@@ -589,7 +590,8 @@ static void xfrm_policy_requeue(struct xfrm_policy *old,
 
 	spin_lock_bh(&pq->hold_queue.lock);
 	skb_queue_splice_init(&pq->hold_queue, &list);
-	del_timer(&pq->hold_timer);
+	if (del_timer(&pq->hold_timer))
+		xfrm_pol_put(old);
 	spin_unlock_bh(&pq->hold_queue.lock);
 
 	if (skb_queue_empty(&list))
@@ -600,7 +602,8 @@ static void xfrm_policy_requeue(struct xfrm_policy *old,
 	spin_lock_bh(&pq->hold_queue.lock);
 	skb_queue_splice(&list, &pq->hold_queue);
 	pq->timeout = XFRM_QUEUE_TMO_MIN;
-	mod_timer(&pq->hold_timer, jiffies);
+	if (!mod_timer(&pq->hold_timer, jiffies))
+		xfrm_pol_hold(new);
 	spin_unlock_bh(&pq->hold_queue.lock);
 }
 
@@ -1787,8 +1790,9 @@ static void xfrm_policy_queue_process(unsigned long arg)
 			goto purge_queue;
 
 		pq->timeout = pq->timeout << 1;
-		mod_timer(&pq->hold_timer, jiffies + pq->timeout);
-		return;
+		if (!mod_timer(&pq->hold_timer, jiffies + pq->timeout))
+			xfrm_pol_hold(pol);
+	goto out;
 	}
 
 	dst_release(dst);
@@ -1819,11 +1823,14 @@ static void xfrm_policy_queue_process(unsigned long arg)
 		err = dst_output(skb);
 	}
 
+out:
+	xfrm_pol_put(pol);
 	return;
 
 purge_queue:
 	pq->timeout = 0;
 	xfrm_queue_purge(&pq->hold_queue);
+	xfrm_pol_put(pol);
 }
 
 static int xdst_queue_output(struct sk_buff *skb)
@@ -1831,7 +1838,8 @@ static int xdst_queue_output(struct sk_buff *skb)
 	unsigned long sched_next;
 	struct dst_entry *dst = skb_dst(skb);
 	struct xfrm_dst *xdst = (struct xfrm_dst *) dst;
-	struct xfrm_policy_queue *pq = &xdst->pols[0]->polq;
+	struct xfrm_policy *pol = xdst->pols[0];
+	struct xfrm_policy_queue *pq = &pol->polq;
 
 	if (pq->hold_queue.qlen > XFRM_MAX_QUEUE_LEN) {
 		kfree_skb(skb);
@@ -1850,10 +1858,12 @@ static int xdst_queue_output(struct sk_buff *skb)
 	if (del_timer(&pq->hold_timer)) {
 		if (time_before(pq->hold_timer.expires, sched_next))
 			sched_next = pq->hold_timer.expires;
+		xfrm_pol_put(pol);
 	}
 
 	__skb_queue_tail(&pq->hold_queue, skb);
-	mod_timer(&pq->hold_timer, sched_next);
+	if (!mod_timer(&pq->hold_timer, sched_next))
+		xfrm_pol_hold(pol);
 
 	spin_unlock_bh(&pq->hold_queue.lock);
 
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 7/7] xfrm: check for a vaild skb in xfrm_policy_queue_process
  2013-10-09 10:59 pull request (net): ipsec 2013-10-09 Steffen Klassert
                   ` (5 preceding siblings ...)
  2013-10-09 10:59 ` [PATCH 6/7] xfrm: Add refcount handling to queued policies Steffen Klassert
@ 2013-10-09 10:59 ` Steffen Klassert
  2013-10-09 17:44 ` pull request (net): ipsec 2013-10-09 David Miller
  7 siblings, 0 replies; 9+ messages in thread
From: Steffen Klassert @ 2013-10-09 10:59 UTC (permalink / raw)
  To: David Miller; +Cc: Herbert Xu, Steffen Klassert, netdev

We might dreference a NULL pointer if the hold_queue is empty,
so add a check to avoid this.

Bug was introduced with git commit a0073fe18 ("xfrm: Add a state
resolution packet queue")

Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/xfrm/xfrm_policy.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 5f9be97..76e1873 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -1772,6 +1772,10 @@ static void xfrm_policy_queue_process(unsigned long arg)
 
 	spin_lock(&pq->hold_queue.lock);
 	skb = skb_peek(&pq->hold_queue);
+	if (!skb) {
+		spin_unlock(&pq->hold_queue.lock);
+		goto out;
+	}
 	dst = skb_dst(skb);
 	sk = skb->sk;
 	xfrm_decode_session(skb, &fl, dst->ops->family);
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: pull request (net): ipsec 2013-10-09
  2013-10-09 10:59 pull request (net): ipsec 2013-10-09 Steffen Klassert
                   ` (6 preceding siblings ...)
  2013-10-09 10:59 ` [PATCH 7/7] xfrm: check for a vaild skb in xfrm_policy_queue_process Steffen Klassert
@ 2013-10-09 17:44 ` David Miller
  7 siblings, 0 replies; 9+ messages in thread
From: David Miller @ 2013-10-09 17:44 UTC (permalink / raw)
  To: steffen.klassert; +Cc: herbert, netdev

From: Steffen Klassert <steffen.klassert@secunet.com>
Date: Wed,  9 Oct 2013 12:59:04 +0200

> 1) We used the wrong netlink attribute to verify the
>    lenght of the replay window on async events. Fix this by
>    using the right netlink attribute.
> 
> 2) Policy lookups can not match the output interface on forwarding.
>    Add the needed informations to the flow informations.
> 
> 3) We update the pmtu when we receive a ICMPV6_DEST_UNREACH message
>    on IPsec with ipv6. This is wrong and leads to strange fragmented
>    packets, only ICMPV6_PKT_TOOBIG messages should update the pmtu.
>    Fix this by removing the ICMPV6_DEST_UNREACH check from the IPsec
>    protocol error handlers.
> 
> 4) The legacy IPsec anti replay mechanism supports anti replay
>    windows up to 32 packets. If a user requests for a bigger
>    anti replay window, we use 32 packets but pretend that we use
>    the requested window size. Fix from Fan Du.
> 
> 5) If asynchronous events are enabled and replay_maxdiff is set to
>    zero, we generate an async event for every received packet instead
>    of checking whether a timeout occurred. Fix from Thomas Egerer.
> 
> 6) Policies need a refcount when the state resolution timer is armed.
>    Otherwise the timer can fire after the policy is deleted.
> 
> 7) We might dreference a NULL pointer if the hold_queue is empty,
>    add a check to avoid this.
> 
> Please pull or let me know if there are problems.

Pulled, thanks a lot Steffen.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2013-10-09 17:44 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-09 10:59 pull request (net): ipsec 2013-10-09 Steffen Klassert
2013-10-09 10:59 ` [PATCH 1/7] xfrm: Fix replay size checking on async events Steffen Klassert
2013-10-09 10:59 ` [PATCH 2/7] xfrm: Decode sessions with output interface Steffen Klassert
2013-10-09 10:59 ` [PATCH 3/7] ipsec: Don't update the pmtu on ICMPV6_DEST_UNREACH Steffen Klassert
2013-10-09 10:59 ` [PATCH 4/7] xfrm: Guard IPsec anti replay window against replay bitmap Steffen Klassert
2013-10-09 10:59 ` [PATCH 5/7] xfrm: Fix aevent generation for each received packet Steffen Klassert
2013-10-09 10:59 ` [PATCH 6/7] xfrm: Add refcount handling to queued policies Steffen Klassert
2013-10-09 10:59 ` [PATCH 7/7] xfrm: check for a vaild skb in xfrm_policy_queue_process Steffen Klassert
2013-10-09 17:44 ` pull request (net): ipsec 2013-10-09 David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).