xfrm: Fix initialize repl field of struct xfrm_state

xfrm: Fix initialize repl field of struct xfrm_state

[Wei Yongjun]

Commit 'xfrm: Move IPsec replay detection functions to a separate file'
  (9fdc4883d92d20842c5acea77a4a21bb1574b495)
introduce repl field to struct xfrm_state, and only initialize it
under SA's netlink create path, the other path, such as pf_key, the
repl field remaining uninitialize. So if the SA is created by pf_key,
any input packet with SA's encryption algorithm will cause panic.

    int xfrm_input()
    {
        ...
        x->repl->advance(x, seq);
        ...
    }

This patch fixed to init default xfrm_replay in xfrm_init_state().

Pid: 0, comm: swapper Not tainted 2.6.38-next+ #14 Bochs Bochs
EIP: 0060:[<c078e5d5>] EFLAGS: 00010206 CPU: 0
EIP is at xfrm_input+0x31c/0x4cc
EAX: dd839c00 EBX: 00000084 ECX: 00000000 EDX: 01000000
ESI: dd839c00 EDI: de3a0780 EBP: dec1de88 ESP: dec1de64
 DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
Process swapper (pid: 0, ti=dec1c000 task=c09c0f20 task.ti=c0992000)
Stack:
 00000000 00000000 00000002 c0ba27c0 00100000 01000000 de3a0798 c0ba27c0
 00000033 dec1de98 c0786848 00000000 de3a0780 dec1dea4 c0786868 00000000
 dec1debc c074ee56 e1da6b8c de3a0780 c074ed44 de3a07a8 dec1decc c074ef32
Call Trace:
 [<c0786848>] xfrm4_rcv_encap+0x22/0x27
 [<c0786868>] xfrm4_rcv+0x1b/0x1d
 [<c074ee56>] ip_local_deliver_finish+0x112/0x1b1
 [<c074ed44>] ? ip_local_deliver_finish+0x0/0x1b1
 [<c074ef32>] NF_HOOK.clone.1+0x3d/0x44
 [<c074ef77>] ip_local_deliver+0x3e/0x44
 [<c074ed44>] ? ip_local_deliver_finish+0x0/0x1b1
 [<c074ec03>] ip_rcv_finish+0x30a/0x332
 [<c074e8f9>] ? ip_rcv_finish+0x0/0x332
 [<c074ef32>] NF_HOOK.clone.1+0x3d/0x44
 [<c074f188>] ip_rcv+0x20b/0x247
 [<c074e8f9>] ? ip_rcv_finish+0x0/0x332
 [<c072797d>] __netif_receive_skb+0x373/0x399
 [<c0727bc1>] netif_receive_skb+0x4b/0x51
 [<e0817e2a>] cp_rx_poll+0x210/0x2c4 [8139cp]
 [<c072818f>] net_rx_action+0x9a/0x17d
 [<c0445b5c>] __do_softirq+0xa1/0x149
 [<c0445abb>] ? __do_softirq+0x0/0x149

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
---
 net/xfrm/xfrm_state.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index d575f05..4274e11 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1980,6 +1980,10 @@ int xfrm_init_state(struct xfrm_state *x)
 	if (x->outer_mode == NULL)
 		goto error;
 
+	err = xfrm_init_replay(x);
+	if (err)
+		goto error;
+
 	x->km.state = XFRM_STATE_VALID;
 
 error:
-- 
1.6.5.2

[PATCH] xfrm: Fix initialize repl field of struct xfrm_state

[Wei Yongjun]

Commit 'xfrm: Move IPsec replay detection functions to a separate file'
  (9fdc4883d92d20842c5acea77a4a21bb1574b495)
introduce repl field to struct xfrm_state, and only initialize it
under SA's netlink create path, the other path, such as pf_key, the
repl field remaining uninitialize. So if the SA is created by pf_key,
any input packet with SA's encryption algorithm will cause panic.

    int xfrm_input()
    {
        ...
        x->repl->advance(x, seq);
        ...
    }

This patch fixed to init default xfrm_replay in xfrm_init_state().

Pid: 0, comm: swapper Not tainted 2.6.38-next+ #14 Bochs Bochs
EIP: 0060:[<c078e5d5>] EFLAGS: 00010206 CPU: 0
EIP is at xfrm_input+0x31c/0x4cc
EAX: dd839c00 EBX: 00000084 ECX: 00000000 EDX: 01000000
ESI: dd839c00 EDI: de3a0780 EBP: dec1de88 ESP: dec1de64
 DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
Process swapper (pid: 0, ti=dec1c000 task=c09c0f20 task.ti=c0992000)
Stack:
 00000000 00000000 00000002 c0ba27c0 00100000 01000000 de3a0798 c0ba27c0
 00000033 dec1de98 c0786848 00000000 de3a0780 dec1dea4 c0786868 00000000
 dec1debc c074ee56 e1da6b8c de3a0780 c074ed44 de3a07a8 dec1decc c074ef32
Call Trace:
 [<c0786848>] xfrm4_rcv_encap+0x22/0x27
 [<c0786868>] xfrm4_rcv+0x1b/0x1d
 [<c074ee56>] ip_local_deliver_finish+0x112/0x1b1
 [<c074ed44>] ? ip_local_deliver_finish+0x0/0x1b1
 [<c074ef32>] NF_HOOK.clone.1+0x3d/0x44
 [<c074ef77>] ip_local_deliver+0x3e/0x44
 [<c074ed44>] ? ip_local_deliver_finish+0x0/0x1b1
 [<c074ec03>] ip_rcv_finish+0x30a/0x332
 [<c074e8f9>] ? ip_rcv_finish+0x0/0x332
 [<c074ef32>] NF_HOOK.clone.1+0x3d/0x44
 [<c074f188>] ip_rcv+0x20b/0x247
 [<c074e8f9>] ? ip_rcv_finish+0x0/0x332
 [<c072797d>] __netif_receive_skb+0x373/0x399
 [<c0727bc1>] netif_receive_skb+0x4b/0x51
 [<e0817e2a>] cp_rx_poll+0x210/0x2c4 [8139cp]
 [<c072818f>] net_rx_action+0x9a/0x17d
 [<c0445b5c>] __do_softirq+0xa1/0x149
 [<c0445abb>] ? __do_softirq+0x0/0x149

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
---
 net/xfrm/xfrm_state.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index d575f05..4274e11 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1980,6 +1980,10 @@ int xfrm_init_state(struct xfrm_state *x)
 	if (x->outer_mode == NULL)
 		goto error;
 
+	err = xfrm_init_replay(x);
+	if (err)
+		goto error;
+
 	x->km.state = XFRM_STATE_VALID;
 
 error:
-- 
1.6.5.2

Re: xfrm: Fix initialize repl field of struct xfrm_state

[David Miller]

From: Wei Yongjun <yjwei@cn.fujitsu.com>
Date: Mon, 21 Mar 2011 13:45:39 +0800

> Commit 'xfrm: Move IPsec replay detection functions to a separate file'
>   (9fdc4883d92d20842c5acea77a4a21bb1574b495)
> introduce repl field to struct xfrm_state, and only initialize it
> under SA's netlink create path, the other path, such as pf_key, the
> repl field remaining uninitialize. So if the SA is created by pf_key,
> any input packet with SA's encryption algorithm will cause panic.

Please, either add an xfrm_init_replay() call to the appropriate spot
in net/key/af_key.c or, if possible, only have the one call in
xfrm_init_state().  Don't leave two calls, one in xfrm_user.c and one
in xfrm_state.c

Anyways, I don't think just making one call from xfrm_init_state() is
possible, because the replay settings need to be assigned before we
can properly call xfrm_init_replay().

Therefore, please fix this by adding the necessary call to af_key.c

Thanks.

Re: xfrm: Fix initialize repl field of struct xfrm_state

[Wei Yongjun]

> From: Wei Yongjun <yjwei@cn.fujitsu.com>
> Date: Mon, 21 Mar 2011 13:45:39 +0800
>
>> Commit 'xfrm: Move IPsec replay detection functions to a separate file'
>>   (9fdc4883d92d20842c5acea77a4a21bb1574b495)
>> introduce repl field to struct xfrm_state, and only initialize it
>> under SA's netlink create path, the other path, such as pf_key, the
>> repl field remaining uninitialize. So if the SA is created by pf_key,
>> any input packet with SA's encryption algorithm will cause panic.
> Please, either add an xfrm_init_replay() call to the appropriate spot
> in net/key/af_key.c or, if possible, only have the one call in
> xfrm_init_state().  Don't leave two calls, one in xfrm_user.c and one
> in xfrm_state.c
>
> Anyways, I don't think just making one call from xfrm_init_state() is
> possible, because the replay settings need to be assigned before we
> can properly call xfrm_init_replay().
>
> Therefore, please fix this by adding the necessary call to af_key.c
Sorry for not said clearly, at the first time I want to do like this. 
But when I grep 'xfrm_init_state', it be used in many place, not
any pf_key, but also XFRM MIGRATE, ipcomp, ipcomp6. So I did this ugly
patch by add this to xfrm_init_state() to avoid dup code.

Not sure whether the other case like ipcomp/ipcomp6 etc can cause panic, if
it panic, maybe we can fix by introduce new xfrm_init_replay() function
like to assign the default reply function.
  int xfrm_init_replay(struct xfrm_state *x)
  {
  	x->repl = &xfrm_replay_legacy;
  	return 0;
  }
and change the orig xfrm_init_replay to xfrm_update_replay()? 
Or dup those code to all used place?

If I was wrong, I will fix this by adding the necessary call to af_key.c.

Thanks.


> Thanks.
>

Re: xfrm: Fix initialize repl field of struct xfrm_state

[David Miller]

From: Wei Yongjun <yjwei@cn.fujitsu.com>
Date: Mon, 21 Mar 2011 14:36:45 +0800

> Sorry for not said clearly, at the first time I want to do like this. 
> But when I grep 'xfrm_init_state', it be used in many place, not
> any pf_key, but also XFRM MIGRATE, ipcomp, ipcomp6. So I did this ugly
> patch by add this to xfrm_init_state() to avoid dup code.
> 
> Not sure whether the other case like ipcomp/ipcomp6 etc can cause panic, if
> it panic, maybe we can fix by introduce new xfrm_init_replay() function
> like to assign the default reply function.
>   int xfrm_init_replay(struct xfrm_state *x)
>   {
>   	x->repl = &xfrm_replay_legacy;
>   	return 0;
>   }
> and change the orig xfrm_init_replay to xfrm_update_replay()? 
> Or dup those code to all used place?
> 
> If I was wrong, I will fix this by adding the necessary call to af_key.c.

Ok, thanks for the explanation.

I think there is a simple way out of this:

1) Rename current xfrm_init_state to __xfrm_init_state, add
   "bool init_replay" argument.  Add the xfrm_init_replay()
   call, as in your patch, but conditionalized on this boolean.

2) Implement xfrm_init_state as inline, which calls
   __xfrm_init_state(..., true)

3) Replace xfrm_init_state() call in xfrm_user.c with
   __xfrm_init_state(..., false)

This seems to avoid all the problems.  We don't need to touch every
caller, and we avoid initializing the replay state twice in xfrm_user

Ok?

Re: xfrm: Fix initialize repl field of struct xfrm_state

[Wei Yongjun]

> From: Wei Yongjun <yjwei@cn.fujitsu.com>
> Date: Mon, 21 Mar 2011 14:36:45 +0800
>
>> Sorry for not said clearly, at the first time I want to do like this. 
>> But when I grep 'xfrm_init_state', it be used in many place, not
>> any pf_key, but also XFRM MIGRATE, ipcomp, ipcomp6. So I did this ugly
>> patch by add this to xfrm_init_state() to avoid dup code.
>>
>> Not sure whether the other case like ipcomp/ipcomp6 etc can cause panic, if
>> it panic, maybe we can fix by introduce new xfrm_init_replay() function
>> like to assign the default reply function.
>>   int xfrm_init_replay(struct xfrm_state *x)
>>   {
>>   	x->repl = &xfrm_replay_legacy;
>>   	return 0;
>>   }
>> and change the orig xfrm_init_replay to xfrm_update_replay()? 
>> Or dup those code to all used place?
>>
>> If I was wrong, I will fix this by adding the necessary call to af_key.c.
> Ok, thanks for the explanation.
>
> I think there is a simple way out of this:
>
> 1) Rename current xfrm_init_state to __xfrm_init_state, add
>    "bool init_replay" argument.  Add the xfrm_init_replay()
>    call, as in your patch, but conditionalized on this boolean.
>
> 2) Implement xfrm_init_state as inline, which calls
>    __xfrm_init_state(..., true)
>
> 3) Replace xfrm_init_state() call in xfrm_user.c with
>    __xfrm_init_state(..., false)
>
> This seems to avoid all the problems.  We don't need to touch every
> caller, and we avoid initializing the replay state twice in xfrm_user
>
> Ok?

This OK, I will do this change, thanks.

>
>

[PATCH v2] xfrm: Fix initialize repl field of struct xfrm_state

[Wei Yongjun]

Commit 'xfrm: Move IPsec replay detection functions to a separate file'
  (9fdc4883d92d20842c5acea77a4a21bb1574b495)
introduce repl field to struct xfrm_state, and only initialize it
under SA's netlink create path, the other path, such as pf_key,
ipcomp/ipcomp6 etc, the repl field remaining uninitialize. So if
the SA is created by pf_key, any input packet with SA's encryption
algorithm will cause panic.

    int xfrm_input()
    {
        ...
        x->repl->advance(x, seq);
        ...
    }

This patch fixed it by introduce new function __xfrm_init_state().

Pid: 0, comm: swapper Not tainted 2.6.38-next+ #14 Bochs Bochs
EIP: 0060:[<c078e5d5>] EFLAGS: 00010206 CPU: 0
EIP is at xfrm_input+0x31c/0x4cc
EAX: dd839c00 EBX: 00000084 ECX: 00000000 EDX: 01000000
ESI: dd839c00 EDI: de3a0780 EBP: dec1de88 ESP: dec1de64
 DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
Process swapper (pid: 0, ti=dec1c000 task=c09c0f20 task.ti=c0992000)
Stack:
 00000000 00000000 00000002 c0ba27c0 00100000 01000000 de3a0798 c0ba27c0
 00000033 dec1de98 c0786848 00000000 de3a0780 dec1dea4 c0786868 00000000
 dec1debc c074ee56 e1da6b8c de3a0780 c074ed44 de3a07a8 dec1decc c074ef32
Call Trace:
 [<c0786848>] xfrm4_rcv_encap+0x22/0x27
 [<c0786868>] xfrm4_rcv+0x1b/0x1d
 [<c074ee56>] ip_local_deliver_finish+0x112/0x1b1
 [<c074ed44>] ? ip_local_deliver_finish+0x0/0x1b1
 [<c074ef32>] NF_HOOK.clone.1+0x3d/0x44
 [<c074ef77>] ip_local_deliver+0x3e/0x44
 [<c074ed44>] ? ip_local_deliver_finish+0x0/0x1b1
 [<c074ec03>] ip_rcv_finish+0x30a/0x332
 [<c074e8f9>] ? ip_rcv_finish+0x0/0x332
 [<c074ef32>] NF_HOOK.clone.1+0x3d/0x44
 [<c074f188>] ip_rcv+0x20b/0x247
 [<c074e8f9>] ? ip_rcv_finish+0x0/0x332
 [<c072797d>] __netif_receive_skb+0x373/0x399
 [<c0727bc1>] netif_receive_skb+0x4b/0x51
 [<e0817e2a>] cp_rx_poll+0x210/0x2c4 [8139cp]
 [<c072818f>] net_rx_action+0x9a/0x17d
 [<c0445b5c>] __do_softirq+0xa1/0x149
 [<c0445abb>] ? __do_softirq+0x0/0x149

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
---
 include/net/xfrm.h    |    1 +
 net/xfrm/xfrm_state.c |   13 ++++++++++++-
 net/xfrm/xfrm_user.c  |    2 +-
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 42a8c32..cffa5dc 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1430,6 +1430,7 @@ extern void xfrm_spd_getinfo(struct net *net, struct xfrmk_spdinfo *si);
 extern u32 xfrm_replay_seqhi(struct xfrm_state *x, __be32 net_seq);
 extern int xfrm_init_replay(struct xfrm_state *x);
 extern int xfrm_state_mtu(struct xfrm_state *x, int mtu);
+extern int __xfrm_init_state(struct xfrm_state *x, bool init_replay);
 extern int xfrm_init_state(struct xfrm_state *x);
 extern int xfrm_prepare_input(struct xfrm_state *x, struct sk_buff *skb);
 extern int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi,
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index d575f05..ff96b14 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1907,7 +1907,7 @@ int xfrm_state_mtu(struct xfrm_state *x, int mtu)
 	return res;
 }
 
-int xfrm_init_state(struct xfrm_state *x)
+int __xfrm_init_state(struct xfrm_state *x, bool init_replay)
 {
 	struct xfrm_state_afinfo *afinfo;
 	struct xfrm_mode *inner_mode;
@@ -1980,12 +1980,23 @@ int xfrm_init_state(struct xfrm_state *x)
 	if (x->outer_mode == NULL)
 		goto error;
 
+	if (init_replay) {
+		err = xfrm_init_replay(x);
+		if (err)
+			goto error;
+	}
+
 	x->km.state = XFRM_STATE_VALID;
 
 error:
 	return err;
 }
 
+int xfrm_init_state(struct xfrm_state *x)
+{
+	return __xfrm_init_state(x, true);
+}
+
 EXPORT_SYMBOL(xfrm_init_state);
 
 int __net_init xfrm_state_init(struct net *net)
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 706385a..fc152d2 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -511,7 +511,7 @@ static struct xfrm_state *xfrm_state_construct(struct net *net,
 
 	xfrm_mark_get(attrs, &x->mark);
 
-	err = xfrm_init_state(x);
+	err = __xfrm_init_state(x, false);
 	if (err)
 		goto error;
 
-- 
1.6.5.2

Re: xfrm: Fix initialize repl field of struct xfrm_state

[Steffen Klassert]

Thanks for catching this!

On Mon, Mar 21, 2011 at 02:49:14PM +0800, Wei Yongjun wrote:
> > Ok, thanks for the explanation.
> >
> > I think there is a simple way out of this:
> >
> > 1) Rename current xfrm_init_state to __xfrm_init_state, add
> >    "bool init_replay" argument.  Add the xfrm_init_replay()
> >    call, as in your patch, but conditionalized on this boolean.
> >
> > 2) Implement xfrm_init_state as inline, which calls
> >    __xfrm_init_state(..., true)
> >
> > 3) Replace xfrm_init_state() call in xfrm_user.c with
> >    __xfrm_init_state(..., false)
> >
> > This seems to avoid all the problems.  We don't need to touch every
> > caller, and we avoid initializing the replay state twice in xfrm_user
> >
> > Ok?
> 
> This OK, I will do this change, thanks.
> 

This looks ok for me too. I just noticed that we also need to clone the
replay_esn/preplay_esn informations and to reinitialize the sequence
number counting on XFRM MIGRATE from the original state before we
call xfrm_init_replay.

I'll fix this up once your patch is applied.

Thanks,

Steffen

Re: [PATCH v2] xfrm: Fix initialize repl field of struct xfrm_state

[David Miller]

From: Wei Yongjun <yjwei@cn.fujitsu.com>
Date: Mon, 21 Mar 2011 15:44:03 +0800

> -int xfrm_init_state(struct xfrm_state *x)
> +int __xfrm_init_state(struct xfrm_state *x, bool init_replay)

Since it is referenced by modules, this new function symbol must
be exported to them.

Re: [PATCH v2] xfrm: Fix initialize repl field of struct xfrm_state

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Mon, 21 Mar 2011 01:00:37 -0700 (PDT)

> From: Wei Yongjun <yjwei@cn.fujitsu.com>
> Date: Mon, 21 Mar 2011 15:44:03 +0800
> 
>> -int xfrm_init_state(struct xfrm_state *x)
>> +int __xfrm_init_state(struct xfrm_state *x, bool init_replay)
> 
> Since it is referenced by modules, this new function symbol must
> be exported to them.

BTW, this is one of the reasons I explicitly mentioned to make
this an inline function. :-)

Re: xfrm: Fix initialize repl field of struct xfrm_state

[Steffen Klassert]

On Sun, Mar 20, 2011 at 11:46:06PM -0700, David Miller wrote:
> 
> Ok, thanks for the explanation.
> 
> I think there is a simple way out of this:
> 
> 1) Rename current xfrm_init_state to __xfrm_init_state, add
>    "bool init_replay" argument.  Add the xfrm_init_replay()
>    call, as in your patch, but conditionalized on this boolean.
> 
> 2) Implement xfrm_init_state as inline, which calls
>    __xfrm_init_state(..., true)
> 
> 3) Replace xfrm_init_state() call in xfrm_user.c with
>    __xfrm_init_state(..., false)
> 
> This seems to avoid all the problems.  We don't need to touch every
> caller, and we avoid initializing the replay state twice in xfrm_user
> 

Btw, looking a bit closer to this. I think it would look a bit cleaner
if we would add the xfrm_init_replay() call to xfrm_init_state() and
to move the xfrm_init_state() call in xfrm_state_construct() behind
the assign of the replay settings.

[PATCH v3] xfrm: Fix initialize repl field of struct xfrm_state

[Wei Yongjun]

Commit 'xfrm: Move IPsec replay detection functions to a separate file'
  (9fdc4883d92d20842c5acea77a4a21bb1574b495)
introduce repl field to struct xfrm_state, and only initialize it
under SA's netlink create path, the other path, such as pf_key,
ipcomp/ipcomp6 etc, the repl field remaining uninitialize. So if
the SA is created by pf_key, any input packet with SA's encryption
algorithm will cause panic.

    int xfrm_input()
    {
        ...
        x->repl->advance(x, seq);
        ...
    }

This patch fixed it by introduce new function __xfrm_init_state().

Pid: 0, comm: swapper Not tainted 2.6.38-next+ #14 Bochs Bochs
EIP: 0060:[<c078e5d5>] EFLAGS: 00010206 CPU: 0
EIP is at xfrm_input+0x31c/0x4cc
EAX: dd839c00 EBX: 00000084 ECX: 00000000 EDX: 01000000
ESI: dd839c00 EDI: de3a0780 EBP: dec1de88 ESP: dec1de64
 DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
Process swapper (pid: 0, ti=dec1c000 task=c09c0f20 task.ti=c0992000)
Stack:
 00000000 00000000 00000002 c0ba27c0 00100000 01000000 de3a0798 c0ba27c0
 00000033 dec1de98 c0786848 00000000 de3a0780 dec1dea4 c0786868 00000000
 dec1debc c074ee56 e1da6b8c de3a0780 c074ed44 de3a07a8 dec1decc c074ef32
Call Trace:
 [<c0786848>] xfrm4_rcv_encap+0x22/0x27
 [<c0786868>] xfrm4_rcv+0x1b/0x1d
 [<c074ee56>] ip_local_deliver_finish+0x112/0x1b1
 [<c074ed44>] ? ip_local_deliver_finish+0x0/0x1b1
 [<c074ef32>] NF_HOOK.clone.1+0x3d/0x44
 [<c074ef77>] ip_local_deliver+0x3e/0x44
 [<c074ed44>] ? ip_local_deliver_finish+0x0/0x1b1
 [<c074ec03>] ip_rcv_finish+0x30a/0x332
 [<c074e8f9>] ? ip_rcv_finish+0x0/0x332
 [<c074ef32>] NF_HOOK.clone.1+0x3d/0x44
 [<c074f188>] ip_rcv+0x20b/0x247
 [<c074e8f9>] ? ip_rcv_finish+0x0/0x332
 [<c072797d>] __netif_receive_skb+0x373/0x399
 [<c0727bc1>] netif_receive_skb+0x4b/0x51
 [<e0817e2a>] cp_rx_poll+0x210/0x2c4 [8139cp]
 [<c072818f>] net_rx_action+0x9a/0x17d
 [<c0445b5c>] __do_softirq+0xa1/0x149
 [<c0445abb>] ? __do_softirq+0x0/0x149

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
---
Changelog:
  v2 -> v3 export __xfrm_init_state().
---
 include/net/xfrm.h    |    1 +
 net/xfrm/xfrm_state.c |   15 ++++++++++++++-
 net/xfrm/xfrm_user.c  |    2 +-
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 42a8c32..cffa5dc 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1430,6 +1430,7 @@ extern void xfrm_spd_getinfo(struct net *net, struct xfrmk_spdinfo *si);
 extern u32 xfrm_replay_seqhi(struct xfrm_state *x, __be32 net_seq);
 extern int xfrm_init_replay(struct xfrm_state *x);
 extern int xfrm_state_mtu(struct xfrm_state *x, int mtu);
+extern int __xfrm_init_state(struct xfrm_state *x, bool init_replay);
 extern int xfrm_init_state(struct xfrm_state *x);
 extern int xfrm_prepare_input(struct xfrm_state *x, struct sk_buff *skb);
 extern int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi,
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index d575f05..f83a3d1 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1907,7 +1907,7 @@ int xfrm_state_mtu(struct xfrm_state *x, int mtu)
 	return res;
 }
 
-int xfrm_init_state(struct xfrm_state *x)
+int __xfrm_init_state(struct xfrm_state *x, bool init_replay)
 {
 	struct xfrm_state_afinfo *afinfo;
 	struct xfrm_mode *inner_mode;
@@ -1980,12 +1980,25 @@ int xfrm_init_state(struct xfrm_state *x)
 	if (x->outer_mode == NULL)
 		goto error;
 
+	if (init_replay) {
+		err = xfrm_init_replay(x);
+		if (err)
+			goto error;
+	}
+
 	x->km.state = XFRM_STATE_VALID;
 
 error:
 	return err;
 }
 
+EXPORT_SYMBOL(__xfrm_init_state);
+
+int xfrm_init_state(struct xfrm_state *x)
+{
+	return __xfrm_init_state(x, true);
+}
+
 EXPORT_SYMBOL(xfrm_init_state);
 
 int __net_init xfrm_state_init(struct net *net)
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 706385a..fc152d2 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -511,7 +511,7 @@ static struct xfrm_state *xfrm_state_construct(struct net *net,
 
 	xfrm_mark_get(attrs, &x->mark);
 
-	err = xfrm_init_state(x);
+	err = __xfrm_init_state(x, false);
 	if (err)
 		goto error;
 
-- 
1.6.5.2

Re: xfrm: Fix initialize repl field of struct xfrm_state

[Wei Yongjun]

> On Sun, Mar 20, 2011 at 11:46:06PM -0700, David Miller wrote:
>> Ok, thanks for the explanation.
>>
>> I think there is a simple way out of this:
>>
>> 1) Rename current xfrm_init_state to __xfrm_init_state, add
>>    "bool init_replay" argument.  Add the xfrm_init_replay()
>>    call, as in your patch, but conditionalized on this boolean.
>>
>> 2) Implement xfrm_init_state as inline, which calls
>>    __xfrm_init_state(..., true)
>>
>> 3) Replace xfrm_init_state() call in xfrm_user.c with
>>    __xfrm_init_state(..., false)
>>
>> This seems to avoid all the problems.  We don't need to touch every
>> caller, and we avoid initializing the replay state twice in xfrm_user
>>
> Btw, looking a bit closer to this. I think it would look a bit cleaner
> if we would add the xfrm_init_replay() call to xfrm_init_state() and
> to move the xfrm_init_state() call in xfrm_state_construct() behind
> the assign of the replay settings.

The xfrm_init_replay() should be call after the call to
  xfrm_update_ae_params(x, attrs);
since xfrm_update_ae_params() may update the replay_esn.

So we need move the xfrm_init_state()  call just before return x.

The other issue:
static void xfrm_update_ae_params()
{
        ...
        memcpy(x->replay_esn, replay_esn,
                       xfrm_replay_state_esn_len(replay_esn));
       ...
}

the memcpy() may cause memory overlap if we build a special
nl_data, we should free it and then do kmemdup()?

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

Re: xfrm: Fix initialize repl field of struct xfrm_state

[Wei Yongjun]

>> On Sun, Mar 20, 2011 at 11:46:06PM -0700, David Miller wrote:
>>> Ok, thanks for the explanation.
>>>
>>> I think there is a simple way out of this:
>>>
>>> 1) Rename current xfrm_init_state to __xfrm_init_state, add
>>>    "bool init_replay" argument.  Add the xfrm_init_replay()
>>>    call, as in your patch, but conditionalized on this boolean.
>>>
>>> 2) Implement xfrm_init_state as inline, which calls
>>>    __xfrm_init_state(..., true)
>>>
>>> 3) Replace xfrm_init_state() call in xfrm_user.c with
>>>    __xfrm_init_state(..., false)
>>>
>>> This seems to avoid all the problems.  We don't need to touch every
>>> caller, and we avoid initializing the replay state twice in xfrm_user
>>>
>> Btw, looking a bit closer to this. I think it would look a bit cleaner
>> if we would add the xfrm_init_replay() call to xfrm_init_state() and
>> to move the xfrm_init_state() call in xfrm_state_construct() behind
>> the assign of the replay settings.
> The xfrm_init_replay() should be call after the call to
>   xfrm_update_ae_params(x, attrs);
> since xfrm_update_ae_params() may update the replay_esn.
>
> So we need move the xfrm_init_state()  call just before return x.


Oh, sorry, the memcpy looks like dup code since we used
kmemdup. It is the same attr XFRMA_REPLAY_ESN_VAL.

> The other issue:
> static void xfrm_update_ae_params()
> {
>         ...
>         memcpy(x->replay_esn, replay_esn,
>                        xfrm_replay_state_esn_len(replay_esn));
>        ...
> }
>
> the memcpy() may cause memory overlap if we build a special
> nl_data, we should free it and then do kmemdup()?
>
>> --
>> 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
>>
> --
> 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
>

Re: xfrm: Fix initialize repl field of struct xfrm_state

[Steffen Klassert]

On Mon, Mar 21, 2011 at 05:18:18PM +0800, Wei Yongjun wrote:
> 
> >> Btw, looking a bit closer to this. I think it would look a bit cleaner
> >> if we would add the xfrm_init_replay() call to xfrm_init_state() and
> >> to move the xfrm_init_state() call in xfrm_state_construct() behind
> >> the assign of the replay settings.
> > The xfrm_init_replay() should be call after the call to
> >   xfrm_update_ae_params(x, attrs);
> > since xfrm_update_ae_params() may update the replay_esn.
> >
> > So we need move the xfrm_init_state()  call just before return x.
> 
> 
> Oh, sorry, the memcpy looks like dup code since we used
> kmemdup. It is the same attr XFRMA_REPLAY_ESN_VAL.
> 

Indeed, we don't need the memcpy here because we do a kmemdup when we
allocate repay_esn/preplay_esn. But we need to memcpy if we call
xfrm_update_ae_params() from xfrm_new_ae().

So we could just replace the kmemdup by kmalloc when we allocate
repay_esn/preplay_esn and move xfrm_init_state() at the end of the
function, as you suggested. xfrm_update_ae_params() would initialize
x->replay_esn and x->preplay_esn properly then.

> > The other issue:
> > static void xfrm_update_ae_params()
> > {
> >         ...
> >         memcpy(x->replay_esn, replay_esn,
> >                        xfrm_replay_state_esn_len(replay_esn));
> >        ...
> > }
> >
> > the memcpy() may cause memory overlap if we build a special
> > nl_data, we should free it and then do kmemdup()?
> >

Re: xfrm: Fix initialize repl field of struct xfrm_state

[Wei Yongjun]

> On Mon, Mar 21, 2011 at 05:18:18PM +0800, Wei Yongjun wrote:
>>>> Btw, looking a bit closer to this. I think it would look a bit cleaner
>>>> if we would add the xfrm_init_replay() call to xfrm_init_state() and
>>>> to move the xfrm_init_state() call in xfrm_state_construct() behind
>>>> the assign of the replay settings.
>>> The xfrm_init_replay() should be call after the call to
>>>   xfrm_update_ae_params(x, attrs);
>>> since xfrm_update_ae_params() may update the replay_esn.
>>>
>>> So we need move the xfrm_init_state()  call just before return x.
>>
>> Oh, sorry, the memcpy looks like dup code since we used
>> kmemdup. It is the same attr XFRMA_REPLAY_ESN_VAL.
>>
> Indeed, we don't need the memcpy here because we do a kmemdup when we
> allocate repay_esn/preplay_esn. But we need to memcpy if we call
> xfrm_update_ae_params() from xfrm_new_ae().
>
> So we could just replace the kmemdup by kmalloc when we allocate
> repay_esn/preplay_esn and move xfrm_init_state() at the end of the
> function, as you suggested. xfrm_update_ae_params() would initialize
> x->replay_esn and x->preplay_esn properly then.

BTW, looking into more about this, another path, XFRM_MSG_NEWAE,
can overwrite the x->replay_esn with the nla_data length, which
may larger then the size we malloc.

>>> The other issue:
>>> static void xfrm_update_ae_params()
>>> {
>>>         ...
>>>         memcpy(x->replay_esn, replay_esn,
>>>                        xfrm_replay_state_esn_len(replay_esn));
>>>        ...
>>> }
>>>
>>> the memcpy() may cause memory overlap if we build a special
>>> nl_data, we should free it and then do kmemdup()?
>>>

Re: [PATCH v3] xfrm: Fix initialize repl field of struct xfrm_state

[David Miller]

From: Wei Yongjun <yjwei@cn.fujitsu.com>
Date: Mon, 21 Mar 2011 16:37:03 +0800

> Commit 'xfrm: Move IPsec replay detection functions to a separate file'
>   (9fdc4883d92d20842c5acea77a4a21bb1574b495)
> introduce repl field to struct xfrm_state, and only initialize it
> under SA's netlink create path, the other path, such as pf_key,
> ipcomp/ipcomp6 etc, the repl field remaining uninitialize. So if
> the SA is created by pf_key, any input packet with SA's encryption
> algorithm will cause panic.

I'll apply this, thanks.

I know you guys are discussing alternative ways to handle this,
but getting the crash fixed in the simplest way now doesn't hurt.

Re: xfrm: Fix initialize repl field of struct xfrm_state

[Steffen Klassert]

On Tue, Mar 22, 2011 at 09:04:38AM +0800, Wei Yongjun wrote:
> 
> BTW, looking into more about this, another path, XFRM_MSG_NEWAE,
> can overwrite the x->replay_esn with the nla_data length, which
> may larger then the size we malloc.
> 

Yes, I've noticed that yesterday too. I'll fix it up.

Thanks!