netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] xfrm: fix crash in XFRM_MSG_GETSA netlink handler
@ 2016-07-05  8:18 Vegard Nossum
  2016-07-05 10:14 ` Nicolas Dichtel
  2016-07-05 19:13 ` David Miller
  0 siblings, 2 replies; 6+ messages in thread
From: Vegard Nossum @ 2016-07-05  8:18 UTC (permalink / raw)
  To: Steffen Klassert, Herbert Xu, Nicolas Dichtel
  Cc: davem, netdev, Vegard Nossum

If we hit any of the error conditions inside xfrm_dump_sa(), then
xfrm_state_walk_init() never gets called. However, we still call
xfrm_state_walk_done() from xfrm_dump_sa_done(), which will crash
because the state walk was never initialized properly.

We can fix this by setting cb->args[0] only after we've processed the
first element and checking this before calling xfrm_state_walk_done().

Fixes: d3623099d3 ("ipsec: add support of limited SA dump")
Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
 net/xfrm/xfrm_user.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index d516845..4fb04ce 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -896,7 +896,8 @@ static int xfrm_dump_sa_done(struct netlink_callback *cb)
 	struct sock *sk = cb->skb->sk;
 	struct net *net = sock_net(sk);
 
-	xfrm_state_walk_done(walk, net);
+	if (cb->args[0])
+		xfrm_state_walk_done(walk, net);
 	return 0;
 }
 
@@ -921,8 +922,6 @@ static int xfrm_dump_sa(struct sk_buff *skb, struct netlink_callback *cb)
 		u8 proto = 0;
 		int err;
 
-		cb->args[0] = 1;
-
 		err = nlmsg_parse(cb->nlh, 0, attrs, XFRMA_MAX,
 				  xfrma_policy);
 		if (err < 0)
@@ -939,6 +938,7 @@ static int xfrm_dump_sa(struct sk_buff *skb, struct netlink_callback *cb)
 			proto = nla_get_u8(attrs[XFRMA_PROTO]);
 
 		xfrm_state_walk_init(walk, proto, filter);
+		cb->args[0] = 1;
 	}
 
 	(void) xfrm_state_walk(net, walk, dump_one_state, &info);
-- 
1.9.1

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

* Re: [PATCH] xfrm: fix crash in XFRM_MSG_GETSA netlink handler
  2016-07-05  8:18 [PATCH] xfrm: fix crash in XFRM_MSG_GETSA netlink handler Vegard Nossum
@ 2016-07-05 10:14 ` Nicolas Dichtel
  2016-07-05 19:13 ` David Miller
  1 sibling, 0 replies; 6+ messages in thread
From: Nicolas Dichtel @ 2016-07-05 10:14 UTC (permalink / raw)
  To: Vegard Nossum, Steffen Klassert, Herbert Xu; +Cc: davem, netdev

Le 05/07/2016 10:18, Vegard Nossum a écrit :
> If we hit any of the error conditions inside xfrm_dump_sa(), then
> xfrm_state_walk_init() never gets called. However, we still call
> xfrm_state_walk_done() from xfrm_dump_sa_done(), which will crash
> because the state walk was never initialized properly.
> 
> We can fix this by setting cb->args[0] only after we've processed the
> first element and checking this before calling xfrm_state_walk_done().
> 
> Fixes: d3623099d3 ("ipsec: add support of limited SA dump")
> Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> Cc: Steffen Klassert <steffen.klassert@secunet.com>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
Acked-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

Thank you for the fix!

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

* Re: [PATCH] xfrm: fix crash in XFRM_MSG_GETSA netlink handler
  2016-07-05  8:18 [PATCH] xfrm: fix crash in XFRM_MSG_GETSA netlink handler Vegard Nossum
  2016-07-05 10:14 ` Nicolas Dichtel
@ 2016-07-05 19:13 ` David Miller
  2016-07-06  2:27   ` Herbert Xu
  1 sibling, 1 reply; 6+ messages in thread
From: David Miller @ 2016-07-05 19:13 UTC (permalink / raw)
  To: vegard.nossum; +Cc: steffen.klassert, herbert, nicolas.dichtel, netdev

From: Vegard Nossum <vegard.nossum@oracle.com>
Date: Tue,  5 Jul 2016 10:18:08 +0200

> If we hit any of the error conditions inside xfrm_dump_sa(), then
> xfrm_state_walk_init() never gets called. However, we still call
> xfrm_state_walk_done() from xfrm_dump_sa_done(), which will crash
> because the state walk was never initialized properly.
> 
> We can fix this by setting cb->args[0] only after we've processed the
> first element and checking this before calling xfrm_state_walk_done().
> 
> Fixes: d3623099d3 ("ipsec: add support of limited SA dump")
> Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> Cc: Steffen Klassert <steffen.klassert@secunet.com>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>

I assume Steffen will pick this up.

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

* Re: [PATCH] xfrm: fix crash in XFRM_MSG_GETSA netlink handler
  2016-07-05 19:13 ` David Miller
@ 2016-07-06  2:27   ` Herbert Xu
  2016-07-10 12:21     ` Steffen Klassert
  0 siblings, 1 reply; 6+ messages in thread
From: Herbert Xu @ 2016-07-06  2:27 UTC (permalink / raw)
  To: David Miller; +Cc: vegard.nossum, steffen.klassert, nicolas.dichtel, netdev

On Tue, Jul 05, 2016 at 12:13:03PM -0700, David Miller wrote:
> From: Vegard Nossum <vegard.nossum@oracle.com>
> Date: Tue,  5 Jul 2016 10:18:08 +0200
> 
> > If we hit any of the error conditions inside xfrm_dump_sa(), then
> > xfrm_state_walk_init() never gets called. However, we still call
> > xfrm_state_walk_done() from xfrm_dump_sa_done(), which will crash
> > because the state walk was never initialized properly.
> > 
> > We can fix this by setting cb->args[0] only after we've processed the
> > first element and checking this before calling xfrm_state_walk_done().
> > 
> > Fixes: d3623099d3 ("ipsec: add support of limited SA dump")
> > Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> > Cc: Steffen Klassert <steffen.klassert@secunet.com>
> > Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> 
> I assume Steffen will pick this up.

I think Steffen said that he is going to be on vacation for two
weeks starting this week.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

* Re: [PATCH] xfrm: fix crash in XFRM_MSG_GETSA netlink handler
  2016-07-06  2:27   ` Herbert Xu
@ 2016-07-10 12:21     ` Steffen Klassert
  2016-07-19  7:35       ` Steffen Klassert
  0 siblings, 1 reply; 6+ messages in thread
From: Steffen Klassert @ 2016-07-10 12:21 UTC (permalink / raw)
  To: Herbert Xu; +Cc: David Miller, vegard.nossum, nicolas.dichtel, netdev

On Wed, Jul 06, 2016 at 10:27:29AM +0800, Herbert Xu wrote:
> On Tue, Jul 05, 2016 at 12:13:03PM -0700, David Miller wrote:
> > From: Vegard Nossum <vegard.nossum@oracle.com>
> > Date: Tue,  5 Jul 2016 10:18:08 +0200
> > 
> > > If we hit any of the error conditions inside xfrm_dump_sa(), then
> > > xfrm_state_walk_init() never gets called. However, we still call
> > > xfrm_state_walk_done() from xfrm_dump_sa_done(), which will crash
> > > because the state walk was never initialized properly.
> > > 
> > > We can fix this by setting cb->args[0] only after we've processed the
> > > first element and checking this before calling xfrm_state_walk_done().
> > > 
> > > Fixes: d3623099d3 ("ipsec: add support of limited SA dump")
> > > Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> > > Cc: Steffen Klassert <steffen.klassert@secunet.com>
> > > Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> > 
> > I assume Steffen will pick this up.
> 
> I think Steffen said that he is going to be on vacation for two
> weeks starting this week.

Right, I'll take care of it as soon as I'm back.

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

* Re: [PATCH] xfrm: fix crash in XFRM_MSG_GETSA netlink handler
  2016-07-10 12:21     ` Steffen Klassert
@ 2016-07-19  7:35       ` Steffen Klassert
  0 siblings, 0 replies; 6+ messages in thread
From: Steffen Klassert @ 2016-07-19  7:35 UTC (permalink / raw)
  To: Herbert Xu; +Cc: David Miller, vegard.nossum, nicolas.dichtel, netdev

On Sun, Jul 10, 2016 at 02:21:13PM +0200, Steffen Klassert wrote:
> On Wed, Jul 06, 2016 at 10:27:29AM +0800, Herbert Xu wrote:
> > On Tue, Jul 05, 2016 at 12:13:03PM -0700, David Miller wrote:
> > > From: Vegard Nossum <vegard.nossum@oracle.com>
> > > Date: Tue,  5 Jul 2016 10:18:08 +0200
> > > 
> > > > If we hit any of the error conditions inside xfrm_dump_sa(), then
> > > > xfrm_state_walk_init() never gets called. However, we still call
> > > > xfrm_state_walk_done() from xfrm_dump_sa_done(), which will crash
> > > > because the state walk was never initialized properly.
> > > > 
> > > > We can fix this by setting cb->args[0] only after we've processed the
> > > > first element and checking this before calling xfrm_state_walk_done().
> > > > 
> > > > Fixes: d3623099d3 ("ipsec: add support of limited SA dump")
> > > > Cc: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> > > > Cc: Steffen Klassert <steffen.klassert@secunet.com>
> > > > Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> > > 
> > > I assume Steffen will pick this up.
> > 
> > I think Steffen said that he is going to be on vacation for two
> > weeks starting this week.
> 
> Right, I'll take care of it as soon as I'm back.

Now applied to the ipsec tree, thanks a lot!

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

end of thread, other threads:[~2016-07-19  7:35 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-05  8:18 [PATCH] xfrm: fix crash in XFRM_MSG_GETSA netlink handler Vegard Nossum
2016-07-05 10:14 ` Nicolas Dichtel
2016-07-05 19:13 ` David Miller
2016-07-06  2:27   ` Herbert Xu
2016-07-10 12:21     ` Steffen Klassert
2016-07-19  7:35       ` Steffen Klassert

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).